Re: [PATCH v5 5/5] sunrpc: derive the pool count instead of caching it in sv_nrpools

[NeilBrown]

On Mon, 06 Jul 2026, Jeff Layton wrote:
> Now that the pool mode is always pernode, svc_serv.sv_nrpools is
> redundant with sv_is_pooled: an unpooled service always has a single
> pool, and a pooled service has svc_pool_map.npools pools (which is one on
> a single-node host). sv_nrpools cannot distinguish an unpooled service
> from a pooled service that happens to have one pool, so it is sv_nrpools,
> not sv_is_pooled, that carries no unique information.
> 
> Replace the cached field with a svc_serv_nrpools() helper that derives
> the count from sv_is_pooled and the pool map, and convert all readers to
> it. svc_pool_map is file-local to svc.c, so export the helper for the
> svc_xprt.c and nfsd callers.
> 
> Reading svc_pool_map.npools without svc_pool_map_mutex is safe: the
> mutex protects only svc_pool_map.count, and npools is already read
> locklessly in svc_pool_for_cpu().
> 
> A pooled service holds a map reference for its whole lifetime, so npools
> is stable while any reader could observe it.  The hot path
> (svc_pool_for_cpu()) already dereferences svc_pool_map for to_pool, and
> npools shares that cacheline, so there is no new locking or coherence
> cost.
> 
> __svc_create() keeps using its local npools argument for the sv_pools[]
> allocation, since sv_is_pooled is not set until svc_create_pooled() has
> returned from it.
> 
> Doing this also removes a modulus operation from svc_pool_for_cpu(),
> which should make for more efficient RPC queueing.
> 
> Assisted-by: Claude:claude-opus-4-8
> Suggested-by: NeilBrown <neilb@xxxxxxxxxxx>
> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> ---
>  fs/nfsd/nfsctl.c           |  2 +-
>  fs/nfsd/nfssvc.c           | 10 ++++-----
>  include/linux/sunrpc/svc.h |  2 +-
>  net/sunrpc/svc.c           | 52 ++++++++++++++++++++++++++++++++--------------
>  net/sunrpc/svc_xprt.c      |  6 +++---
>  5 files changed, 46 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index bc16fc7ca24f..0543e5bb842f 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -1526,7 +1526,7 @@ int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,
>  
>  	rcu_read_lock();
>  
> -	for (i = 0; i < nn->nfsd_serv->sv_nrpools; i++) {
> +	for (i = 0; i < svc_serv_nrpools(nn->nfsd_serv); i++) {
>  		struct svc_rqst *rqstp;
>  		long thread_skip = 0;
>  
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index a8ea4dbfa56b..2edf716ea022 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -655,7 +655,7 @@ int nfsd_nrpools(struct net *net)
>  	if (nn->nfsd_serv == NULL)
>  		return 0;
>  	else
> -		return nn->nfsd_serv->sv_nrpools;
> +		return svc_serv_nrpools(nn->nfsd_serv);
>  }
>  
>  int nfsd_get_nrthreads(int n, int *nthreads, struct net *net)
> @@ -665,7 +665,7 @@ int nfsd_get_nrthreads(int n, int *nthreads, struct net *net)
>  	int i;
>  
>  	if (serv)
> -		for (i = 0; i < serv->sv_nrpools && i < n; i++)
> +		for (i = 0; i < svc_serv_nrpools(serv) && i < n; i++)
>  			nthreads[i] = serv->sv_pools[i].sp_nrthrmax;
>  	return 0;
>  }
> @@ -699,8 +699,8 @@ int nfsd_set_nrthreads(int n, int *nthreads, struct net *net)
>  	if (n == 1)
>  		return svc_set_num_threads(nn->nfsd_serv, nn->min_threads, nthreads[0]);
>  
> -	if (n > nn->nfsd_serv->sv_nrpools)
> -		n = nn->nfsd_serv->sv_nrpools;
> +	if (n > svc_serv_nrpools(nn->nfsd_serv))
> +		n = svc_serv_nrpools(nn->nfsd_serv);
>  
>  	/* enforce a global maximum number of threads */
>  	tot = 0;
> @@ -731,7 +731,7 @@ int nfsd_set_nrthreads(int n, int *nthreads, struct net *net)
>  	}
>  
>  	/* Anything undefined in array is considered to be 0 */
> -	for (i = n; i < nn->nfsd_serv->sv_nrpools; ++i) {
> +	for (i = n; i < svc_serv_nrpools(nn->nfsd_serv); ++i) {
>  		err = svc_set_pool_threads(nn->nfsd_serv,
>  					   &nn->nfsd_serv->sv_pools[i],
>  					   0, 0);
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index 3a0152d926fb..3c885ab6ad41 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -85,7 +85,6 @@ struct svc_serv {
>  
>  	char *			sv_name;	/* service name */
>  
> -	unsigned int		sv_nrpools;	/* number of thread pools */
>  	bool			sv_is_pooled;	/* is this a pooled service? */
>  	struct svc_pool *	sv_pools;	/* array of thread pools */
>  	int			(*sv_threadfn)(void *data);
> @@ -480,6 +479,7 @@ void		   svc_wake_up(struct svc_serv *);
>  void		   svc_reserve(struct svc_rqst *rqstp, int space);
>  void		   svc_pool_wake_idle_thread(struct svc_pool *pool);
>  struct svc_pool   *svc_pool_for_cpu(struct svc_serv *serv);
> +unsigned int	   svc_serv_nrpools(const struct svc_serv *serv);
>  char *		   svc_print_addr(struct svc_rqst *, char *, size_t);
>  const char *	   svc_proc_name(const struct svc_rqst *rqstp);
>  int		   svc_encode_result_payload(struct svc_rqst *rqstp,
> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> index ece69cb0138a..800514a14f17 100644
> --- a/net/sunrpc/svc.c
> +++ b/net/sunrpc/svc.c
> @@ -224,7 +224,7 @@ svc_pool_map_set_cpumask(struct task_struct *task, unsigned int pidx)
>  	unsigned int node = m->pool_to[pidx];
>  
>  	/*
> -	 * The caller checks for sv_nrpools > 1, which
> +	 * The caller checks for more than one pool, which
>  	 * implies that we've been initialized.
>  	 */
>  	WARN_ON_ONCE(m->count == 0);
> @@ -234,6 +234,24 @@ svc_pool_map_set_cpumask(struct task_struct *task, unsigned int pidx)
>  	set_cpus_allowed_ptr(task, cpumask_of_node(node));
>  }
>  
> +/**
> + * svc_serv_nrpools - number of thread pools backing a service
> + * @serv: An RPC service
> + *
> + * Pooled services all share the global svc_pool_map, so their pool count
> + * is svc_pool_map.npools. Unpooled services have a single pool. Reading
> + * npools without svc_pool_map_mutex is safe: a pooled service holds a map
> + * reference for its whole lifetime, so npools is stable once set.
> + *
> + * Return value:
> + *   The number of pools in @serv
> + */
> +unsigned int svc_serv_nrpools(const struct svc_serv *serv)
> +{
> +	return serv->sv_is_pooled ? svc_pool_map.npools : 1;
> +}
> +EXPORT_SYMBOL_GPL(svc_serv_nrpools);

I would make this a static-inline.

> +
>  /**
>   * svc_pool_for_cpu - Select pool to run a thread on this cpu
>   * @serv: An RPC service
> @@ -247,12 +265,15 @@ svc_pool_map_set_cpumask(struct task_struct *task, unsigned int pidx)
>  struct svc_pool *svc_pool_for_cpu(struct svc_serv *serv)
>  {
>  	struct svc_pool_map *m = &svc_pool_map;
> +	unsigned int nrpools = svc_serv_nrpools(serv);
>  	unsigned int pidx, i;
>  
> -	if (serv->sv_nrpools <= 1)
> +	if (nrpools <= 1)
>  		return serv->sv_pools;
>  
> -	pidx = m->to_pool[cpu_to_node(raw_smp_processor_id())] % serv->sv_nrpools;
> +	pidx = m->to_pool[cpu_to_node(raw_smp_processor_id())];
> +	if (pidx >= nrpools)
> +		pidx = 0;

The values stored in svc_pool_map.to_pool are all less than
svc_pool_map.npools.
So that if() condition cannot be true. 

But those two things don't reduce the correctness of the patch.

Reviewed-by: NeilBrown <neil@xxxxxxxxxx>

Thanks for doing this.
NeilBrown


>  
>  	/*
>  	 * It's possible to have a pool with no threads. Userland can just set
> @@ -265,7 +286,7 @@ struct svc_pool *svc_pool_for_cpu(struct svc_serv *serv)
>  	 * populated pool, trading NUMA locality for a guarantee that the
>  	 * transport is serviced.
>  	 */
> -	for (i = 0; i < serv->sv_nrpools; i++) {
> +	for (i = 0; i < nrpools; i++) {
>  		struct svc_pool *pool = &serv->sv_pools[pidx];
>  
>  		/* This is set under the sp_mutex and rarely ever changes. A
> @@ -274,7 +295,7 @@ struct svc_pool *svc_pool_for_cpu(struct svc_serv *serv)
>  		if (data_race(pool->sp_nrthreads))
>  			return pool;
>  
> -		if (++pidx >= serv->sv_nrpools)
> +		if (++pidx >= nrpools)
>  			pidx = 0;
>  	}
>  
> @@ -414,15 +435,13 @@ __svc_create(struct svc_program *prog, int nprogs, struct svc_stat *stats,
>  
>  	__svc_init_bc(serv);
>  
> -	serv->sv_nrpools = npools;
> -	serv->sv_pools =
> -		kzalloc_objs(struct svc_pool, serv->sv_nrpools);
> +	serv->sv_pools = kzalloc_objs(struct svc_pool, npools);
>  	if (!serv->sv_pools) {
>  		kfree(serv);
>  		return NULL;
>  	}
>  
> -	for (i = 0; i < serv->sv_nrpools; i++) {
> +	for (i = 0; i < npools; i++) {
>  		struct svc_pool *pool = &serv->sv_pools[i];
>  
>  		dprintk("svc: initialising pool %u for %s\n",
> @@ -520,7 +539,7 @@ svc_destroy(struct svc_serv **servp)
>  
>  	cache_clean_deferred(serv);
>  
> -	for (i = 0; i < serv->sv_nrpools; i++) {
> +	for (i = 0; i < svc_serv_nrpools(serv); i++) {
>  		struct svc_pool *pool = &serv->sv_pools[i];
>  
>  		svc_pool_destroy_counters(pool);
> @@ -732,7 +751,7 @@ int svc_new_thread(struct svc_serv *serv, struct svc_pool *pool)
>  	}
>  
>  	rqstp->rq_task = task;
> -	if (serv->sv_nrpools > 1)
> +	if (svc_serv_nrpools(serv) > 1)
>  		svc_pool_map_set_cpumask(task, pool->sp_id);
>  
>  	svc_sock_update_bufs(serv);
> @@ -858,8 +877,9 @@ int
>  svc_set_num_threads(struct svc_serv *serv, unsigned int min_threads,
>  		    unsigned int nrservs)
>  {
> -	unsigned int base = nrservs / serv->sv_nrpools;
> -	unsigned int remain = nrservs % serv->sv_nrpools;
> +	unsigned int nrpools = svc_serv_nrpools(serv);
> +	unsigned int base = nrservs / nrpools;
> +	unsigned int remain = nrservs % nrpools;
>  	int i, err = 0;
>  
>  	/*
> @@ -870,9 +890,9 @@ svc_set_num_threads(struct svc_serv *serv, unsigned int min_threads,
>  	 * @nrservs.
>  	 */
>  	if (base == 0 && nrservs != 0)
> -		remain = serv->sv_nrpools;
> +		remain = nrpools;
>  
> -	for (i = 0; i < serv->sv_nrpools; ++i) {
> +	for (i = 0; i < nrpools; ++i) {
>  		struct svc_pool *pool = &serv->sv_pools[i];
>  		int threads = base;
>  
> @@ -906,7 +926,7 @@ unsigned int svc_serv_maxthreads(const struct svc_serv *serv)
>  {
>  	unsigned int i, max = 0;
>  
> -	for (i = 0; i < serv->sv_nrpools; i++)
> +	for (i = 0; i < svc_serv_nrpools(serv); i++)
>  		max += data_race(serv->sv_pools[i].sp_nrthrmax);
>  	return max;
>  }
> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> index 63d1002e63e7..40040af588fb 100644
> --- a/net/sunrpc/svc_xprt.c
> +++ b/net/sunrpc/svc_xprt.c
> @@ -1188,7 +1188,7 @@ static void svc_clean_up_xprts(struct svc_serv *serv, struct net *net)
>  	struct svc_xprt *xprt;
>  	int i;
>  
> -	for (i = 0; i < serv->sv_nrpools; i++) {
> +	for (i = 0; i < svc_serv_nrpools(serv); i++) {
>  		struct svc_pool *pool = &serv->sv_pools[i];
>  		struct llist_node *q, **t1, *t2;
>  
> @@ -1517,7 +1517,7 @@ static void *svc_pool_stats_start(struct seq_file *m, loff_t *pos)
>  		return SEQ_START_TOKEN;
>  	if (!si->serv)
>  		return NULL;
> -	return pidx > si->serv->sv_nrpools ? NULL
> +	return pidx > svc_serv_nrpools(si->serv) ? NULL
>  		: &si->serv->sv_pools[pidx - 1];
>  }
>  
> @@ -1535,7 +1535,7 @@ static void *svc_pool_stats_next(struct seq_file *m, void *p, loff_t *pos)
>  		pool = &serv->sv_pools[0];
>  	} else {
>  		unsigned int pidx = (pool - &serv->sv_pools[0]);
> -		if (pidx < serv->sv_nrpools-1)
> +		if (pidx < svc_serv_nrpools(serv) - 1)
>  			pool = &serv->sv_pools[pidx+1];
>  		else
>  			pool = NULL;
> 
> -- 
> 2.55.0
> 
>