Re: [PATCH v4 3/4] sunrpc: guarantee a thread per CPU-bearing node when auto-distributing

On Thu, 2026-07-02 at 08:37 +1000, NeilBrown wrote:
> On Thu, 02 Jul 2026, Jeff Layton wrote:
> > svc_set_num_threads() spreads the requested thread count evenly across
> > the service's pools. In pernode mode each pool maps to a NUMA node, and
> > svc_pool_for_cpu() steers an incoming transport to the pool for the node
> > it arrived on. When fewer threads than pools are requested, even
> > distribution leaves some nodes' pools empty, and a transport steered to
> > an empty pool has no thread to service it.
> > 
> > Floor each CPU-bearing node's pool at one thread when auto-distributing a
> > non-zero count, so no such pool is left empty. The resulting total may
> > exceed the requested count. This only affects the auto-distribute path
> > (a single-value array, i.e. svc_set_num_threads()); callers that set
> > per-pool counts explicitly via svc_set_pool_threads() are unchanged and
> > may still set a pool to zero. Nodes without CPUs (e.g. memory-only nodes)
> > get no thread, as nothing is steered to them.
> > 
> > Assisted-by: Claude:claude-opus-4-8
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> >  net/sunrpc/svc.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> > index c9fba7edaace..ae93a6f51087 100644
> > --- a/net/sunrpc/svc.c
> > +++ b/net/sunrpc/svc.c
> > @@ -837,6 +837,12 @@ EXPORT_SYMBOL_GPL(svc_set_pool_threads);
> >   * are multiple pools then the new threads or victims will be distributed
> >   * evenly among them.
> >   *
> > + * When @nrservs is non-zero but smaller than the number of pools, even
> > + * distribution would leave some pools empty. Since each pool maps to a
> > + * NUMA node and only services transports steered to that node, every
> > + * pool whose node has CPUs is instead guaranteed at least one thread.
> > + * The resulting total may therefore exceed @nrservs.
> > + *
> >   * Caller must ensure mutual exclusion between this and server startup or
> >   * shutdown.
> >   *
> > @@ -861,6 +867,15 @@ svc_set_num_threads(struct svc_serv *serv, unsigned int min_threads,
> >  			--remain;
> >  		}
> >  
> > +		/*
> > +		 * Don't let a node's pool sit empty while threads are
> > +		 * being auto-distributed: a transport steered there would
> > +		 * have nothing to service it.
> > +		 */
> > +		if (threads == 0 && nrservs &&
> > +		    nr_cpus_node(svc_pool_map_get_node(pool->sp_id)))
> 
> svc_pool_map_init_pernode() uses for_each_node_with_cpus() so we can be
> certain that each node which has been allocated a pool will have at
> least 1 cpu.  Thus that last condition isn't needed.
> 
> I would probably address the problem outside the loop with
> 
> if (base == 0 && nrservs != 0)
>    /* We need at least one thread per pool for correct functionality */
>    remain = serv->sv_nrpools;
> 
> or similar.  But your version works too and this isn't
> performance-critical code.
> 
> Reviewed-by: NeilBrown <neil-+NVA1uvv1dVBDLzU/[email protected]>
> 
> 

I like your version better. I'll adopt it.

Thanks!
-- 
Jeff Layton <[email protected]>