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

[Jeff Layton <jlayton@kernel.org>]

On Thu, 2026-07-09 at 08:51 +1000, NeilBrown wrote:
> On Wed, 08 Jul 2026, Jeff Layton wrote:
> > On Wed, 2026-07-08 at 15:33 +1000, NeilBrown wrote:
> > > On Tue, 07 Jul 2026, Jeff Layton wrote:
> > > > On Tue, 2026-07-07 at 08:50 +1000, NeilBrown wrote:
> > > > > 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@ownmail.net>
> > > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > > > ---
> > > > > >  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.
> > > > > 
> > > > 
> > > > That would mean that we would have to export svc_pool_map, which is
> > > > currently private to svc.c.
> > > 
> > > Why is exporting svc_pool_map more problematic than exporting svc_serv_nrpools?
> > > I think it would be good for at least the code in svc_pool_for_cpu() to
> > > inline the function.  Maybe it already does?  Or maybe marking it
> > > "inline" but still exporting it would work.
> > > 
> > > Thanks,
> > > NeilBrown
> > > 
> > 
> > Doing what you suggest would increase the interface "surface" between
> > sunrpc.ko and nfsd.ko.
> > 
> > Currently struct svc_pool_map is only defined in net/sunrpc/svc.c, so
> > we'd not only need to export the symbol, but also we'd have to make
> > that definition public.
> > 
> > Why expose so many internal details to other modules when all we need
> > is this one accessor? I think keeping this interface small is
> > preferable.
> 
> In general, yes.  But it is not without cost.  Sometimes the cost
> matters.
> Interface size has an abstract value.  Code size can have concrete
> value.  Comparing them is subjective I guess.
> I have no concrete evidence that there is a problem, so I'll stop
> objecting.
> 

Your point is certainly valid, and this _is_ a hot path. If we can
demonstrate that this matters, then I'd be happy to revisit the idea. I
don't have the cycles to do so at the moment though.

-- 
Jeff Layton <jlayton@kernel.org>