nfsd/sunrpc: add support for a workqueue-based nfsd
Subsystems (5)
cover nfsd/sunrpc: add support for a workqueue-based nfsd
tl;dr: this code works and is much simpler than the dedicated thread
pool, but there are some latencies in the workqueue code that seem to keep it from being as fast as it could be.
This patchset is a little skunkworks project that I've been poking at for the last few weeks. Currently nfsd uses a dedicated thread pool to handle RPCs, but that requires maintaining a rather large swath of "fiddly" code to handle the threads and transports.
This patchset represents an alternative approach, which makes nfsd use workqueues to do its bidding rather than a dedicated thread pool. When a transport needs to do work, we simply queue it to the workqueue in softirq context and let it service the transport.
The current draft is runtime-switchable via a new sunrpc pool_mode module parameter setting. When that's set to "workqueue", nfsd will use a workqueue-based service. One of the goals of this patchset was to *not* need to change any userland code, so starting it up using rpc.nfsd still works as expected. The only real difference is that the nfsdfs "threads" file is reinterpreted as the "max_active" value for the workqueue.
This code has a lot of potential to simplify nfsd significantly and I think it may also scale better on larger machines. When testing with an exported tmpfs on my craptacular test machine, the workqueue based code seems to be a little faster than a dedicated thread pool.
Currently though, performance takes a nose dive (~%40) when I'm writing to (relatively slow) SATA disks. With the help of some tracepoints, I think this is mostly due to some significant latency in the workqueue code.
When I queue a thread using the legacy dedicated thread pool, I see ~.2ms of latency between the softirq function queueing it to a given thread and the thread picking that work up. When I queue it to a workqueue however, that latency jumps to ~30ms (average).
My current theory is that this latency interferes with the ability to batch up requests to the disks and that is what accounts for the massive slowdown.
So, I have several goals here in posting this:
1) to get some early feedback on this code. Does this seem reasonable, assuming that we can address the workqueue latency problems?
2) get some insight about the latency from those with a better understanding of the CMWQ code. Any thoughts as to why we might be seeing such high latency here? Any ideas of what we can do about it?
3) I'm also cc'ing Al due to some changes in patch #10 to allow nfsd to manage its fs_structs a little differently. Does that approach seem reasonable?
Jeff Layton (14):
sunrpc: add a new svc_serv_ops struct and move sv_shutdown into it
sunrpc: move sv_function into sv_ops
sunrpc: move sv_module parm into sv_ops
sunrpc: turn enqueueing a svc_xprt into a svc_serv operation
sunrpc: abstract out svc_set_num_threads to sv_ops
sunrpc: move pool_mode definitions into svc.h
sunrpc: factor svc_rqst allocation and freeing from sv_nrthreads
refcounting
sunrpc: set up workqueue function in svc_xprt
sunrpc: add basic support for workqueue-based services
nfsd: keep a reference to the fs_struct in svc_rqst
nfsd: add support for workqueue based service processing
sunrpc: keep a cache of svc_rqsts for each NUMA node
sunrpc: add more tracepoints around svc_xprt handling
sunrpc: add tracepoints around svc_sock handling
fs/fs_struct.c | 60 +++++++-- fs/lockd/svc.c | 7 +- fs/nfs/callback.c | 6 +- fs/nfsd/nfssvc.c | 107 ++++++++++++--- include/linux/fs_struct.h | 4 + include/linux/sunrpc/svc.h | 97 +++++++++++--- include/linux/sunrpc/svc_xprt.h | 3 + include/linux/sunrpc/svcsock.h | 1 + include/trace/events/sunrpc.h | 60 ++++++++- net/sunrpc/Kconfig | 10 ++ net/sunrpc/Makefile | 1 + net/sunrpc/svc.c | 141 +++++++++++--------- net/sunrpc/svc_wq.c | 281 ++++++++++++++++++++++++++++++++++++++++ net/sunrpc/svc_xprt.c | 66 +++++++++- net/sunrpc/svcsock.c | 6 + 15 files changed, 737 insertions(+), 113 deletions(-) create mode 100644 net/sunrpc/svc_wq.c
Signature · 1 line
2.1.0
RFC PATCH 1/14 sunrpc: add a new svc_serv_ops struct and move sv_shutdown into it
In later patches we'll need to abstract out more operations on a per-service level, besides sv_shutdown and sv_function.
Declare a new svc_serv_ops struct to hold these operations, and move sv_shutdown into this struct.
--- fs/lockd/svc.c | 6 +++++- fs/nfs/callback.c | 5 ++++- fs/nfsd/nfssvc.c | 6 +++++- include/linux/sunrpc/svc.h | 20 ++++++++++---------- net/sunrpc/svc.c | 18 +++++++++--------- 5 files changed, 33 insertions(+), 22 deletions(-)
fs/lockd/svc.c +5 -1
@@ -326,6 +326,10 @@ out_rqst:return error; } +static struct svc_serv_ops lockd_sv_ops = { + .svo_shutdown = svc_rpcb_cleanup, +}; +static struct svc_serv *lockd_create_svc (void ) {struct svc_serv *serv;
@@ -350,7 +354,7 @@ static struct svc_serv *lockd_create_svc(void)printk (KERN_WARNING "lockd_up: no pid, %d users??\n" , nlmsvc_users); - serv =svc_create (&nlmsvc_program,LOCKD_BUFSIZE , svc_rpcb_cleanup); + serv =svc_create (&nlmsvc_program,LOCKD_BUFSIZE , &lockd_sv_ops);if (!serv) {printk (KERN_WARNING "lockd_up: create service failed\n" );return ERR_PTR (-ENOMEM );
fs/nfs/callback.c +4 -1
@@ -306,6 +306,9 @@ err_bind:return ret; } +static struct svc_serv_ops nfs_cb_sv_ops = { +}; +static struct svc_serv *nfs_callback_create_svc (int minorversion) {struct nfs_callback_data *cb_info = &nfs_callback_info[minorversion];
@@ -331,7 +334,7 @@ static struct svc_serv *nfs_callback_create_svc(int minorversion)printk (KERN_WARNING "nfs_callback_create_svc: no kthread, %d users??\n" , cb_info->users); - serv =svc_create (&nfs4_callback_program,NFS4_CALLBACK_BUFSIZE ,NULL ); + serv =svc_create (&nfs4_callback_program,NFS4_CALLBACK_BUFSIZE , &nfs_cb_sv_ops);if (!serv) {printk (KERN_ERR "nfs_callback_create_svc: create service failed\n" );return ERR_PTR (-ENOMEM );
fs/nfsd/nfssvc.c +5 -1
@@ -390,6 +390,10 @@ static int nfsd_get_default_max_blksize(void)return ret; } +static struct svc_serv_ops nfsd_sv_ops = { + .svo_shutdown = nfsd_last_thread, +}; +int nfsd_create_serv (struct net *net) {int error;
@@ -404,7 +408,7 @@ int nfsd_create_serv(struct net *net) nfsd_max_blksize =nfsd_get_default_max_blksize ();nfsd_reset_versions (); nn->nfsd_serv =svc_create_pooled (&nfsd_program, nfsd_max_blksize, - nfsd_last_thread, nfsd,THIS_MODULE ); + &nfsd_sv_ops, nfsd,THIS_MODULE );if (nn->nfsd_serv ==NULL )return -ENOMEM ;
include/linux/sunrpc/svc.h +10 -10
@@ -54,6 +54,13 @@ struct svc_pool {unsigned long sp_flags; }____cacheline_aligned_in_smp ; +struct svc_serv ; + +struct svc_serv_ops { +/* Callback to use when last thread exits. */ +void (*svo_shutdown)(struct svc_serv *serv,struct net *net); +}; +/* * RPC service. *
@@ -85,13 +92,7 @@ struct svc_serv {unsigned int sv_nrpools;/* number of thread pools */ struct svc_pool * sv_pools;/* array of thread pools */ - -void (*sv_shutdown)(struct svc_serv *serv, -struct net *net); -/* Callback to use when last thread -* exits. -*/ - +struct svc_serv_ops * sv_ops;/* server operations */ struct module * sv_module;/* optional module to count when * adding threads */ svc_thread_fn sv_function;/* main function for threads */
@@ -429,13 +430,12 @@ int svc_rpcb_setup(struct svc_serv *serv, struct net *net);void svc_rpcb_cleanup (struct svc_serv *serv,struct net *net);int svc_bind (struct svc_serv *serv,struct net *net);struct svc_serv *svc_create (struct svc_program *,unsigned int , -void (*shutdown)(struct svc_serv *,struct net *net)); +struct svc_serv_ops *);struct svc_rqst *svc_prepare_thread (struct svc_serv *serv,struct svc_pool *pool,int node);void svc_exit_thread (struct svc_rqst *);struct svc_serv *svc_create_pooled (struct svc_program *,unsigned int , -void (*shutdown)(struct svc_serv *,struct net *net), -svc_thread_fn ,struct module *); +struct svc_serv_ops *,svc_thread_fn ,struct module *);int svc_set_num_threads (struct svc_serv *,struct svc_pool *,int );int svc_pool_stats_open (struct svc_serv *serv,struct file *file);void svc_destroy (struct svc_serv *);
net/sunrpc/svc.c +9 -9
@@ -423,7 +423,7 @@ EXPORT_SYMBOL_GPL(svc_bind);*/ static struct svc_serv *__svc_create (struct svc_program *prog,unsigned int bufsize,int npools, -void (*shutdown)(struct svc_serv *serv,struct net *net)) +struct svc_serv_ops *ops) {struct svc_serv *serv;unsigned int vers;
@@ -440,7 +440,7 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools, bufsize =RPCSVC_MAXPAYLOAD ; serv->sv_max_payload = bufsize? bufsize :4096 ; serv->sv_max_mesg =roundup (serv->sv_max_payload +PAGE_SIZE ,PAGE_SIZE ); - serv->sv_shutdown = shutdown; + serv->sv_ops = ops; xdrsize =0 ;while (prog) { prog->pg_lovers = prog->pg_nvers-1 ;
@@ -486,21 +486,21 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,struct svc_serv *svc_create (struct svc_program *prog,unsigned int bufsize, -void (*shutdown)(struct svc_serv *serv,struct net *net)) +struct svc_serv_ops *ops) { -return __svc_create (prog, bufsize,/*npools*/ 1 , shutdown); +return __svc_create (prog, bufsize,/*npools*/ 1 , ops); }EXPORT_SYMBOL_GPL (svc_create);struct svc_serv *svc_create_pooled (struct svc_program *prog,unsigned int bufsize, -void (*shutdown)(struct svc_serv *serv,struct net *net), -svc_thread_fn func,struct module *mod) +struct svc_serv_ops *ops,svc_thread_fn func, +struct module *mod) {struct svc_serv *serv;unsigned int npools =svc_pool_map_get (); - serv =__svc_create (prog, bufsize, npools, shutdown); + serv =__svc_create (prog, bufsize, npools, ops);if (!serv) goto out_err;
@@ -517,8 +517,8 @@ void svc_shutdown_net(struct svc_serv *serv, struct net *net) {svc_close_net (serv, net); -if (serv->sv_shutdown) - serv->sv_shutdown (serv, net); +if (serv->sv_ops->svo_shutdown) + serv->sv_ops->svo_shutdown (serv, net); }EXPORT_SYMBOL_GPL (svc_shutdown_net);
Signature · 1 line
2.1.0
RFC PATCH 2/14 sunrpc: move sv_function into sv_ops
In reply to Jeff LaytonSince we now have a container for holding svc_serv operations, move the sv_function into it as well.
--- fs/nfsd/nfssvc.c | 3 ++- include/linux/sunrpc/svc.h | 11 +++-------- net/sunrpc/svc.c | 8 +++----- 3 files changed, 8 insertions(+), 14 deletions(-)
fs/nfsd/nfssvc.c +2 -1
@@ -392,6 +392,7 @@ static int nfsd_get_default_max_blksize(void)static struct svc_serv_ops nfsd_sv_ops = { .svo_shutdown = nfsd_last_thread, + .svo_function = nfsd, };int nfsd_create_serv (struct net *net)
@@ -408,7 +409,7 @@ int nfsd_create_serv(struct net *net) nfsd_max_blksize =nfsd_get_default_max_blksize ();nfsd_reset_versions (); nn->nfsd_serv =svc_create_pooled (&nfsd_program, nfsd_max_blksize, - &nfsd_sv_ops, nfsd,THIS_MODULE ); + &nfsd_sv_ops,THIS_MODULE );if (nn->nfsd_serv ==NULL )return -ENOMEM ;
include/linux/sunrpc/svc.h +3 -8
@@ -19,11 +19,6 @@#include <linux/wait.h> #include <linux/mm.h> -/* -* This is the RPC server thread function prototype -*/ -typedef int (*svc_thread_fn )(void *); -/* statistics for svc_pool structures */ struct svc_pool_stats {atomic_long_t packets;
@@ -58,7 +53,8 @@ struct svc_serv;struct svc_serv_ops {/* Callback to use when last thread exits. */ -void (*svo_shutdown)(struct svc_serv *serv,struct net *net); +void (*svo_shutdown)(struct svc_serv *,struct net *); +int (*svo_function)(void *); };/*
@@ -95,7 +91,6 @@ struct svc_serv {struct svc_serv_ops * sv_ops;/* server operations */ struct module * sv_module;/* optional module to count when * adding threads */ -svc_thread_fn sv_function;/* main function for threads */ #if defined(CONFIG_SUNRPC_BACKCHANNEL )struct list_head sv_cb_list;/* queue for callback requests * that arrive over the same
@@ -435,7 +430,7 @@ struct svc_rqst *svc_prepare_thread(struct svc_serv *serv, struct svc_pool *pool, int node);void svc_exit_thread (struct svc_rqst *);struct svc_serv *svc_create_pooled (struct svc_program *,unsigned int , -struct svc_serv_ops *,svc_thread_fn ,struct module *); +struct svc_serv_ops *,struct module *);int svc_set_num_threads (struct svc_serv *,struct svc_pool *,int );int svc_pool_stats_open (struct svc_serv *serv,struct file *file);void svc_destroy (struct svc_serv *);
net/sunrpc/svc.c +3 -5
@@ -34,7 +34,7 @@static void svc_unregister (const struct svc_serv *serv,struct net *net); -#define svc_serv_is_pooled (serv) ((serv)->sv_function) +#define svc_serv_is_pooled (serv) ((serv)->sv_ops->svo_function)/* * Mode for mapping cpus to pools.
@@ -494,8 +494,7 @@ EXPORT_SYMBOL_GPL(svc_create);struct svc_serv *svc_create_pooled (struct svc_program *prog,unsigned int bufsize, -struct svc_serv_ops *ops,svc_thread_fn func, -struct module *mod) +struct svc_serv_ops *ops,struct module *mod) {struct svc_serv *serv;unsigned int npools =svc_pool_map_get ();
@@ -504,7 +503,6 @@ svc_create_pooled(struct svc_program *prog, unsigned int bufsize,if (!serv) goto out_err; - serv->sv_function = func; serv->sv_module = mod;return serv; out_err:
@@ -740,7 +738,7 @@ svc_set_num_threads(struct svc_serv *serv, struct svc_pool *pool, int nrservs) }__module_get (serv->sv_module); - task =kthread_create_on_node (serv->sv_function, rqstp, + task =kthread_create_on_node (serv->sv_ops->svo_function, rqstp, node,"%s" , serv->sv_name);if (IS_ERR (task)) { error =PTR_ERR (task);
Signature · 1 line
2.1.0
RFC PATCH 3/14 sunrpc: move sv_module parm into sv_ops
In reply to Jeff Layton...not technically an operation, but it's more convenient and cleaner to pass the module pointer in this struct.
--- fs/nfsd/nfssvc.c | 3 ++- include/linux/sunrpc/svc.h | 9 ++++++--- net/sunrpc/svc.c | 8 +++----- 3 files changed, 11 insertions(+), 9 deletions(-)
fs/nfsd/nfssvc.c +2 -1
@@ -393,6 +393,7 @@ static int nfsd_get_default_max_blksize(void)static struct svc_serv_ops nfsd_sv_ops = { .svo_shutdown = nfsd_last_thread, .svo_function = nfsd, + .svo_module =THIS_MODULE , };int nfsd_create_serv (struct net *net)
@@ -409,7 +410,7 @@ int nfsd_create_serv(struct net *net) nfsd_max_blksize =nfsd_get_default_max_blksize ();nfsd_reset_versions (); nn->nfsd_serv =svc_create_pooled (&nfsd_program, nfsd_max_blksize, - &nfsd_sv_ops,THIS_MODULE ); + &nfsd_sv_ops);if (nn->nfsd_serv ==NULL )return -ENOMEM ;
include/linux/sunrpc/svc.h +6 -3
@@ -54,7 +54,12 @@ struct svc_serv;struct svc_serv_ops {/* Callback to use when last thread exits. */ void (*svo_shutdown)(struct svc_serv *,struct net *); + +/* function for service threads to run */ int (*svo_function)(void *); + +/* optional module to count when adding threads (pooled svcs only) */ +struct module *svo_module; };/*
@@ -89,8 +94,6 @@ struct svc_serv {unsigned int sv_nrpools;/* number of thread pools */ struct svc_pool * sv_pools;/* array of thread pools */ struct svc_serv_ops * sv_ops;/* server operations */ -struct module * sv_module;/* optional module to count when -* adding threads */ #if defined(CONFIG_SUNRPC_BACKCHANNEL )struct list_head sv_cb_list;/* queue for callback requests * that arrive over the same
@@ -430,7 +433,7 @@ struct svc_rqst *svc_prepare_thread(struct svc_serv *serv, struct svc_pool *pool, int node);void svc_exit_thread (struct svc_rqst *);struct svc_serv *svc_create_pooled (struct svc_program *,unsigned int , -struct svc_serv_ops *,struct module *); +struct svc_serv_ops *);int svc_set_num_threads (struct svc_serv *,struct svc_pool *,int );int svc_pool_stats_open (struct svc_serv *serv,struct file *file);void svc_destroy (struct svc_serv *);
net/sunrpc/svc.c +3 -5
@@ -494,7 +494,7 @@ EXPORT_SYMBOL_GPL(svc_create);struct svc_serv *svc_create_pooled (struct svc_program *prog,unsigned int bufsize, -struct svc_serv_ops *ops,struct module *mod) +struct svc_serv_ops *ops) {struct svc_serv *serv;unsigned int npools =svc_pool_map_get ();
@@ -502,8 +502,6 @@ svc_create_pooled(struct svc_program *prog, unsigned int bufsize, serv =__svc_create (prog, bufsize, npools, ops);if (!serv) goto out_err; - - serv->sv_module = mod;return serv; out_err:svc_pool_map_put ();
@@ -737,12 +735,12 @@ svc_set_num_threads(struct svc_serv *serv, struct svc_pool *pool, int nrservs)break ; } -__module_get (serv->sv_module); +__module_get (serv->sv_ops->svo_module); task =kthread_create_on_node (serv->sv_ops->svo_function, rqstp, node,"%s" , serv->sv_name);if (IS_ERR (task)) { error =PTR_ERR (task); -module_put (serv->sv_module); +module_put (serv->sv_ops->svo_module);svc_exit_thread (rqstp);break ; }
Signature · 1 line
2.1.0
RFC PATCH 4/14 sunrpc: turn enqueueing a svc_xprt into a svc_serv operation
In reply to Jeff LaytonFor now, all services use svc_xprt_do_enqueue, but once we add workqueue-based service support, we'll need to do something different.
--- fs/lockd/svc.c | 3 ++- fs/nfs/callback.c | 1 + fs/nfsd/nfssvc.c | 11 ++++++----- include/linux/sunrpc/svc.h | 3 +++ include/linux/sunrpc/svc_xprt.h | 1 + net/sunrpc/svc_xprt.c | 10 +++++----- 6 files changed, 18 insertions(+), 11 deletions(-)
fs/lockd/svc.c +2 -1
@@ -327,7 +327,8 @@ out_rqst: }static struct svc_serv_ops lockd_sv_ops = { - .svo_shutdown = svc_rpcb_cleanup, + .svo_shutdown = svc_rpcb_cleanup, + .svo_enqueue_xprt = svc_xprt_do_enqueue, };static struct svc_serv *lockd_create_svc (void )
fs/nfs/callback.c +1 -0
@@ -307,6 +307,7 @@ err_bind: }static struct svc_serv_ops nfs_cb_sv_ops = { + .svo_enqueue_xprt = svc_xprt_do_enqueue, };static struct svc_serv *nfs_callback_create_svc (int minorversion)
fs/nfsd/nfssvc.c +6 -5
@@ -390,10 +390,11 @@ static int nfsd_get_default_max_blksize(void)return ret; } -static struct svc_serv_ops nfsd_sv_ops = { - .svo_shutdown = nfsd_last_thread, - .svo_function = nfsd, - .svo_module =THIS_MODULE , +static struct svc_serv_ops nfsd_thread_sv_ops = { + .svo_shutdown = nfsd_last_thread, + .svo_function = nfsd, + .svo_enqueue_xprt = svc_xprt_do_enqueue, + .svo_module =THIS_MODULE , };int nfsd_create_serv (struct net *net)
@@ -410,7 +411,7 @@ int nfsd_create_serv(struct net *net) nfsd_max_blksize =nfsd_get_default_max_blksize ();nfsd_reset_versions (); nn->nfsd_serv =svc_create_pooled (&nfsd_program, nfsd_max_blksize, - &nfsd_sv_ops); + &nfsd_thread_sv_ops);if (nn->nfsd_serv ==NULL )return -ENOMEM ;
include/linux/sunrpc/svc.h +3 -0
@@ -58,6 +58,9 @@ struct svc_serv_ops {/* function for service threads to run */ int (*svo_function)(void *); +/* queue up a transport for servicing */ +void (*svo_enqueue_xprt)(struct svc_xprt *); +/* optional module to count when adding threads (pooled svcs only) */ struct module *svo_module; };
include/linux/sunrpc/svc_xprt.h +1 -0
@@ -116,6 +116,7 @@ void svc_xprt_init(struct net *, struct svc_xprt_class *, struct svc_xprt *, struct svc_serv *);int svc_create_xprt (struct svc_serv *,const char *,struct net *,const int ,const unsigned short ,int ); +void svc_xprt_do_enqueue (struct svc_xprt *xprt);void svc_xprt_enqueue (struct svc_xprt *xprt);void svc_xprt_put (struct svc_xprt *xprt);void svc_xprt_copy_addrs (struct svc_rqst *rqstp,struct svc_xprt *xprt);
net/sunrpc/svc_xprt.c +5 -5
@@ -24,7 +24,6 @@ static int svc_deferred_recv(struct svc_rqst *rqstp);static struct cache_deferred_req *svc_defer (struct cache_req *req);static void svc_age_temp_xprts (unsigned long closure);static void svc_delete_xprt (struct svc_xprt *xprt); -static void svc_xprt_do_enqueue (struct svc_xprt *xprt);/* apparently the "standard" is that clients close * idle connections after 5 minutes, servers after
@@ -226,12 +225,12 @@ static void svc_xprt_received(struct svc_xprt *xprt) }/* As soon as we clear busy, the xprt could be closed and -* 'put', so we need a reference to call svc_xprt_do_enqueue with: +* 'put', so we need a reference to call svc_enqueue_xprt with: */ svc_xprt_get (xprt);smp_mb__before_atomic ();clear_bit (XPT_BUSY , &xprt->xpt_flags); -svc_xprt_do_enqueue (xprt); + xprt->xpt_server->sv_ops->svo_enqueue_xprt (xprt);svc_xprt_put (xprt); }
@@ -321,7 +320,7 @@ static bool svc_xprt_has_something_to_do(struct svc_xprt *xprt)return false; } -static void svc_xprt_do_enqueue (struct svc_xprt *xprt) +void svc_xprt_do_enqueue (struct svc_xprt *xprt) {struct svc_pool *pool;struct svc_rqst *rqstp =NULL ;
@@ -403,6 +402,7 @@ redo_search: out:trace_svc_xprt_do_enqueue (xprt, rqstp); } +EXPORT_SYMBOL_GPL (svc_xprt_do_enqueue);/* * Queue up a transport with data pending. If there are idle nfsd
@@ -413,7 +413,7 @@ void svc_xprt_enqueue(struct svc_xprt *xprt) {if (test_bit (XPT_BUSY , &xprt->xpt_flags))return ; -svc_xprt_do_enqueue (xprt); + xprt->xpt_server->sv_ops->svo_enqueue_xprt (xprt); }EXPORT_SYMBOL_GPL (svc_xprt_enqueue);
Signature · 1 line
2.1.0
RFC PATCH 5/14 sunrpc: abstract out svc_set_num_threads to sv_ops
In reply to Jeff LaytonAdd an operation that will do setup of the service. In the case of a classic thread-based service that means starting up threads. In the case of a workqueue-based service, the setup will do something different.
--- fs/nfsd/nfssvc.c | 8 +++++--- include/linux/sunrpc/svc.h | 3 +++ 2 files changed, 8 insertions(+), 3 deletions(-)
fs/nfsd/nfssvc.c +5 -3
@@ -394,6 +394,7 @@ static struct svc_serv_ops nfsd_thread_sv_ops = { .svo_shutdown = nfsd_last_thread, .svo_function = nfsd, .svo_enqueue_xprt = svc_xprt_do_enqueue, + .svo_setup = svc_set_num_threads, .svo_module = THIS_MODULE, };
@@ -506,8 +507,8 @@ int nfsd_set_nrthreads(int n, int *nthreads, struct net *net)/* apply the new numbers */ svc_get (nn->nfsd_serv);for (i =0 ; i < n; i++) { - err =svc_set_num_threads (nn->nfsd_serv, &nn->nfsd_serv->sv_pools[i], - nthreads[i]); + err = nn->nfsd_serv->sv_ops->svo_setup (nn->nfsd_serv, + &nn->nfsd_serv->sv_pools[i], nthreads[i]);if (err)break ; }
@@ -546,7 +547,8 @@ nfsd_svc(int nrservs, struct net *net) error =nfsd_startup_net (nrservs, net);if (error) goto out_destroy; - error =svc_set_num_threads (nn->nfsd_serv,NULL , nrservs); + error = nn->nfsd_serv->sv_ops->svo_setup (nn->nfsd_serv, +NULL , nrservs);if (error) goto out_shutdown;/* We are holding a reference to nn->nfsd_serv which
include/linux/sunrpc/svc.h +3 -0
@@ -61,6 +61,9 @@ struct svc_serv_ops {/* queue up a transport for servicing */ void (*svo_enqueue_xprt)(struct svc_xprt *); +/* set up thread (or whatever) execution context */ +int (*svo_setup)(struct svc_serv *,struct svc_pool *,int ); +/* optional module to count when adding threads (pooled svcs only) */ struct module *svo_module; };
Signature · 1 line
2.1.0
RFC PATCH 6/14 sunrpc: move pool_mode definitions into svc.h
In reply to Jeff LaytonIn later patches, we're going to need to allow code external to svc.c to figure out what pool_mode is in use. Move these definitions into svc.h to prepare for that.
Also, make the svc_pool_map object available and exported so that other modules can peek in there to get insight into what pool mode is in use. Likewise, export svc_pool_map_get/put function to make it safe to do so.
--- include/linux/sunrpc/svc.h | 25 +++++++++++++++++++++++++ net/sunrpc/svc.c | 31 +++++++------------------------ 2 files changed, 32 insertions(+), 24 deletions(-)
include/linux/sunrpc/svc.h +25 -0
@@ -428,6 +428,29 @@ struct svc_procedure { };/* +* Mode for mapping cpus to pools. +*/ +enum { +SVC_POOL_AUTO =-1 ,/* choose one of the others */ +SVC_POOL_GLOBAL ,/* no mapping, just a single global pool +* (legacy & UP mode) */ +SVC_POOL_PERCPU ,/* one pool per cpu */ +SVC_POOL_PERNODE /* one pool per numa node */ +}; + +struct svc_pool_map { +int count;/* How many svc_servs use us */ +int mode;/* Note: int not enum to avoid +* warnings about "enumeration value +* not handled in switch" */ +unsigned int npools; +unsigned int *pool_to;/* maps pool id to cpu or node */ +unsigned int *to_pool;/* maps cpu or node to pool id */ +}; + +extern struct svc_pool_map svc_pool_map; + +/* * Function prototypes. */ int svc_rpcb_setup (struct svc_serv *serv,struct net *net);
@@ -438,6 +461,8 @@ struct svc_serv *svc_create(struct svc_program *, unsigned int,struct svc_rqst *svc_prepare_thread (struct svc_serv *serv,struct svc_pool *pool,int node);void svc_exit_thread (struct svc_rqst *); +unsigned int svc_pool_map_get (void ); +void svc_pool_map_put (void );struct svc_serv *svc_create_pooled (struct svc_program *,unsigned int ,struct svc_serv_ops *);int svc_set_num_threads (struct svc_serv *,struct svc_pool *,int );
net/sunrpc/svc.c +7 -24
@@ -36,34 +36,17 @@ static void svc_unregister(const struct svc_serv *serv, struct net *net);#define svc_serv_is_pooled (serv) ((serv)->sv_ops->svo_function) -/* -* Mode for mapping cpus to pools. -*/ -enum { -SVC_POOL_AUTO =-1 ,/* choose one of the others */ -SVC_POOL_GLOBAL ,/* no mapping, just a single global pool -* (legacy & UP mode) */ -SVC_POOL_PERCPU ,/* one pool per cpu */ -SVC_POOL_PERNODE /* one pool per numa node */ -};#define SVC_POOL_DEFAULT SVC_POOL_GLOBAL/* * Structure for mapping cpus to pools and vice versa. * Setup once during sunrpc initialisation. */ -static struct svc_pool_map { -int count;/* How many svc_servs use us */ -int mode;/* Note: int not enum to avoid -* warnings about "enumeration value -* not handled in switch" */ -unsigned int npools; -unsigned int *pool_to;/* maps pool id to cpu or node */ -unsigned int *to_pool;/* maps cpu or node to pool id */ -} svc_pool_map = { - .count =0 , +struct svc_pool_map svc_pool_map = { .mode =SVC_POOL_DEFAULT }; +EXPORT_SYMBOL_GPL (svc_pool_map); +static DEFINE_MUTEX (svc_pool_map_mutex);/* protects svc_pool_map.count only */ static int
@@ -236,7 +219,7 @@ svc_pool_map_init_pernode(struct svc_pool_map *m)* vice versa). Initialise the map if we're the first user. * Returns the number of pools. */ -static unsigned int +unsigned int svc_pool_map_get (void ) {struct svc_pool_map *m = &svc_pool_map;
@@ -271,7 +254,7 @@ svc_pool_map_get(void)mutex_unlock (&svc_pool_map_mutex);return m->npools; } - +EXPORT_SYMBOL_GPL (svc_pool_map_get);/* * Drop a reference to the global map of cpus to pools.
@@ -280,7 +263,7 @@ svc_pool_map_get(void)* mode using the pool_mode module option without * rebooting or re-loading sunrpc.ko. */ -static void +void svc_pool_map_put (void ) {struct svc_pool_map *m = &svc_pool_map;
@@ -297,7 +280,7 @@ svc_pool_map_put(void)mutex_unlock (&svc_pool_map_mutex); } - +EXPORT_SYMBOL_GPL (svc_pool_map_put);static int svc_pool_map_get_node (unsigned int pidx) {
Signature · 1 line
2.1.0
RFC PATCH 7/14 sunrpc: factor svc_rqst allocation and freeing from sv_nrthreads refcounting
In reply to Jeff LaytonIn later patches, we'll want to be able to allocate and free svc_rqst structures without monkeying with the serv->sv_nrthreads refcount.
Factor those pieces out of their respective functions.
--- include/linux/sunrpc/svc.h | 3 +++ net/sunrpc/svc.c | 58 ++++++++++++++++++++++++++++++---------------- 2 files changed, 41 insertions(+), 20 deletions(-)
include/linux/sunrpc/svc.h +3 -0
@@ -458,8 +458,11 @@ void svc_rpcb_cleanup(struct svc_serv *serv, struct net *net);int svc_bind (struct svc_serv *serv,struct net *net);struct svc_serv *svc_create (struct svc_program *,unsigned int ,struct svc_serv_ops *); +struct svc_rqst *svc_rqst_alloc (struct svc_serv *serv, +struct svc_pool *pool,int node);struct svc_rqst *svc_prepare_thread (struct svc_serv *serv,struct svc_pool *pool,int node); +void svc_rqst_free (struct svc_rqst *);void svc_exit_thread (struct svc_rqst *);unsigned int svc_pool_map_get (void );void svc_pool_map_put (void );
net/sunrpc/svc.c +38 -20
104 lines not on this page · raw message
Signature · 1 line
2.1.0
RFC PATCH 8/14 sunrpc: set up workqueue function in svc_xprt
In reply to Jeff LaytonFor workqueue-based services we'll want to queue a workqueue job whenever a xprt needs to be serviced. Add a work_struct to svc_xprt and initialize it from a new work_func_t field in the svc_serv_ops.
--- include/linux/sunrpc/svc.h | 3 +++ include/linux/sunrpc/svc_xprt.h | 1 + net/sunrpc/svc.c | 2 +- net/sunrpc/svc_xprt.c | 2 ++ 4 files changed, 7 insertions(+), 1 deletion(-)
include/linux/sunrpc/svc.h +3 -0
@@ -58,6 +58,9 @@ struct svc_serv_ops {/* function for service threads to run */ int (*svo_function)(void *); +/* xprt work function */ +work_func_t svo_xprt_work; +/* queue up a transport for servicing */ void (*svo_enqueue_xprt)(struct svc_xprt *);
include/linux/sunrpc/svc_xprt.h +1 -0
@@ -53,6 +53,7 @@ struct svc_xprt {struct kref xpt_ref;struct list_head xpt_list;struct list_head xpt_ready; +struct work_struct xpt_work;unsigned long xpt_flags;#define XPT_BUSY 0/* enqueued/receiving */ #define XPT_CONN 1/* conn pending */
net/sunrpc/svc.c +1 -1
@@ -34,7 +34,7 @@static void svc_unregister (const struct svc_serv *serv,struct net *net); -#define svc_serv_is_pooled (serv) ((serv)->sv_ops->svo_function) +#define svc_serv_is_pooled (serv) ((serv)->sv_ops->svo_function || (serv)->sv_ops->svo_xprt_work)#define SVC_POOL_DEFAULT SVC_POOL_GLOBAL
net/sunrpc/svc_xprt.c +2 -0
@@ -157,6 +157,8 @@ void svc_xprt_init(struct net *net, struct svc_xprt_class *xcl, xprt->xpt_ops = xcl->xcl_ops;kref_init (&xprt->xpt_ref); xprt->xpt_server = serv; +if (serv->sv_ops->svo_xprt_work) +INIT_WORK (&xprt->xpt_work, serv->sv_ops->svo_xprt_work);INIT_LIST_HEAD (&xprt->xpt_list);INIT_LIST_HEAD (&xprt->xpt_ready);INIT_LIST_HEAD (&xprt->xpt_deferred);
Signature · 1 line
2.1.0
RFC PATCH 9/14 sunrpc: add basic support for workqueue-based services
In reply to Jeff LaytonAdd a new "workqueue" pool mode setting. When that is configured, we'll set up a svc_pool for each NUMA node, but don't bother with the pool <=> cpu mapping arrays.
We use an unbound workqueue, which should naturally make each xprt be queued to a CPU within the current NUMA node.
--- include/linux/sunrpc/svc.h | 10 ++- include/linux/sunrpc/svc_xprt.h | 1 + include/linux/sunrpc/svcsock.h | 1 + net/sunrpc/Kconfig | 10 +++ net/sunrpc/Makefile | 1 + net/sunrpc/svc.c | 15 ++++ net/sunrpc/svc_wq.c | 148 ++++++++++++++++++++++++++++++++++++++++ net/sunrpc/svc_xprt.c | 49 ++++++++++++- 8 files changed, 233 insertions(+), 2 deletions(-) create mode 100644 net/sunrpc/svc_wq.c
include/linux/sunrpc/svc.h +9 -1
@@ -47,6 +47,7 @@ struct svc_pool {#define SP_TASK_PENDING (0)/* still work to do even if no * xprt is queued. */ unsigned long sp_flags; +struct work_struct sp_work;/* per-pool work struct */ }____cacheline_aligned_in_smp ;struct svc_serv ;
@@ -103,6 +104,7 @@ struct svc_serv {unsigned int sv_nrpools;/* number of thread pools */ struct svc_pool * sv_pools;/* array of thread pools */ struct svc_serv_ops * sv_ops;/* server operations */ +struct workqueue_struct *sv_wq;/* workqueue for wq-based services */ #if defined(CONFIG_SUNRPC_BACKCHANNEL )struct list_head sv_cb_list;/* queue for callback requests * that arrive over the same
@@ -438,7 +440,8 @@ enum {SVC_POOL_GLOBAL ,/* no mapping, just a single global pool * (legacy & UP mode) */ SVC_POOL_PERCPU ,/* one pool per cpu */ -SVC_POOL_PERNODE /* one pool per numa node */ +SVC_POOL_PERNODE ,/* one pool per numa node */ + SVC_POOL_WORKQUEUE, /* workqueue-based service */ }; struct svc_pool_map {
@@ -486,6 +489,11 @@ void svc_reserve(struct svc_rqst *rqstp, int space);struct svc_pool *svc_pool_for_cpu (struct svc_serv *serv,int cpu);char *svc_print_addr (struct svc_rqst *,char *,size_t ); +#if IS_ENABLED (CONFIG_SUNRPC_SVC_WORKQUEUE ) +int svc_wq_setup (struct svc_serv *,struct svc_pool *,int ); +void svc_wq_enqueue_xprt (struct svc_xprt *); +#endif +#define RPC_MAX_ADDRBUFLEN (63U)/*
include/linux/sunrpc/svc_xprt.h +1 -0
@@ -117,6 +117,7 @@ void svc_xprt_init(struct net *, struct svc_xprt_class *, struct svc_xprt *, struct svc_serv *);int svc_create_xprt (struct svc_serv *,const char *,struct net *,const int ,const unsigned short ,int ); +bool svc_xprt_has_something_to_do (struct svc_xprt *xprt);void svc_xprt_do_enqueue (struct svc_xprt *xprt);void svc_xprt_enqueue (struct svc_xprt *xprt);void svc_xprt_put (struct svc_xprt *xprt);
include/linux/sunrpc/svcsock.h +1 -0
@@ -53,6 +53,7 @@ static inline u32 svc_sock_final_rec(struct svc_sock *svsk)*/ void svc_close_net (struct svc_serv *,struct net *);int svc_recv (struct svc_rqst *,long ); +int svc_wq_recv (struct svc_rqst *);int svc_send (struct svc_rqst *);void svc_drop (struct svc_rqst *);void svc_sock_update_bufs (struct svc_serv *serv);
net/sunrpc/Kconfig +10 -0
@@ -71,3 +71,13 @@ config SUNRPC_XPRT_RDMA_SERVER choose M here: the module will be called svcrdma. If unsure, say N. + +config SUNRPC_SVC_WORKQUEUE + bool "Support for workqueue-based SUNRPC services" + depends on SUNRPC + default n + help + Traditional SUNRPC services have required a dedicated thread + to handle incoming requests. This option enables support for + queueing incoming reqests to a workqueue instead, eliminating + the need for a dedicated thread pool.
net/sunrpc/Makefile +1 -0
@@ -18,3 +18,4 @@ sunrpc-$(CONFIG_SUNRPC_DEBUG) += debugfs.o sunrpc-$(CONFIG_SUNRPC_BACKCHANNEL) += backchannel_rqst.o bc_svc.o sunrpc-$(CONFIG_PROC_FS) += stats.o sunrpc-$(CONFIG_SYSCTL) += sysctl.o +sunrpc-$(CONFIG_SUNRPC_SVC_WORKQUEUE) += svc_wq.o
net/sunrpc/svc.c +15 -0
@@ -71,6 +71,10 @@ param_set_pool_mode(const char *val, struct kernel_param *kp) *ip =SVC_POOL_PERCPU ; else if (!strncmp(val, "pernode", 7)) *ip =SVC_POOL_PERNODE ; +#if IS_ENABLED (CONFIG_SUNRPC_SVC_WORKQUEUE ) + else if (!strncmp(val, "workqueue", 9)) + *ip =SVC_POOL_WORKQUEUE ; +#endif else err = -EINVAL ;
@@ -94,6 +98,8 @@ param_get_pool_mode(char *buf, struct kernel_param *kp)return strlcpy (buf,"percpu" ,20 );case SVC_POOL_PERNODE :return strlcpy (buf,"pernode" ,20 ); +case SVC_POOL_WORKQUEUE : +return strlcpy (buf,"workqueue" ,20 );default :return sprintf (buf,"%d" , *ip); }
@@ -242,6 +248,10 @@ svc_pool_map_get(void)case SVC_POOL_PERNODE : npools =svc_pool_map_init_pernode (m);break ; +case SVC_POOL_WORKQUEUE : +/* workqueues get a pool per numa node, but don't need a map */ + npools = nr_node_ids; +break ; }if (npools <0 ) {
@@ -534,6 +544,11 @@ svc_destroy(struct svc_serv *serv)if (svc_serv_is_pooled (serv))svc_pool_map_put (); +if (serv->sv_wq) { +destroy_workqueue (serv->sv_wq); +module_put (serv->sv_ops->svo_module); + } +kfree (serv->sv_pools);kfree (serv); }
net/sunrpc/svc_wq.c new file +148 -0
148 lines not on this page · raw message
net/sunrpc/svc_xprt.c +48 -1
61 lines not on this page · raw message
Signature · 1 line
2.1.0
Re: [RFC PATCH 09/14] sunrpc: add basic support for workqueue-based services
Quoted 8 lines · Jeff Layton
net/sunrpc/Kconfig +0
-0@@ -71,3 +71,13 @@ config SUNRPC_XPRT_RDMA_SERVER choose M here: the module will be called svcrdma. If unsure, say N.
net/sunrpc/Kconfig config SUNRPC_XPRT_RDMA_SERVER · line 83
Quoted 10 lines
net/sunrpc/Kconfig +10
-0+ +config SUNRPC_SVC_WORKQUEUE + bool "Support for workqueue-based SUNRPC services" + depends on SUNRPC + default n + help + Traditional SUNRPC services have required a dedicated thread + to handle incoming requests. This option enables support for + queueing incoming reqests to a workqueue instead, eliminating + the need for a dedicated thread pool.
Minor point, but: If people don't want this, they can turn it off at runtime. It's annoying to test all the possible combination of build options, and this doesn't seem likely to let people e.g. build a significantly smaller kernel, so I wouldn't bother.
--b.
Re: [RFC PATCH 09/14] sunrpc: add basic support for workqueue-based services
Quoted 25 lines · On Mon, 8 Dec 2014 15:47:09 -0500 "J. Bruce Fields" <[email protected]>…
Quoted 8 lines · Jeff Layton
net/sunrpc/Kconfig +0
-0@@ -71,3 +71,13 @@ config SUNRPC_XPRT_RDMA_SERVER choose M here: the module will be called svcrdma. If unsure, say N.net/sunrpc/Kconfig config SUNRPC_XPRT_RDMA_SERVER · line 83
Quoted 10 lines
net/sunrpc/Kconfig +10
-0+ +config SUNRPC_SVC_WORKQUEUE + bool "Support for workqueue-based SUNRPC services" + depends on SUNRPC + default n + help + Traditional SUNRPC services have required a dedicated thread + to handle incoming requests. This option enables support for + queueing incoming reqests to a workqueue instead, eliminating + the need for a dedicated thread pool.Minor point, but: If people don't want this, they can turn it off at runtime. It's annoying to test all the possible combination of build options, and this doesn't seem likely to let people e.g. build a significantly smaller kernel, so I wouldn't bother.
Quoted 1 line
--b.
Ok, fair enough. I added the config option since this was experimental, but if we keep it runtime-switchable then I think you're probably right and there's no need.
Signature · 1 line
Jeff Layton <[email protected]>
RFC PATCH 10/14 nfsd: keep a reference to the fs_struct in svc_rqst
In reply to Jeff LaytonWhen we convert this code to use a workqueue, we won't want to allocate a new fs_struct to handle each RPC. Doing so might also be problematic since we'd be swapping out the ->fs value on a "public" workqueue kthread.
Change the code to allocate an fs struct when when allocating a svc_rqst and then switch to using that in the "nfsd" function.
--- fs/fs_struct.c | 59 +++++++++++++++++++++++++++++++++++++++------- fs/nfsd/nfssvc.c | 17 ++++++------- include/linux/fs_struct.h | 3 +++ include/linux/sunrpc/svc.h | 1 + net/sunrpc/svc.c | 8 +++++++ 5 files changed, 69 insertions(+), 19 deletions(-)
fs/fs_struct.c +50 -9
@@ -127,26 +127,67 @@ struct fs_struct *copy_fs_struct(struct fs_struct *old) }return fs; } +EXPORT_SYMBOL_GPL (copy_fs_struct); -int unshare_fs_struct (void ) +/* Replace current fs struct with one given. Return a pointer to old one. */ +static struct fs_struct * +swap_fs_struct (struct fs_struct *new_fs) { -struct fs_struct *fs = current->fs; -struct fs_struct *new_fs =copy_fs_struct (fs); -int kill; - -if (!new_fs) -return -ENOMEM ; +struct fs_struct *old_fs;task_lock (current); + old_fs = current->fs; + current->fs = new_fs; +task_unlock (current); + +return old_fs; +} + +/* Put a reference to a fs_struct. */ +void put_fs_struct (struct fs_struct *fs) +{ +bool kill; +spin_lock (&fs->lock); kill = !--fs->users; - current->fs = new_fs;spin_unlock (&fs->lock); -task_unlock (current);if (kill)free_fs_struct (fs); +} +EXPORT_SYMBOL_GPL (put_fs_struct); + +/* Take an extra reference to a fs_struct. Caller must already hold one! */ +struct fs_struct * +get_fs_struct (struct fs_struct *fs) +{ +spin_lock (&fs->lock); + ++fs->users; +spin_unlock (&fs->lock); +return fs; +} +EXPORT_SYMBOL_GPL (get_fs_struct); + +/* +* Swap in a new fs_struct and drop the reference on the old one. +* Caller must have already taken the reference to the new one. +*/ +void replace_fs_struct (struct fs_struct *new_fs) +{ +struct fs_struct *old_fs =swap_fs_struct (new_fs); + +put_fs_struct (old_fs); +} +EXPORT_SYMBOL_GPL (replace_fs_struct); + +int unshare_fs_struct (void ) +{ +struct fs_struct *new_fs =copy_fs_struct (current->fs); + +if (!new_fs) +return -ENOMEM ; +replace_fs_struct (new_fs);return 0 ; }EXPORT_SYMBOL_GPL (unshare_fs_struct);
fs/nfsd/nfssvc.c +7 -10
@@ -582,15 +582,13 @@ nfsd(void *vrqstp)/* Lock module and set up kernel thread */ mutex_lock (&nfsd_mutex); -/* At this point, the thread shares current->fs -* with the init process. We need to create files with a -* umask of 0 instead of init's umask. */ -if (unshare_fs_struct () <0 ) { -printk ("Unable to start nfsd thread: out of memory\n" ); - goto out; - } - - current->fs->umask =0 ; +/* +* At this point, the thread shares current->fs with the init process. +* We need to create files with a umask of 0 instead of init's umask, +* so switch to the fs_struct associated with the rqstp. +*/ +get_fs_struct (rqstp->rq_fs); +replace_fs_struct (rqstp->rq_fs);/* * thread is spawned with all signals set to SIG_IGN, re-enable
@@ -632,7 +630,6 @@ nfsd(void *vrqstp)mutex_lock (&nfsd_mutex); nfsdstats.th_cnt --; -out: rqstp->rq_server =NULL ;/* Release the thread */
include/linux/fs_struct.h +3 -0
@@ -21,7 +21,10 @@ extern void set_fs_root(struct fs_struct *, const struct path *);extern void set_fs_pwd (struct fs_struct *,const struct path *);extern struct fs_struct *copy_fs_struct (struct fs_struct *);extern void free_fs_struct (struct fs_struct *); +extern void replace_fs_struct (struct fs_struct *);extern int unshare_fs_struct (void ); +struct fs_struct *get_fs_struct (struct fs_struct *); +void put_fs_struct (struct fs_struct *);static inline void get_fs_root (struct fs_struct *fs,struct path *root) {
include/linux/sunrpc/svc.h +1 -0
@@ -295,6 +295,7 @@ struct svc_rqst {struct svc_cacherep * rq_cacherep;/* cache info */ struct task_struct *rq_task;/* service thread */ spinlock_t rq_lock;/* per-request lock */ +struct fs_struct *rq_fs; };#define SVC_NET (svc_rqst) (svc_rqst->rq_xprt->xpt_net)
net/sunrpc/svc.c +8 -0
@@ -20,6 +20,7 @@#include <linux/module.h> #include <linux/kthread.h> #include <linux/slab.h> +#include <linux/fs_struct.h> #include <linux/sunrpc/types.h> #include <linux/sunrpc/xdr.h>
@@ -622,6 +623,11 @@ svc_rqst_alloc(struct svc_serv *serv, struct svc_pool *pool, int node)if (!svc_init_buffer (rqstp, serv->sv_max_mesg, node)) goto out_enomem; + rqstp->rq_fs =copy_fs_struct (current->fs); +if (!rqstp->rq_fs) + goto out_enomem; + + rqstp->rq_fs->umask =0 ;return rqstp; out_enomem:svc_rqst_free (rqstp);
@@ -784,6 +790,8 @@ svc_rqst_free(struct svc_rqst *rqstp)kfree (rqstp->rq_resp);kfree (rqstp->rq_argp);kfree (rqstp->rq_auth_data); +if (rqstp->rq_fs) +put_fs_struct (rqstp->rq_fs);kfree_rcu (rqstp, rq_rcu_head); }EXPORT_SYMBOL_GPL (svc_rqst_free);
Signature · 1 line
2.1.0
RFC PATCH 11/14 nfsd: add support for workqueue based service processing
In reply to Jeff LaytonAllow nfsd to create a workqueue based nfsd service if the sunrpc pool_mode is set to "workqueue".
--- fs/fs_struct.c | 3 +- fs/nfsd/nfssvc.c | 77 +++++++++++++++++++++++++++++++++++++++++++++-- include/linux/fs_struct.h | 1 + 3 files changed, 78 insertions(+), 3 deletions(-)
fs/fs_struct.c +2 -1
@@ -130,7 +130,7 @@ struct fs_struct *copy_fs_struct(struct fs_struct *old)EXPORT_SYMBOL_GPL (copy_fs_struct);/* Replace current fs struct with one given. Return a pointer to old one. */ -static struct fs_struct * +struct fs_struct *swap_fs_struct (struct fs_struct *new_fs) {struct fs_struct *old_fs;
@@ -142,6 +142,7 @@ swap_fs_struct(struct fs_struct *new_fs)return old_fs; } +EXPORT_SYMBOL_GPL (swap_fs_struct);/* Put a reference to a fs_struct. */ void put_fs_struct (struct fs_struct *fs)
fs/nfsd/nfssvc.c +75 -2
105 lines not on this page · raw message
include/linux/fs_struct.h +1 -0
@@ -20,6 +20,7 @@ extern void exit_fs(struct task_struct *);extern void set_fs_root (struct fs_struct *,const struct path *);extern void set_fs_pwd (struct fs_struct *,const struct path *);extern struct fs_struct *copy_fs_struct (struct fs_struct *); +extern struct fs_struct *swap_fs_struct (struct fs_struct *);extern void free_fs_struct (struct fs_struct *);extern void replace_fs_struct (struct fs_struct *);extern int unshare_fs_struct (void );
Signature · 1 line
2.1.0
RFC PATCH 12/14 sunrpc: keep a cache of svc_rqsts for each NUMA node
In reply to Jeff LaytonAllocating an entire svc_rqst (including all of the pages, etc...) for each workqueue request is pretty expensive. Keep a cache of allocated svc_rqst structures for each NUMA node that we keep in svc_pool.
In order to keep the cache from growing without bound, we register a shrinker. Since the cache is already NUMA-aware, we can use a NUMA-aware shrinker as well.
--- fs/nfsd/nfssvc.c | 6 +- include/linux/sunrpc/svc.h | 17 ++++++ net/sunrpc/svc.c | 1 + net/sunrpc/svc_wq.c | 136 ++++++++++++++++++++++++++++++++++++++++++++- 4 files changed, 154 insertions(+), 6 deletions(-)
fs/nfsd/nfssvc.c +2 -4
@@ -672,7 +672,6 @@ nfsd(void *vrqstp)static void nfsd_work (struct work_struct *work) { -int node =numa_node_id (); struct svc_xprt *xprt = container_of(work, struct svc_xprt, xpt_work);struct net *net = xprt->xpt_net;struct nfsd_net *nn =net_generic (net, nfsd_net_id);
@@ -681,7 +680,7 @@ nfsd_work(struct work_struct *work)struct fs_struct *saved_fs;int err; - rqstp =svc_rqst_alloc (serv, &serv->sv_pools[node], node); + rqstp =find_or_alloc_svc_rqst (serv);if (!rqstp) {/* Alloc failure. Give up for now, and requeue the work */ queue_work (serv->sv_wq, &xprt->xpt_work);
@@ -703,8 +702,7 @@ nfsd_work(struct work_struct *work) saved_fs =swap_fs_struct (saved_fs);put_fs_struct (saved_fs); - -svc_rqst_free (rqstp); +put_svc_rqst (rqstp); }static struct svc_serv_ops nfsd_wq_sv_ops = {
include/linux/sunrpc/svc.h +17 -0
@@ -105,6 +105,7 @@ struct svc_serv {struct svc_pool * sv_pools;/* array of thread pools */ struct svc_serv_ops * sv_ops;/* server operations */ struct workqueue_struct *sv_wq;/* workqueue for wq-based services */ +struct shrinker sv_shrinker;/* for shrinking svc_rqst caches */ #if defined(CONFIG_SUNRPC_BACKCHANNEL )struct list_head sv_cb_list;/* queue for callback requests * that arrive over the same
@@ -274,6 +275,7 @@ struct svc_rqst {#define RQ_VICTIM (5)/* about to be shut down */ #define RQ_BUSY (6)/* request is busy */ unsigned long rq_flags;/* flags field */ +unsigned long rq_time;/* when rqstp was last put */ void * rq_argp;/* decoded arguments */ void * rq_resp;/* xdr'd results */
@@ -493,6 +495,21 @@ char * svc_print_addr(struct svc_rqst *, char *, size_t);#if IS_ENABLED (CONFIG_SUNRPC_SVC_WORKQUEUE )int svc_wq_setup (struct svc_serv *,struct svc_pool *,int );void svc_wq_enqueue_xprt (struct svc_xprt *); +struct svc_rqst *find_or_alloc_svc_rqst (struct svc_serv *serv); +void exit_svc_rqst_cache (struct svc_serv *serv); + +static inline void +put_svc_rqst (struct svc_rqst *rqstp) +{ + rqstp->rq_time = jiffies; +clear_bit (RQ_BUSY , &rqstp->rq_flags); +} +#else +static inline void +exit_svc_rqst_cache (struct svc_serv *serv) +{ +return ; +}#endif #define RPC_MAX_ADDRBUFLEN (63U)
net/sunrpc/svc.c +1 -0
@@ -547,6 +547,7 @@ svc_destroy(struct svc_serv *serv)if (serv->sv_wq) {destroy_workqueue (serv->sv_wq); +exit_svc_rqst_cache (serv);module_put (serv->sv_ops->svo_module); }
net/sunrpc/svc_wq.c +134 -2
166 lines not on this page · raw message
Signature · 1 line
2.1.0
RFC PATCH 13/14 sunrpc: add more tracepoints around svc_xprt handling
In reply to Jeff LaytonTurn the svc_xprt_dequeue tracepoint into an event class, and use it to add events for svc_xprt_received and svc_xprt_enqueue.
--- include/trace/events/sunrpc.h | 14 ++++++++++++-- net/sunrpc/svc_wq.c | 1 + net/sunrpc/svc_xprt.c | 5 ++++- 3 files changed, 17 insertions(+), 3 deletions(-)
include/trace/events/sunrpc.h +12 -2
@@ -496,7 +496,7 @@ DEFINE_EVENT(svc_rqst_status, svc_send, { (1UL << XPT_CACHE_AUTH), "XPT_CACHE_AUTH"}, \ { (1UL << XPT_LOCAL), "XPT_LOCAL"}) -TRACE_EVENT(svc_xprt_do_enqueue, +TRACE_EVENT(svc_xprt_enqueue, TP_PROTO(struct svc_xprt *xprt, struct svc_rqst *rqst), TP_ARGS(xprt, rqst),
@@ -517,7 +517,7 @@ TRACE_EVENT(svc_xprt_do_enqueue, show_svc_xprt_flags(__entry->xprt->xpt_flags)) ); -TRACE_EVENT(svc_xprt_dequeue, +DECLARE_EVENT_CLASS(svc_xprt_event, TP_PROTO(struct svc_xprt *xprt), TP_ARGS(xprt),
@@ -539,6 +539,15 @@ TRACE_EVENT(svc_xprt_dequeue, show_svc_xprt_flags(__entry->flags)) ); +DEFINE_EVENT (svc_xprt_event, svc_xprt_dequeue, + TP_PROTO(struct svc_xprt *xprt), TP_ARGS(xprt)); + +DEFINE_EVENT (svc_xprt_event, svc_xprt_received, + TP_PROTO(struct svc_xprt *xprt), TP_ARGS(xprt)); + +DEFINE_EVENT (svc_xprt_event, svc_xprt_active, + TP_PROTO(struct svc_xprt *xprt), TP_ARGS(xprt)); + TRACE_EVENT(svc_wake_up, TP_PROTO(int pid),
@@ -574,6 +583,7 @@ TRACE_EVENT(svc_handle_xprt, (struct sockaddr *)&__entry->xprt->xpt_remote, __entry->len, show_svc_xprt_flags(__entry->xprt->xpt_flags)) ); +#endif /* _TRACE_SUNRPC_H */ #include <trace/define_trace.h>
net/sunrpc/svc_wq.c +1 -0
@@ -275,6 +275,7 @@ svc_wq_enqueue_xprt(struct svc_xprt *xprt) } out:svc_xprt_get (xprt); +trace_svc_xprt_enqueue (xprt,NULL );queue_work (serv->sv_wq, &xprt->xpt_work); }EXPORT_SYMBOL_GPL (svc_wq_enqueue_xprt);
net/sunrpc/svc_xprt.c +4 -1
@@ -221,6 +221,7 @@ static struct svc_xprt *__svc_xpo_create(struct svc_xprt_class *xcl,*/ static void svc_xprt_received (struct svc_xprt *xprt) { +trace_svc_xprt_received (xprt);if (!test_bit (XPT_BUSY , &xprt->xpt_flags)) {WARN_ONCE (1 ,"xprt=0x%p already busy!" , xprt);return ;
@@ -402,7 +403,7 @@ redo_search: rqstp =NULL ;put_cpu (); out: -trace_svc_xprt_do_enqueue (xprt, rqstp); +trace_svc_xprt_enqueue (xprt, rqstp); }EXPORT_SYMBOL_GPL (svc_xprt_do_enqueue);
@@ -746,6 +747,8 @@ static int svc_handle_xprt(struct svc_rqst *rqstp, struct svc_xprt *xprt)struct svc_serv *serv = rqstp->rq_server;int len =0 ; +trace_svc_xprt_active (xprt); +if (test_bit (XPT_CLOSE , &xprt->xpt_flags)) {dprintk ("svc_recv: found XPT_CLOSE\n" );svc_delete_xprt (xprt);
Signature · 1 line
2.1.0
RFC PATCH 14/14 sunrpc: add tracepoints around svc_sock handling
In reply to Jeff Layton--- include/trace/events/sunrpc.h | 46 +++++++++++++++++++++++++++++++++++++++++++ net/sunrpc/svcsock.c | 6 ++++++ 2 files changed, 52 insertions(+)
include/trace/events/sunrpc.h +46 -0
@@ -9,6 +9,7 @@#include <linux/sunrpc/svc.h> #include <linux/sunrpc/xprtsock.h> #include <linux/sunrpc/svc_xprt.h> +#include <linux/sunrpc/svcsock.h> #include <net/tcp_states.h> #include <linux/net.h> #include <linux/tracepoint.h>
@@ -584,6 +585,51 @@ TRACE_EVENT(svc_handle_xprt, show_svc_xprt_flags(__entry->xprt->xpt_flags)) ); +DECLARE_EVENT_CLASS (svc_socket_event, + TP_PROTO(struct sock *sk), + +TP_ARGS (sk), + +TP_STRUCT__entry ( + __field(struct sock *, sk) + __field(struct svc_sock *, svsk) + __field_struct(struct sockaddr_storage, ss) + __field(unsigned long, xpt_flags) + ), + +TP_fast_assign ( + __entry->sk = sk; + __entry->svsk = sk->sk_user_data; + if (__entry->svsk) { + struct svc_xprt *xprt = &__entry->svsk->sk_xprt; + + memcpy(&__entry->ss, &xprt->xpt_remote, + sizeof(__entry->ss)); + __entry->xpt_flags = xprt->xpt_flags; + } else { +memset (&__entry->ss,0 ,sizeof (__entry->ss)); + __entry->xpt_flags =0 ; + } + ), + +TP_printk ("sk=0x%p peer=%pIScp sk_state=%s xpt_flags=%s" , __entry->sk, + (struct sockaddr *)&__entry->ss, +rpc_show_sock_state (__entry->sk->sk_state), +show_svc_xprt_flags (__entry->xpt_flags)) +); + +DEFINE_EVENT (svc_socket_event, svc_tcp_listen_data_ready, + TP_PROTO(struct sock *sk), TP_ARGS(sk)); + +DEFINE_EVENT (svc_socket_event, svc_tcp_state_change, + TP_PROTO(struct sock *sk), TP_ARGS(sk)); + +DEFINE_EVENT (svc_socket_event, svc_tcp_data_ready, + TP_PROTO(struct sock *sk), TP_ARGS(sk)); + +DEFINE_EVENT (svc_socket_event, svc_tcp_accept, + TP_PROTO(struct sock *sk), TP_ARGS(sk)); +#endif /* _TRACE_SUNRPC_H */ #include <trace/define_trace.h>
net/sunrpc/svcsock.c +6 -0
@@ -44,6 +44,7 @@#include <asm/uaccess.h> #include <asm/ioctls.h> #include <trace/events/skb.h> +#include <trace/events/sunrpc.h> #include <linux/sunrpc/types.h> #include <linux/sunrpc/clnt.h>
@@ -765,6 +766,7 @@ static void svc_tcp_listen_data_ready(struct sock *sk)struct svc_sock *svsk = (struct svc_sock *)sk->sk_user_data;wait_queue_head_t *wq; +trace_svc_tcp_listen_data_ready (sk);dprintk ("svc: socket %p TCP (listen) state change %d\n" , sk, sk->sk_state);
@@ -799,6 +801,7 @@ static void svc_tcp_state_change(struct sock *sk)struct svc_sock *svsk = (struct svc_sock *)sk->sk_user_data;wait_queue_head_t *wq =sk_sleep (sk); +trace_svc_tcp_state_change (sk);dprintk ("svc: socket %p TCP (connected) state change %d (svsk %p)\n" , sk, sk->sk_state, sk->sk_user_data);
@@ -817,6 +820,7 @@ static void svc_tcp_data_ready(struct sock *sk)struct svc_sock *svsk = (struct svc_sock *)sk->sk_user_data;wait_queue_head_t *wq =sk_sleep (sk); +trace_svc_tcp_data_ready (sk);dprintk ("svc: socket %p TCP data ready (svsk %p)\n" , sk, sk->sk_user_data);if (svsk) {
@@ -842,6 +846,8 @@ static struct svc_xprt *svc_tcp_accept(struct svc_xprt *xprt)int err, slen; RPC_IFDEBUG(char buf[RPC_MAX_ADDRBUFLEN]); +trace_svc_tcp_accept (svsk->sk_sk); +dprintk ("svc: tcp_accept %p sock %p\n" , svsk, sock);if (!sock)return NULL ;
Signature · 1 line
2.1.0
Hello, Jeff.
Quoted 3 lines · Jeff Layton
2) get some insight about the latency from those with a better understanding of the CMWQ code. Any thoughts as to why we might be seeing such high latency here? Any ideas of what we can do about it?
The latency is prolly from concurrency management. Work items which participate in concurrency management (the ones on per-cpu workqueues w/o WQ_CPU_INTENSIVE set) tend to get penalized on latency side quite a bit as the "run" durations for all such work items end up being serialized on the cpu. Setting WQ_CPU_INTENSIVE on the workqueue disables concurrency management and so does making the workqueue unbound. If strict cpu locality is likely to be beneficial and each work item isn't likely to consume huge amount of cpu cycles, WQ_CPU_INTENSIVE would fit better; otherwise, WQ_UNBOUND to let the scheduler do its thing.
Thanks.
Signature · 1 line
tejun
Quoted 18 lines · On Tue, 2 Dec 2014 14:18:14 -0500 Tejun Heo <[email protected]> wrote:
Hello, Jeff.
Quoted 3 lines · Jeff Layton
2) get some insight about the latency from those with a better understanding of the CMWQ code. Any thoughts as to why we might be seeing such high latency here? Any ideas of what we can do about it?
The latency is prolly from concurrency management. Work items which participate in concurrency management (the ones on per-cpu workqueues w/o
WQ_CPU_INTENSIVEset) tend to get penalized on latency side quite a bit as the "run" durations for all such work items end up being serialized on the cpu. SettingWQ_CPU_INTENSIVEon the workqueue disables concurrency management and so does making the workqueue unbound. If strict cpu locality is likely to be beneficial and each work item isn't likely to consume huge amount of cpu cycles,WQ_CPU_INTENSIVEwould fit better; otherwise,WQ_UNBOUNDto let the scheduler do its thing.
Quoted 2 lines
Thanks.
Thanks Tejun,
I'm already using WQ_UNBOUND workqueues. If that exempts this code from the concurrency management, then that's probably not the problem. The jobs here aren't terribly CPU intensive, but they can sleep for a long time while waiting on I/O, etc...
I don't think we necessarily need CPU locality (though that's nice to have of course), but NUMA affinity will likely be important. It looked like you had done some work a year or so ago to make unbound workqueues prefer to queue work on the same NUMA node which meshes nicely with what I think we want for this.
I'll keep looking at it -- let me know if you have any other thoughts on the latency...
Cheers!
Signature · 1 line
Jeff Layton <[email protected]>
Hello, Jeff.
Quoted 4 lines · Jeff Layton
I'm already using
WQ_UNBOUNDworkqueues. If that exempts this code from the concurrency management, then that's probably not the problem. The jobs here aren't terribly CPU intensive, but they can sleep for a long time while waiting on I/O, etc...
Yeap, noticed that. Replied in another message.
Quoted 5 lines
I don't think we necessarily need CPU locality (though that's nice to have of course), but NUMA affinity will likely be important. It looked like you had done some work a year or so ago to make unbound workqueues prefer to queue work on the same NUMA node which meshes nicely with what I think we want for this.
Yeap, unbound workqueues are NUMA-affine by default.
Thanks.
Signature · 1 line
tejun
On Tue, Dec 02, 2014 at 02:18:14PM -0500, Tejun Heo wrote:
...
Quoted 4 lines
unbound. If strict cpu locality is likely to be beneficial and each work item isn't likely to consume huge amount of cpu cycles,
WQ_CPU_INTENSIVEwould fit better; otherwise,WQ_UNBOUNDto let the scheduler do its thing.
Hmmm... but you're already using WQ_UNBOUND. Concurrency management doesn't matter for unbound workqueues. They really just behave as shared worker thread pools. Does turning on WQ_HIGHPRI change anything? Workqueue always prefers hot workers which can lead to the hot ones being penalized for consuming too much CPU time.
Thanks.
Signature · 1 line
tejun
Quoted 7 lines · On Tue, 2 Dec 2014 14:26:55 -0500 Tejun Heo <[email protected]> wrote:
On Tue, Dec 02, 2014 at 02:18:14PM -0500, Tejun Heo wrote:
...Quoted 4 lines
unbound. If strict cpu locality is likely to be beneficial and each work item isn't likely to consume huge amount of cpu cycles,
WQ_CPU_INTENSIVEwould fit better; otherwise,WQ_UNBOUNDto let the scheduler do its thing.
Quoted 8 lines
Hmmm... but you're already using
WQ_UNBOUND. Concurrency management doesn't matter for unbound workqueues. They really just behave as shared worker thread pools. Does turning onWQ_HIGHPRIchange anything? Workqueue always prefers hot workers which can lead to the hot ones being penalized for consuming too much CPU time.Thanks.
WQ_HIGHPRI doesn't seem to help. FWIW, here's the fio status line from that test:
threaded nfsd:
WRITE: io=12628KB, aggrb=210KB/s, minb=52KB/s, maxb=52KB/s, mint=60064msec, maxt=60107msec
workqueue nfsd:
WRITE: io=7464KB, aggrb=124KB/s, minb=30KB/s, maxb=31KB/s, mint=60066msec, maxt=60133msec
...and the workqueue numbers didn't change much from the case without WQ_HIGHPRI. I didn't gather significant queueing latency numbers for that run but I can get those together tomorrow.
My fio test config follows for anyone who's interested (it's tiobench-example.fio without the read tests):
-------------------------[snip]-------------------------- [global]
direct=1
size=128m
bsrange=4k-4k
timeout=60
numjobs=4 ; 4 simultaneous threads for each job
[f1]
rw=write -------------------------[snip]--------------------------
... the server in this case is an older quad core AMD Phenom CPU, with a 5400rpm SATA disk under it on a 3Gbps SATA interface. The client is mounting it using NFSv3 and I'm only running a single client here.
The only difference between the two tests above is that whether nfsd is running with the sunrpc.ko pool_mode set to "global" or "workqueue".
Signature · 1 line
Jeff Layton <[email protected]>
Quoted 14 lines · On Tue, 2 Dec 2014 13:24:09 -0500 Jeff Layton <[email protected]> wrote:
tl;dr: this code works and is much simpler than the dedicated thread
pool, but there are some latencies in the workqueue code that seem to keep it from being as fast as it could be.This patchset is a little skunkworks project that I've been poking at for the last few weeks. Currently nfsd uses a dedicated thread pool to handle RPCs, but that requires maintaining a rather large swath of "fiddly" code to handle the threads and transports.
This patchset represents an alternative approach, which makes nfsd use workqueues to do its bidding rather than a dedicated thread pool. When a transport needs to do work, we simply queue it to the workqueue in softirq context and let it service the transport.
Quoted 7 lines
The current draft is runtime-switchable via a new sunrpc
pool_modemodule parameter setting. When that's set to "workqueue", nfsd will use a workqueue-based service. One of the goals of this patchset was to *not* need to change any userland code, so starting it up using rpc.nfsd still works as expected. The only real difference is that the nfsdfs "threads" file is reinterpreted as the "max_active" value for the workqueue.
Hi Jeff,
I haven't looked very closely at the code, but in principal I think this is an excellent idea. Having to set a number of threads manually was never nice as it is impossible to give sensible guidance on what an appropriate number is.
Tying max_active to "threads" doesn't really make sense I think. "max_active" is a per-cpu number and I think the only meaningful numbers are "1" (meaning concurrent works might mutually deadlock) or infinity (which is approximated as 512). I would just ignore the "threads" number when workqueues are used.... or maybe enable workqueues when "auto" is written to "threads"??
Using a shrinker to manage the allocation and freeing of svc_rqst is a really good idea. It will put pressure on the effective number of threads when needed, but will not artificially constrain things.
The combination of workqueue and shrinker seems like a perfect match for nfsd.
I hope you can work out the latency issues!
Thanks,
NeilBrown
Quoted 25 lines · On Wed, 3 Dec 2014 12:11:18 +1100 NeilBrown <[email protected]> wrote:
Quoted 14 lines · On Tue, 2 Dec 2014 13:24:09 -0500 Jeff Layton <[email protected]> wrote:
tl;dr: this code works and is much simpler than the dedicated thread
pool, but there are some latencies in the workqueue code that seem to keep it from being as fast as it could be.This patchset is a little skunkworks project that I've been poking at for the last few weeks. Currently nfsd uses a dedicated thread pool to handle RPCs, but that requires maintaining a rather large swath of "fiddly" code to handle the threads and transports.
This patchset represents an alternative approach, which makes nfsd use workqueues to do its bidding rather than a dedicated thread pool. When a transport needs to do work, we simply queue it to the workqueue in softirq context and let it service the transport.
Quoted 7 lines
The current draft is runtime-switchable via a new sunrpc
pool_modemodule parameter setting. When that's set to "workqueue", nfsd will use a workqueue-based service. One of the goals of this patchset was to *not* need to change any userland code, so starting it up using rpc.nfsd still works as expected. The only real difference is that the nfsdfs "threads" file is reinterpreted as the "max_active" value for the workqueue.
Quoted 5 lines
Hi Jeff,
I haven't looked very closely at the code, but in principal I think this is an excellent idea. Having to set a number of threads manually was never nice as it is impossible to give sensible guidance on what an appropriate number is.
Yes, this should be a lot more "dynamic" in principle. A lightly-used nfs server using workqueues should really not consume much at all in the way of resources.
Quoted 7 lines
Tying
max_activeto "threads" doesn't really make sense I think. "max_active" is a per-cpu number and I think the only meaningful numbers are "1" (meaning concurrent works might mutually deadlock) or infinity (which is approximated as 512). I would just ignore the "threads" number when workqueues are used.... or maybe enable workqueues when "auto" is written to "threads"??
That's a good point. I had originally thought that max_active on an unbound workqueue would be the number of concurrent jobs that could run across all the CPUs, but now that I look I'm not sure that's really the case.
Certainly, not bounding this at all has some appeal, but having some limit here does help put an upper bound on the size of the svc_rqst cache.
I'll definitely give that some thought.
Quoted 4 lines
Using a shrinker to manage the allocation and freeing of
svc_rqstis a really good idea. It will put pressure on the effective number of threads when needed, but will not artificially constrain things.
That's my hope. We might also consider reaping idle rqsts ourselves as needed somehow, but for now I decided not to worry about it until we see whether this is really viable or not.
Quoted 7 lines
The combination of workqueue and shrinker seems like a perfect match for nfsd.
I hope you can work out the latency issues!
Thanks,
NeilBrown
I've heard random grumblings from various people in the past that workqueues have significant latency, but this is the first time I've really hit it in practice. If we can get this fixed, then that may be a significant perf win for all workqueue users. For instance, rpciod in the NFS client is all workqueue-based. Getting that latency down could really help things.
I'm currently trying to roll up a kernel module for benchmarking the workqueue dispatching code in the hopes that we can use that to help nail it down.
Thanks for having a look!
Signature · 1 line
Jeff Layton <[email protected]>
Hello, Neil, Jeff.
Quoted 4 lines · Jeff Layton
That's a good point. I had originally thought that
max_activeon an unbound workqueue would be the number of concurrent jobs that could run across all the CPUs, but now that I look I'm not sure that's really the case.
@max_active is a per-pool number. By default, unbound wqs use per-node pools, so @max_active would be per-node. Currently, @max_active is mostly meant as a protection against run-away workqueues creating crazy number of workers, which has been enough for the existing wq users. *Maybe* it makes sense to make it actually mean maximum concurrency which would prolly involve aggregated per-cpu distribution mechanism so that we don't end up inc'ing and dec'ing the same counter from all CPUs on each work item execution.
However, I do agree with Neil that making it user configurable is almost always painful. It's usually a question without a good answer and the same value may behave differently depending on a lot of implementation details and a better approach, probably, is to use @max_active as the last resort protection mechanism while providing automatic throttling of in-flight work items which is meaningful for the specific use cases.
Quoted 10 lines
I've heard random grumblings from various people in the past that workqueues have significant latency, but this is the first time I've really hit it in practice. If we can get this fixed, then that may be a significant perf win for all workqueue users. For instance, rpciod in the NFS client is all workqueue-based. Getting that latency down could really help things.
I'm currently trying to roll up a kernel module for benchmarking the workqueue dispatching code in the hopes that we can use that to help nail it down.
Definitely, there were some reportings but nothing really got tracked down properly. It'd be awesome to actually find out where the latency is coming from.
Thanks!
Signature · 1 line
tejun
Quoted 36 lines · On Wed, 3 Dec 2014 10:56:49 -0500 Tejun Heo <[email protected]> wrote:
Hello, Neil, Jeff.
Quoted 4 lines · Jeff Layton
That's a good point. I had originally thought that
max_activeon an unbound workqueue would be the number of concurrent jobs that could run across all the CPUs, but now that I look I'm not sure that's really the case.@max_active is a per-pool number. By default, unbound wqs use per-node pools, so @max_active would be per-node. Currently, @max_active is mostly meant as a protection against run-away workqueues creating crazy number of workers, which has been enough for the existing wq users. *Maybe* it makes sense to make it actually mean maximum concurrency which would prolly involve aggregated per-cpu distribution mechanism so that we don't end up inc'ing and dec'ing the same counter from all CPUs on each work item execution.
However, I do agree with Neil that making it user configurable is almost always painful. It's usually a question without a good answer and the same value may behave differently depending on a lot of implementation details and a better approach, probably, is to use @max_active as the last resort protection mechanism while providing automatic throttling of in-flight work items which is meaningful for the specific use cases.
Quoted 10 lines
I've heard random grumblings from various people in the past that workqueues have significant latency, but this is the first time I've really hit it in practice. If we can get this fixed, then that may be a significant perf win for all workqueue users. For instance, rpciod in the NFS client is all workqueue-based. Getting that latency down could really help things.
I'm currently trying to roll up a kernel module for benchmarking the workqueue dispatching code in the hopes that we can use that to help nail it down.
Quoted 6 lines
Definitely, there were some reportings but nothing really got tracked down properly. It'd be awesome to actually find out where the latency is coming from.
Thanks!
I think I might have figured this out (and before I go any farther allow me to say <facepalm>), thanks to the workqueue tracepoints in the code. What I noticed is that when things are fairly idle, the work is picked up quickly, but once things get busy it takes a lot longer.
I think that the issue is in the design of the workqueue-based nfsd code. In particular, I attached a work_struct to the svc_xprt which is limiting the code to only process one RPC at a time for a xprt, from beginning to end.
So, even if we requeue that work after the receive phase is done, the workqueue won't pick it up again until the thing is processed and the reply is sent.
What I think I need to do is to do the receive phase using the work_struct attached to the xprt, and then do the rest of the processing from the context of a different work_struct (possibly one attached to the svc_rqst), which should free up the xprt's work_struct sooner.
I'm going to work on changing that today and see if it improves things.
Thanks for the help so far!
Signature · 1 line
Jeff Layton <[email protected]>
Quoted 66 lines · On Wed, 3 Dec 2014 11:04:05 -0500 Jeff Layton <[email protected]> wrote:
Quoted 36 lines · On Wed, 3 Dec 2014 10:56:49 -0500 Tejun Heo <[email protected]> wrote:
Hello, Neil, Jeff.
Quoted 4 lines · Jeff Layton
That's a good point. I had originally thought that
max_activeon an unbound workqueue would be the number of concurrent jobs that could run across all the CPUs, but now that I look I'm not sure that's really the case.@max_active is a per-pool number. By default, unbound wqs use per-node pools, so @max_active would be per-node. Currently, @max_active is mostly meant as a protection against run-away workqueues creating crazy number of workers, which has been enough for the existing wq users. *Maybe* it makes sense to make it actually mean maximum concurrency which would prolly involve aggregated per-cpu distribution mechanism so that we don't end up inc'ing and dec'ing the same counter from all CPUs on each work item execution.
However, I do agree with Neil that making it user configurable is almost always painful. It's usually a question without a good answer and the same value may behave differently depending on a lot of implementation details and a better approach, probably, is to use @max_active as the last resort protection mechanism while providing automatic throttling of in-flight work items which is meaningful for the specific use cases.
Quoted 10 lines
I've heard random grumblings from various people in the past that workqueues have significant latency, but this is the first time I've really hit it in practice. If we can get this fixed, then that may be a significant perf win for all workqueue users. For instance, rpciod in the NFS client is all workqueue-based. Getting that latency down could really help things.
I'm currently trying to roll up a kernel module for benchmarking the workqueue dispatching code in the hopes that we can use that to help nail it down.
Quoted 6 lines
Definitely, there were some reportings but nothing really got tracked down properly. It'd be awesome to actually find out where the latency is coming from.
Thanks!
I think I might have figured this out (and before I go any farther allow me to say <facepalm>), thanks to the workqueue tracepoints in the code. What I noticed is that when things are fairly idle, the work is picked up quickly, but once things get busy it takes a lot longer.
I think that the issue is in the design of the workqueue-based nfsd code. In particular, I attached a
work_structto thesvc_xprtwhich is limiting the code to only process one RPC at a time for a xprt, from beginning to end.So, even if we requeue that work after the receive phase is done, the workqueue won't pick it up again until the thing is processed and the reply is sent.
What I think I need to do is to do the receive phase using the
work_structattached to the xprt, and then do the rest of the processing from the context of a differentwork_struct(possibly one attached to thesvc_rqst), which should free up the xprt'swork_structsooner.
Quoted 3 lines
I'm going to work on changing that today and see if it improves things.
Thanks for the help so far!
Yes! That does help. The new workqueue based code is a little (a few percent?) slower than the thread-based code across the board. I suspect that's due to the fact that I'm having to queue each RPC to the workqueue twice (once for the receive and once to do the processing).
I suspect that I can remedy that, but I'll have to think about the best way to do it.
Thanks again for the help!
Signature · 1 line
Jeff Layton <[email protected]>
Quoted 73 lines · Jeff Layton
Quoted 66 lines · On Wed, 3 Dec 2014 11:04:05 -0500 Jeff Layton <[email protected]> wrote:
Quoted 36 lines · On Wed, 3 Dec 2014 10:56:49 -0500 Tejun Heo <[email protected]> wrote:
Hello, Neil, Jeff.
Quoted 4 lines · Jeff Layton
That's a good point. I had originally thought that
max_activeon an unbound workqueue would be the number of concurrent jobs that could run across all the CPUs, but now that I look I'm not sure that's really the case.@max_active is a per-pool number. By default, unbound wqs use per-node pools, so @max_active would be per-node. Currently, @max_active is mostly meant as a protection against run-away workqueues creating crazy number of workers, which has been enough for the existing wq users. *Maybe* it makes sense to make it actually mean maximum concurrency which would prolly involve aggregated per-cpu distribution mechanism so that we don't end up inc'ing and dec'ing the same counter from all CPUs on each work item execution.
However, I do agree with Neil that making it user configurable is almost always painful. It's usually a question without a good answer and the same value may behave differently depending on a lot of implementation details and a better approach, probably, is to use @max_active as the last resort protection mechanism while providing automatic throttling of in-flight work items which is meaningful for the specific use cases.
Quoted 10 lines
I've heard random grumblings from various people in the past that workqueues have significant latency, but this is the first time I've really hit it in practice. If we can get this fixed, then that may be a significant perf win for all workqueue users. For instance, rpciod in the NFS client is all workqueue-based. Getting that latency down could really help things.
I'm currently trying to roll up a kernel module for benchmarking the workqueue dispatching code in the hopes that we can use that to help nail it down.
Quoted 6 lines
Definitely, there were some reportings but nothing really got tracked down properly. It'd be awesome to actually find out where the latency is coming from.
Thanks!
I think I might have figured this out (and before I go any farther allow me to say <facepalm>), thanks to the workqueue tracepoints in the code. What I noticed is that when things are fairly idle, the work is picked up quickly, but once things get busy it takes a lot longer.
I think that the issue is in the design of the workqueue-based nfsd code. In particular, I attached a
work_structto thesvc_xprtwhich is limiting the code to only process one RPC at a time for a xprt, from beginning to end.So, even if we requeue that work after the receive phase is done, the workqueue won't pick it up again until the thing is processed and the reply is sent.
What I think I need to do is to do the receive phase using the
work_structattached to the xprt, and then do the rest of the processing from the context of a differentwork_struct(possibly one attached to thesvc_rqst), which should free up the xprt'swork_structsooner.Quoted 3 lines
I'm going to work on changing that today and see if it improves things.
Thanks for the help so far!
Quoted 8 lines
Yes! That does help. The new workqueue based code is a little (a few percent?) slower than the thread-based code across the board. I suspect that's due to the fact that I'm having to queue each RPC to the workqueue twice (once for the receive and once to do the processing).
I suspect that I can remedy that, but I'll have to think about the best way to do it.
Which workqueue are you using? Since the receive code is non-blocking, I'd expect you might be able to use rpciod, for the initial socket reads, but you wouldn't want to use that for the actual knfsd processing.
Signature · 5 lines
Trond Myklebust Linux NFS client maintainer, PrimaryData [email protected]
Quoted 79 lines · On Wed, 3 Dec 2014 14:08:01 -0500 Trond Myklebust…
Quoted 78 lines · Jeff Layton
Quoted 66 lines · On Wed, 3 Dec 2014 11:04:05 -0500 Jeff Layton <[email protected]> wrote:
Quoted 36 lines · On Wed, 3 Dec 2014 10:56:49 -0500 Tejun Heo <[email protected]> wrote:
Hello, Neil, Jeff.
Quoted 4 lines · Jeff Layton
That's a good point. I had originally thought that
max_activeon an unbound workqueue would be the number of concurrent jobs that could run across all the CPUs, but now that I look I'm not sure that's really the case.@max_active is a per-pool number. By default, unbound wqs use per-node pools, so @max_active would be per-node. Currently, @max_active is mostly meant as a protection against run-away workqueues creating crazy number of workers, which has been enough for the existing wq users. *Maybe* it makes sense to make it actually mean maximum concurrency which would prolly involve aggregated per-cpu distribution mechanism so that we don't end up inc'ing and dec'ing the same counter from all CPUs on each work item execution.
However, I do agree with Neil that making it user configurable is almost always painful. It's usually a question without a good answer and the same value may behave differently depending on a lot of implementation details and a better approach, probably, is to use @max_active as the last resort protection mechanism while providing automatic throttling of in-flight work items which is meaningful for the specific use cases.
Quoted 10 lines
I've heard random grumblings from various people in the past that workqueues have significant latency, but this is the first time I've really hit it in practice. If we can get this fixed, then that may be a significant perf win for all workqueue users. For instance, rpciod in the NFS client is all workqueue-based. Getting that latency down could really help things.
I'm currently trying to roll up a kernel module for benchmarking the workqueue dispatching code in the hopes that we can use that to help nail it down.
Quoted 6 lines
Definitely, there were some reportings but nothing really got tracked down properly. It'd be awesome to actually find out where the latency is coming from.
Thanks!
I think I might have figured this out (and before I go any farther allow me to say <facepalm>), thanks to the workqueue tracepoints in the code. What I noticed is that when things are fairly idle, the work is picked up quickly, but once things get busy it takes a lot longer.
I think that the issue is in the design of the workqueue-based nfsd code. In particular, I attached a
work_structto thesvc_xprtwhich is limiting the code to only process one RPC at a time for a xprt, from beginning to end.So, even if we requeue that work after the receive phase is done, the workqueue won't pick it up again until the thing is processed and the reply is sent.
What I think I need to do is to do the receive phase using the
work_structattached to the xprt, and then do the rest of the processing from the context of a differentwork_struct(possibly one attached to thesvc_rqst), which should free up the xprt'swork_structsooner.Quoted 3 lines
I'm going to work on changing that today and see if it improves things.
Thanks for the help so far!
Yes! That does help. The new workqueue based code is a little (a few percent?) slower than the thread-based code across the board. I suspect that's due to the fact that I'm having to queue each RPC to the workqueue twice (once for the receive and once to do the processing).
Quoted 9 lines
Quoted 3 lines
I suspect that I can remedy that, but I'll have to think about the best way to do it.
Which workqueue are you using? Since the receive code is non-blocking, I'd expect you might be able to use rpciod, for the initial socket reads, but you wouldn't want to use that for the actual knfsd processing.
I'm using the same (nfsd) workqueue for everything. The workqueue isn't really the bottleneck though, it's the work_struct.
Basically, the problem is that the work_struct in the svc_xprt was remaining busy for far too long. So, even though the XPT_BUSY bit had cleared, the work wouldn't get picked up again until the previous workqueue job had returned.
With the change I made today, I just added a new work_struct to svc_rqst and queue that to the same workqueue to do svc_process as soon as the receive is done. That means though that each RPC ends up waiting in the queue twice (once to do the receive and once to process the RPC), and I think that's probably the reason for the performance delta.
What I think I'm going to do on the next pass is have the job that enqueues the xprt instead try to find an svc_rqst. If it finds it, then it can go ahead and queue the work struct in it to do the receive and processing in a single go.
If it can't find one, it'll queue the xprt's work to allocate one and then queue that to do all of the work as before. That will likely penalize the case where there isn't an available svc_rqst, but in the common case that there is one it should go quickly.
Thanks!
Signature · 1 line
Jeff Layton <[email protected]>
Quoted 16 lines · Jeff Layton
Quoted 5 lines · On Wed, 3 Dec 2014 14:08:01 -0500 Trond Myklebust…
Which workqueue are you using? Since the receive code is non-blocking, I'd expect you might be able to use rpciod, for the initial socket reads, but you wouldn't want to use that for the actual knfsd processing.
I'm using the same (nfsd) workqueue for everything. The workqueue isn't really the bottleneck though, it's the
work_struct.Basically, the problem is that the
work_structin thesvc_xprtwas remaining busy for far too long. So, even though theXPT_BUSYbit had cleared, the work wouldn't get picked up again until the previous workqueue job had returned.
Quoted 5 lines
With the change I made today, I just added a new
work_structtosvc_rqstand queue that to the same workqueue to dosvc_processas soon as the receive is done. That means though that each RPC ends up waiting in the queue twice (once to do the receive and once to process the RPC), and I think that's probably the reason for the performance delta.
Why would the queuing latency still be significant now?
Quoted 9 lines
What I think I'm going to do on the next pass is have the job that enqueues the xprt instead try to find an
svc_rqst. If it finds it, then it can go ahead and queue the work struct in it to do the receive and processing in a single go.If it can't find one, it'll queue the xprt's work to allocate one and then queue that to do all of the work as before. That will likely penalize the case where there isn't an available
svc_rqst, but in the common case that there is one it should go quickly.
Signature · 5 lines
Trond Myklebust Linux NFS client maintainer, PrimaryData [email protected]
Quoted 17 lines · On Wed, 3 Dec 2014 14:59:43 -0500 Trond Myklebust…
Quoted 16 lines · Jeff Layton
Quoted 5 lines · On Wed, 3 Dec 2014 14:08:01 -0500 Trond Myklebust…
Which workqueue are you using? Since the receive code is non-blocking, I'd expect you might be able to use rpciod, for the initial socket reads, but you wouldn't want to use that for the actual knfsd processing.
I'm using the same (nfsd) workqueue for everything. The workqueue isn't really the bottleneck though, it's the
work_struct.Basically, the problem is that the
work_structin thesvc_xprtwas remaining busy for far too long. So, even though theXPT_BUSYbit had cleared, the work wouldn't get picked up again until the previous workqueue job had returned.
Quoted 8 lines
Quoted 5 lines
With the change I made today, I just added a new
work_structtosvc_rqstand queue that to the same workqueue to dosvc_processas soon as the receive is done. That means though that each RPC ends up waiting in the queue twice (once to do the receive and once to process the RPC), and I think that's probably the reason for the performance delta.Why would the queuing latency still be significant now?
That, I'm not clear on yet and that may not be why this is slower. But, I was seeing slightly faster performance with reads before I made today's changes. If changing how these jobs get queued doesn't help the performance, then I'll have to look elsewhere...
Quoted 11 lines
Quoted 9 lines
What I think I'm going to do on the next pass is have the job that enqueues the xprt instead try to find an
svc_rqst. If it finds it, then it can go ahead and queue the work struct in it to do the receive and processing in a single go.If it can't find one, it'll queue the xprt's work to allocate one and then queue that to do all of the work as before. That will likely penalize the case where there isn't an available
svc_rqst, but in the common case that there is one it should go quickly.
Signature · 1 line
Jeff Layton <[email protected]>
Quoted 26 lines · Jeff Layton
Quoted 23 lines · On Wed, 3 Dec 2014 14:59:43 -0500 Trond Myklebust…
Quoted 21 lines · Jeff Layton
Quoted 5 lines · On Wed, 3 Dec 2014 14:08:01 -0500 Trond Myklebust…
Which workqueue are you using? Since the receive code is non-blocking, I'd expect you might be able to use rpciod, for the initial socket reads, but you wouldn't want to use that for the actual knfsd processing.
I'm using the same (nfsd) workqueue for everything. The workqueue isn't really the bottleneck though, it's the
work_struct.Basically, the problem is that the
work_structin thesvc_xprtwas remaining busy for far too long. So, even though theXPT_BUSYbit had cleared, the work wouldn't get picked up again until the previous workqueue job had returned.With the change I made today, I just added a new
work_structtosvc_rqstand queue that to the same workqueue to dosvc_processas soon as the receive is done. That means though that each RPC ends up waiting in the queue twice (once to do the receive and once to process the RPC), and I think that's probably the reason for the performance delta.
Quoted 7 lines
Quoted 2 lines
Why would the queuing latency still be significant now?
That, I'm not clear on yet and that may not be why this is slower. But, I was seeing slightly faster performance with reads before I made today's changes. If changing how these jobs get queued doesn't help the performance, then I'll have to look elsewhere...
Do you have a good method for measuring that latency? If the queuing latency turns out to depend on the execution latency for each job, then perhaps running the message receives on a separate low latency queue could help (hence the suggestion to use rpciod).
Signature · 5 lines
Trond Myklebust Linux NFS client maintainer, PrimaryData [email protected]
Quoted 35 lines · On Wed, 3 Dec 2014 15:44:31 -0500 Trond Myklebust…
Quoted 26 lines · Jeff Layton
Quoted 23 lines · On Wed, 3 Dec 2014 14:59:43 -0500 Trond Myklebust…
Quoted 21 lines · Jeff Layton
Quoted 5 lines · On Wed, 3 Dec 2014 14:08:01 -0500 Trond Myklebust…
Which workqueue are you using? Since the receive code is non-blocking, I'd expect you might be able to use rpciod, for the initial socket reads, but you wouldn't want to use that for the actual knfsd processing.
I'm using the same (nfsd) workqueue for everything. The workqueue isn't really the bottleneck though, it's the
work_struct.Basically, the problem is that the
work_structin thesvc_xprtwas remaining busy for far too long. So, even though theXPT_BUSYbit had cleared, the work wouldn't get picked up again until the previous workqueue job had returned.With the change I made today, I just added a new
work_structtosvc_rqstand queue that to the same workqueue to dosvc_processas soon as the receive is done. That means though that each RPC ends up waiting in the queue twice (once to do the receive and once to process the RPC), and I think that's probably the reason for the performance delta.Quoted 7 lines
Quoted 2 lines
Why would the queuing latency still be significant now?
That, I'm not clear on yet and that may not be why this is slower. But, I was seeing slightly faster performance with reads before I made today's changes. If changing how these jobs get queued doesn't help the performance, then I'll have to look elsewhere...
Quoted 5 lines
Do you have a good method for measuring that latency? If the queuing latency turns out to depend on the execution latency for each job, then perhaps running the message receives on a separate low latency queue could help (hence the suggestion to use rpciod).
I was using ftrace with the sunrpc:* and workqueue:* tracepoints, and had a simple perl script to postprocess the trace info to figure out average/min/max latency.
I don't think the queueing latency is that significant per-se, but I think the best thing is to avoid making multiple trips through the workqueue per RPC if we can help it. I tested and pushed a newer patchset to my repo last night that does that (at least if there's already a svc_rqst available when the xprt needs servicing). It seems to be pretty close speed-wise to the thread-based code.
The next step is to test this out on something larger-scale. I'm hoping to get access to just such a test rig soon. Once we have some results from that, I think I'll have a much better idea of how viable this approach is and where other potential bottlenecks might be.
Thanks!
Signature · 1 line
Jeff Layton <[email protected]>
I am looking at how to reduce total RPC execution time in NFS/RDMA. mountstats output shows that RPC backlog wait is too long, but increasing the credit limit doesn't seem help. Would this patchset help reducing total RPC execution time?
Shirley
Quoted 3 lines · Jeff Layton
I was using ftrace with the sunrpc:* and workqueue:* tracepoints, and had a simple perl script to postprocess the trace info to figure out average/min/max latency.
Quoted 4 lines · On Thu, 04 Dec 2014 09:17:17 -0800 Shirley Ma <[email protected]> wrote:
I am looking at how to reduce total RPC execution time in NFS/RDMA. mountstats output shows that RPC backlog wait is too long, but increasing the credit limit doesn't seem help. Would this patchset help reducing total RPC execution time?
Shirley
I'm not sure. It depends on why you're seeing a backlog.
So far, my testing still shows this to be slightly (~2%) slower than the same setup running a threaded nfsd, but that could be different with a NUMA server or faster disks.
Probably there is some more tuning to do here before this is quite ready for prime-time. It may be worth testing in your environment though if you have the time and ability to do so.
Quoted 4 lines · Jeff Layton
Quoted 3 lines
I was using ftrace with the sunrpc:* and workqueue:* tracepoints, and had a simple perl script to postprocess the trace info to figure out average/min/max latency.
Signature · 1 line
Jeff Layton <[email protected]>
Quoted 8 lines · Jeff Layton
Quoted 4 lines · On Thu, 04 Dec 2014 09:17:17 -0800 Shirley Ma <[email protected]> wrote:
Quoted 4 lines
I am looking at how to reduce total RPC execution time in NFS/RDMA. mountstats output shows that RPC backlog wait is too long, but increasing the credit limit doesn't seem help. Would this patchset help reducing total RPC execution time?
Shirley
I'm not sure. It depends on why you're seeing a backlog.
What's the major factors contributing RPC backlog wait time? I guess I need to probe the backlog wait path to display fine-grained latency cost.
Quoted 12 lines
So far, my testing still shows this to be slightly (~2%) slower than the same setup running a threaded nfsd, but that could be different with a NUMA server or faster disks.
Probably there is some more tuning to do here before this is quite ready for prime-time. It may be worth testing in your environment though if you have the time and ability to do so.
Quoted 4 lines
Quoted 4 lines · Jeff Layton
Quoted 3 lines
Quoted 3 lines
I was using ftrace with the sunrpc:* and workqueue:* tracepoints, and had a simple perl script to postprocess the trace info to figure out average/min/max latency.
Quoted 32 lines · Tejun Heo
Hello, Neil, Jeff.
Quoted 4 lines · Jeff Layton
That's a good point. I had originally thought that
max_activeon an unbound workqueue would be the number of concurrent jobs that could run across all the CPUs, but now that I look I'm not sure that's really the case.@max_active is a per-pool number. By default, unbound wqs use per-node pools, so @max_active would be per-node. Currently, @max_active is mostly meant as a protection against run-away workqueues creating crazy number of workers, which has been enough for the existing wq users. *Maybe* it makes sense to make it actually mean maximum concurrency which would prolly involve aggregated per-cpu distribution mechanism so that we don't end up inc'ing and dec'ing the same counter from all CPUs on each work item execution.
However, I do agree with Neil that making it user configurable is almost always painful. It's usually a question without a good answer and the same value may behave differently depending on a lot of implementation details and a better approach, probably, is to use @max_active as the last resort protection mechanism while providing automatic throttling of in-flight work items which is meaningful for the specific use cases.
Quoted 7 lines
I've heard random grumblings from various people in the past that workqueues have significant latency, but this is the first time I've really hit it in practice. If we can get this fixed, then that may be a significant perf win for all workqueue users. For instance, rpciod in the NFS client is all workqueue-based. Getting that latency down could really help things.
Quoted 7 lines
Quoted 3 lines
I'm currently trying to roll up a kernel module for benchmarking the workqueue dispatching code in the hopes that we can use that to help nail it down.
Definitely, there were some reportings but nothing really got tracked down properly. It'd be awesome to actually find out where the latency is coming from.
I’ve measured some long (>10usec) latencies for queue_workqueue() and wake_up_bit() calls with CPU-intensive workloads.
Often these APIs are invoked while one or more spinlocks are held. That makes it easy to back up a lot of work if one of these calls is slow for any reason.
For wake_up_bit(), this doesn’t include how long it takes before the awoken process is run. With CPU-intensive workloads, it’s often the case that hundreds of usecs elapse, and the awoken process is migrated to another CPU (observed via ftrace).
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com