Re: [PATCH v4 1/4] sunrpc: route to a populated pool in svc_pool_for_cpu()

[NeilBrown]

On Thu, 02 Jul 2026, Jeff Layton wrote:
> On Thu, 2026-07-02 at 08:17 -0400, Jeff Layton wrote:
> > On Thu, 2026-07-02 at 08:13 +1000, NeilBrown wrote:
> > > On Thu, 02 Jul 2026, Jeff Layton wrote:
> > > > svc_set_num_threads() spreads the requested threads evenly across the
> > > > service's pools (base = nrservs / sv_nrpools).  When a service runs
> > > > fewer threads than it has pools -- e.g. an nfsd configured with fewer
> > > > threads than the host has NUMA nodes while running in "pernode" or
> > > > "percpu" mode -- the trailing pools are left with no threads at all.
> > > > 
> > > > svc_xprt_enqueue() selects a pool from the CPU servicing the transport,
> > > > queues the transport on that pool's sp_xprts, and only wakes a thread
> > > > from the same pool.  Each thread services exclusively its own pool, so a
> > > > transport that lands on a threadless pool is enqueued on sp_xprts and
> > > > never picked up: the connection hangs indefinitely.
> > > > 
> > > > Have svc_pool_for_cpu() skip pools that currently have no threads,
> > > > falling back to the next populated pool.  This trades NUMA locality for
> > > > a guarantee that the work is actually serviced.  sp_nrthreads is only
> > > > updated under the service mutex; the lockless read here is a best-effort
> > > > routing hint, so annotate it with data_race().
> > > > 
> > > > Fixes: 0f0257eaa5d2 ("svc: Move the xprt independent code to the svc_xprt.c file")
> > > 
> > > Why that commit?  Did this ever work correctly?
> > > It seems more likely that 
> > > Fixes: 3262c816a3d7 ("[PATCH] knfsd: split svc_serv into pools")
> > > is appropriate.
> > > 
> > 
> > Indeed. Good catch.
> > 
> 
> I had the LLM run this down. We're both wrong.
> 
> It briefly worked properly after 3262c816a3d7 ("split svc_serv into
> pools"), but then was broken in the same series in commit bfd241600a3b
> ("knfsd: make rpc threads pools numa aware"). So I think we want:
> 
> Fixes: bfd241600a3b ("knfsd: make rpc threads pools numa aware")

That is certainly credible - thanks.

> 
> 
> > > > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > > > ---
> > > >  net/sunrpc/svc.c | 26 +++++++++++++++++++++++++-
> > > >  1 file changed, 25 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> > > > index dd80a2eaaa74..82fb7faf563f 100644
> > > > --- a/net/sunrpc/svc.c
> > > > +++ b/net/sunrpc/svc.c
> > > > @@ -402,6 +402,7 @@ struct svc_pool *svc_pool_for_cpu(struct svc_serv *serv)
> > > >  	struct svc_pool_map *m = &svc_pool_map;
> > > >  	int cpu = raw_smp_processor_id();
> > > >  	unsigned int pidx = 0;
> > > > +	unsigned int i;
> > > >  
> > > >  	if (serv->sv_nrpools <= 1)
> > > >  		return serv->sv_pools;
> > > > @@ -414,8 +415,31 @@ struct svc_pool *svc_pool_for_cpu(struct svc_serv *serv)
> > > >  		pidx = m->to_pool[cpu_to_node(cpu)];
> > > >  		break;
> > > >  	}
> > > > +	pidx %= serv->sv_nrpools;
> > > > +
> > > > +	/*
> > > > +	 * Threads are spread evenly across the pools, but when there are
> > > > +	 * fewer threads than pools some pools can end up with none. A
> > > > +	 * transport enqueued on a threadless pool would never be picked
> > > > +	 * up, since each thread only services its own pool. Fall back to
> > > > +	 * the next populated pool, trading NUMA locality for a guarantee
> > > > +	 * that the transport is serviced.
> > > > +	 */
> > > > +	for (i = 0; i < serv->sv_nrpools; i++) {
> > > > +		struct svc_pool *pool = &serv->sv_pools[pidx];
> > > > +
> > > > +		/* This is set under the sp_mutex and rarely ever changes. A
> > > > +		 * data race here is harmless.
> > > > +		 */
> > > > +		if (data_race(pool->sp_nrthreads))
> > > > +			return pool;
> > > > +
> > > > +		if (++pidx >= serv->sv_nrpools)
> > > > +			pidx = 0;
> > > > +	}
> > > >  
> > > > -	return &serv->sv_pools[pidx % serv->sv_nrpools];
> > > > +	/* No pool has any threads; nothing can service the transport. */
> > > 
> > > Would a WARN_ON_ONCE() be appropriate here?
> > > 
> > 
> > Maybe a pr_notice_once()? A stack trace isn't particularly helpful
> > here, but it would be good to let someone know that this isn't
> > optimally configured. I'll add one for v5.
> > 
> > 
> 
> This is probably not feasible, as there are cases where we legitimately
> queue the call to a pool with no threads.
> 
> At startup, we create listeners and then threads only get spun up
> later. If we get a RPC on the listener port during that window, the
> message would fire. There's a similar window on shutdown.
> 
> It might not hurt to add a tracepoint there though.

and a comment explaining when this might happen?

The start-up window makes sense - as long as we start a thread on each
pool it should be safe.  But if we don't (if the admin request zero on
some pools) we can still get stuck.  Is there something simple we can do
about that, or does the admin get to keep both halves?

And on shutdown the threads stop before the sockets are cleaned up - but
they do get cleaned up, so all good.

Thanks,
NeilBrown


> 
> > > I think this is a sensible defensive-programming approach.
> > > 
> > > Reviewed-by: NeilBrown <neil@xxxxxxxxxx>
> > > 
> > 
> > Thanks!
> > 
> > > > +	return &serv->sv_pools[pidx];
> > > >  }
> > > >  
> > > >  static int svc_rpcb_setup(struct svc_serv *serv, struct net *net)
> > > > 
> > > > -- 
> > > > 2.54.0
> > > > 
> > > > 
> 
> -- 
> Jeff Layton <jlayton@xxxxxxxxxx>
>