| Message ID | 163763097544.7284.15751243743215653521.stgit@noble.brown (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SUNRPC: clean up server thread management | expand |
> On Nov 22, 2021, at 8:29 PM, NeilBrown <neilb@suse.de> wrote: > > Using sv_lock means we don't need to hold the service mutex over these > updates. > > In particular, svc_exit_thread() no longer requires synchronisation, so > threads can exit asynchronously. > > Signed-off-by: NeilBrown <neilb@suse.de> > --- > fs/nfsd/nfssvc.c | 5 ++--- > net/sunrpc/svc.c | 9 +++++++-- > 2 files changed, 9 insertions(+), 5 deletions(-) > > diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c > index fc5899502a83..e9c9fa820b17 100644 > --- a/fs/nfsd/nfssvc.c > +++ b/fs/nfsd/nfssvc.c > @@ -55,9 +55,8 @@ static __be32 nfsd_init_request(struct svc_rqst *, > struct svc_process_info *); > > /* > - * nfsd_mutex protects nn->nfsd_serv -- both the pointer itself and the members > - * of the svc_serv struct. In particular, ->sv_nrthreads but also to some > - * extent ->sv_temp_socks and ->sv_permsocks. > + * nfsd_mutex protects nn->nfsd_serv -- both the pointer itself and some members > + * of the svc_serv struct such as ->sv_temp_socks and ->sv_permsocks. > * > * If (out side the lock) nn->nfsd_serv is non-NULL, then it must point to a > * properly initialised 'struct svc_serv' with ->sv_nrthreads > 0 (unless > diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c > index acddc6e12e9e..2b2042234e4b 100644 > --- a/net/sunrpc/svc.c > +++ b/net/sunrpc/svc.c > @@ -523,7 +523,7 @@ EXPORT_SYMBOL_GPL(svc_shutdown_net); > > /* > * Destroy an RPC service. Should be called with appropriate locking to > - * protect the sv_nrthreads, sv_permsocks and sv_tempsocks. > + * protect sv_permsocks and sv_tempsocks. > */ > void > svc_destroy(struct kref *ref) > @@ -639,7 +639,10 @@ svc_prepare_thread(struct svc_serv *serv, struct svc_pool *pool, int node) > return ERR_PTR(-ENOMEM); > > svc_get(serv); > - serv->sv_nrthreads++; > + spin_lock_bh(&serv->sv_lock); > + serv->sv_nrthreads += 1; > + spin_unlock_bh(&serv->sv_lock); atomic_t would be somewhat lighter weight. Can it be used here instead? > + > spin_lock_bh(&pool->sp_lock); > pool->sp_nrthreads++; > list_add_rcu(&rqstp->rq_all, &pool->sp_all_threads); > @@ -880,7 +883,9 @@ svc_exit_thread(struct svc_rqst *rqstp) > list_del_rcu(&rqstp->rq_all); > spin_unlock_bh(&pool->sp_lock); > > + spin_lock_bh(&serv->sv_lock); > serv->sv_nrthreads -= 1; > + spin_unlock_bh(&serv->sv_lock); > svc_sock_update_bufs(serv); > > svc_rqst_free(rqstp); > > -- Chuck Lever
On Wed, 24 Nov 2021, Chuck Lever III wrote: > > @@ -639,7 +639,10 @@ svc_prepare_thread(struct svc_serv *serv, struct svc_pool *pool, int node) > > return ERR_PTR(-ENOMEM); > > > > svc_get(serv); > > - serv->sv_nrthreads++; > > + spin_lock_bh(&serv->sv_lock); > > + serv->sv_nrthreads += 1; > > + spin_unlock_bh(&serv->sv_lock); > > atomic_t would be somewhat lighter weight. Can it be used here > instead? > We could.... but sv_nrthreads is read-mostly. There are 11 places where we would need to call "atomic_read()", and just two where we benefit from the simplicity of atomic_inc/dec. And even if I did achieve dynamic threads count management, we would not be changing sv_nrthreads often enough that any performance difference would be noticeable. So I'd rather stick with using the spinlock and keeping the read-side simple. Thanks, NeilBrown
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c index fc5899502a83..e9c9fa820b17 100644 --- a/fs/nfsd/nfssvc.c +++ b/fs/nfsd/nfssvc.c @@ -55,9 +55,8 @@ static __be32 nfsd_init_request(struct svc_rqst *, struct svc_process_info *); /* - * nfsd_mutex protects nn->nfsd_serv -- both the pointer itself and the members - * of the svc_serv struct. In particular, ->sv_nrthreads but also to some - * extent ->sv_temp_socks and ->sv_permsocks. + * nfsd_mutex protects nn->nfsd_serv -- both the pointer itself and some members + * of the svc_serv struct such as ->sv_temp_socks and ->sv_permsocks. * * If (out side the lock) nn->nfsd_serv is non-NULL, then it must point to a * properly initialised 'struct svc_serv' with ->sv_nrthreads > 0 (unless diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c index acddc6e12e9e..2b2042234e4b 100644 --- a/net/sunrpc/svc.c +++ b/net/sunrpc/svc.c @@ -523,7 +523,7 @@ EXPORT_SYMBOL_GPL(svc_shutdown_net); /* * Destroy an RPC service. Should be called with appropriate locking to - * protect the sv_nrthreads, sv_permsocks and sv_tempsocks. + * protect sv_permsocks and sv_tempsocks. */ void svc_destroy(struct kref *ref) @@ -639,7 +639,10 @@ svc_prepare_thread(struct svc_serv *serv, struct svc_pool *pool, int node) return ERR_PTR(-ENOMEM); svc_get(serv); - serv->sv_nrthreads++; + spin_lock_bh(&serv->sv_lock); + serv->sv_nrthreads += 1; + spin_unlock_bh(&serv->sv_lock); + spin_lock_bh(&pool->sp_lock); pool->sp_nrthreads++; list_add_rcu(&rqstp->rq_all, &pool->sp_all_threads); @@ -880,7 +883,9 @@ svc_exit_thread(struct svc_rqst *rqstp) list_del_rcu(&rqstp->rq_all); spin_unlock_bh(&pool->sp_lock); + spin_lock_bh(&serv->sv_lock); serv->sv_nrthreads -= 1; + spin_unlock_bh(&serv->sv_lock); svc_sock_update_bufs(serv); svc_rqst_free(rqstp);
Using sv_lock means we don't need to hold the service mutex over these updates. In particular, svc_exit_thread() no longer requires synchronisation, so threads can exit asynchronously. Signed-off-by: NeilBrown <neilb@suse.de> --- fs/nfsd/nfssvc.c | 5 ++--- net/sunrpc/svc.c | 9 +++++++-- 2 files changed, 9 insertions(+), 5 deletions(-)