On Mon, 2024-07-15 at 17:14 +1000, NeilBrown wrote:
> sp_nrthreads is only ever accessed under the service mutex
> nlmsvc_mutex nfs_callback_mutex nfsd_mutex
> so these is no need for it to be an atomic_t.
>
> The fact that all code using it is single-threaded means that we can
> simplify svc_pool_victim and remove the temporary elevation of
> sp_nrthreads.
>
> Signed-off-by: NeilBrown <neilb@xxxxxxx>
> ---
> fs/nfsd/nfsctl.c | 2 +-
> fs/nfsd/nfssvc.c | 2 +-
> include/linux/sunrpc/svc.h | 4 ++--
> net/sunrpc/svc.c | 31 +++++++++++--------------------
> 4 files changed, 15 insertions(+), 24 deletions(-)
>
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 5b0f2e0d7ccf..d85b6d1fa31f 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -1769,7 +1769,7 @@ int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info)
> struct svc_pool *sp = &nn->nfsd_serv->sv_pools[i];
>
> err = nla_put_u32(skb, NFSD_A_SERVER_THREADS,
> - atomic_read(&sp->sp_nrthreads));
> + sp->sp_nrthreads);
> if (err)
> goto err_unlock;
> }
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index 4438cdcd4873..7377422a34df 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -641,7 +641,7 @@ int nfsd_get_nrthreads(int n, int *nthreads, struct net *net)
>
> if (serv)
> for (i = 0; i < serv->sv_nrpools && i < n; i++)
> - nthreads[i] = atomic_read(&serv->sv_pools[i].sp_nrthreads);
> + nthreads[i] = serv->sv_pools[i].sp_nrthreads;
> return 0;
> }
>
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index e4fa25fafa97..99e9345d829e 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -33,9 +33,9 @@
> * node traffic on multi-node NUMA NFS servers.
> */
> struct svc_pool {
> - unsigned int sp_id; /* pool id; also node id on NUMA */
> + unsigned int sp_id; /* pool id; also node id on NUMA */
> struct lwq sp_xprts; /* pending transports */
> - atomic_t sp_nrthreads; /* # of threads in pool */
> + unsigned int sp_nrthreads; /* # of threads in pool */
> struct list_head sp_all_threads; /* all server threads */
> struct llist_head sp_idle_threads; /* idle server threads */
>
> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> index 072ad115ae3d..0d8588bc693c 100644
> --- a/net/sunrpc/svc.c
> +++ b/net/sunrpc/svc.c
> @@ -725,7 +725,7 @@ svc_prepare_thread(struct svc_serv *serv, struct svc_pool *pool, int node)
> serv->sv_nrthreads += 1;
> spin_unlock_bh(&serv->sv_lock);
>
> - atomic_inc(&pool->sp_nrthreads);
> + pool->sp_nrthreads += 1;
>
> /* Protected by whatever lock the service uses when calling
> * svc_set_num_threads()
> @@ -780,31 +780,22 @@ svc_pool_victim(struct svc_serv *serv, struct svc_pool *target_pool,
> struct svc_pool *pool;
> unsigned int i;
>
> -retry:
> pool = target_pool;
>
> - if (pool != NULL) {
> - if (atomic_inc_not_zero(&pool->sp_nrthreads))
> - goto found_pool;
> - return NULL;
> - } else {
> + if (!pool) {
> for (i = 0; i < serv->sv_nrpools; i++) {
> pool = &serv->sv_pools[--(*state) % serv->sv_nrpools];
> - if (atomic_inc_not_zero(&pool->sp_nrthreads))
> - goto found_pool;
> + if (pool->sp_nrthreads)
> + break;
> }
> - return NULL;
> }
>
> -found_pool:
> - set_bit(SP_VICTIM_REMAINS, &pool->sp_flags);
> - set_bit(SP_NEED_VICTIM, &pool->sp_flags);
> - if (!atomic_dec_and_test(&pool->sp_nrthreads))
> + if (pool && pool->sp_nrthreads) {
> + set_bit(SP_VICTIM_REMAINS, &pool->sp_flags);
> + set_bit(SP_NEED_VICTIM, &pool->sp_flags);
> return pool;
> - /* Nothing left in this pool any more */
> - clear_bit(SP_NEED_VICTIM, &pool->sp_flags);
> - clear_bit(SP_VICTIM_REMAINS, &pool->sp_flags);
> - goto retry;
> + }
> + return NULL;
> }
>
> static int
> @@ -883,7 +874,7 @@ svc_set_num_threads(struct svc_serv *serv, struct svc_pool *pool, int nrservs)
> if (!pool)
> nrservs -= serv->sv_nrthreads;
> else
> - nrservs -= atomic_read(&pool->sp_nrthreads);
> + nrservs -= pool->sp_nrthreads;
>
> if (nrservs > 0)
> return svc_start_kthreads(serv, pool, nrservs);
> @@ -953,7 +944,7 @@ svc_exit_thread(struct svc_rqst *rqstp)
>
> list_del_rcu(&rqstp->rq_all);
>
> - atomic_dec(&pool->sp_nrthreads);
> + pool->sp_nrthreads -= 1;
>
> spin_lock_bh(&serv->sv_lock);
> serv->sv_nrthreads -= 1;
I don't think svc_exit_thread is called with the nfsd_mutex held, so if
several threads were exiting at the same time, they could race here.
--
Jeff Layton <jlayton@xxxxxxxxxx>
