Re: [PATCH] sunrpc: fix lost wakeups when stopping service threads

["Chuck Lever" <cel@kernel.org>]

On Fri, Oct 30, 2026, at 6:45 PM, NeilBrown wrote:
> On Sat, 31 Oct 2026, Kal Cutter Conley wrote:
>> Stopping NFS can leave rpc.nfsd 0 in an uninterruptible wait with
>> this stack:
>> 
>>   svc_stop_kthreads [sunrpc]
>>   svc_set_num_threads [sunrpc]
>>   nfsd_svc [nfsd]
>>   write_threads [nfsd]
>>   nfsctl_transaction_write [nfsd]
>> 
>> SP_NEED_VICTIM and SP_VICTIM_REMAINS remain set while all remaining
>> nfsd threads sleep in svc_recv() and none has RQ_VICTIM set.
>> 
>> The stop request is published with unordered set_bit() operations.
>> svc_pool_wake_idle_thread() checks the idle list and task state before
>> calling wake_up_process(). If it sees an empty list or TASK_RUNNING,
>> it skips the wakeup and its memory barrier. The requester can therefore
>> miss a sleeping worker while that worker misses the stop request.
>> 
>> Order work publication before both lockless checks, even when no
>> wakeup occurs. Pair with set_current_state() and llist_add() in
>> svc_thread_wait_for_work(), so the worker observes the pending work or
>> the requester observes the idle worker and wakes it.
>> 
>> Fixes: 60749cbe3d8a ("sunrpc: change sp_nrthreads from atomic_t to unsigned int.")
>> Signed-off-by: Kal Cutter Conley <kalcutterc@nvidia.com>
>> ---
>>  net/sunrpc/svc.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>> 
>> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
>> index af8d87c1de83..f3cc63acc5e1 100644
>> --- a/net/sunrpc/svc.c
>> +++ b/net/sunrpc/svc.c
>> @@ -29,6 +29,8 @@
>>  #include <linux/sunrpc/clnt.h>
>>  #include <linux/sunrpc/bc_xprt.h>
>>  
>> +#include <asm/barrier.h>
>> +
>>  #include <trace/events/sunrpc.h>
>>  
>>  #include "fail.h"
>> @@ -693,6 +695,13 @@ void svc_pool_wake_idle_thread(struct svc_pool *pool)
>>  	struct svc_rqst	*rqstp;
>>  	struct llist_node *ln;
>>  
>> +	/*
>> +	 * Order work publication before the idle-list and task-state checks,
>> +	 * even when wake_up_process() is skipped. Pairs with the barriers in
>> +	 * set_current_state() and llist_add() in svc_thread_wait_for_work().
>> +	 */
>> +	smp_mb();
>> +
>
> I don't like this.  Stand-alone barriers are, I think, harder to read
> than barriers combined with operations, like smp_load_acquire() or
> similar.  And this one seems quite detached from anything.
>
> If I understand correctly, the possible race is between
> svc_thread_wait_for_work which includes:
>
> 		llist_add(&rqstp->rq_idle, &pool->sp_idle_threads);
> 		if (likely(svc_thread_should_sleep(rqstp)))
>
> and svc_stop_kthread which includes:
>
> 		set_bit(SP_NEED_VICTIM, &pool->sp_flags);
> 		svc_pool_wake_idle_thread(pool);
>
> The first sets ->sp_idle_threads.first and then checks SP_NEED_VICTIM
> and the second sets SP_NEED_VICTIM and then checks
> ->sp_idle_threads.first.
>
> So we need barriers to ensure the values are seen is the expected order.
> Have I missed anything?
>
> llist_add() always calls try_cmpxchg() which is fully ordered, so no
> extra barriers are needed there.
>
> There is no barrier between the set_bit)( and the READ_ONCE() in
> svc_pool_wake_idle_thread(), so you added one.  I agree that is correct
> and solves the problem.
> But it is only this call to svc_pool_wake_idle_thread() that needs a
> barrier, not all calls.  So I would prefer the barrier went in
> svc_stop_kthreads().   Do you have a reason for not including it there?
>
> We do need a full memory barrier - a "release" on the set_bit and an
> "acquire" on the test of ->sp_idle_threads.first
>
> I would be happy with smp_load_acquire(->sp_idle_theads_first) in
> svc_pool_wake_idle_thread().  I'm not sure it is needed though.
>
> I would like set_bit_release(SP_NEED_VICTIM) in svc_stop_kthreads()
> but we don't have that.
> We could do
>    atomic_long_fetch_or_release(BIT(SP_NEED_VICTIM), 
> (atomic_long_t*)&pool->sp_flags)
> but that is ugly.
>
> So I think the best solution is
>
> 		set_bit(SP_NEED_VICTIM, &pool->sp_flags);
> 		/* Order SP_NEED_VICTIM before testing sp_idle_threads,
>                  * matching svc_thread_wait_for_work() which orders
>                  * setting sp_idle_threads before testing the bit
> 		 */
> +               smp_mb__after_atomic();
> 		svc_pool_wake_idle_thread(pool);
>
> We probably need the same in svc_wake_up() after the set_bit().

Kal's patch as written adds an mfence-class instruction to every transport
enqueue (a common case) for no ordering benefit. So let's go with Neil's
proposed solution. Kal, can you send a v2?


-- 
Chuck Lever (Come to NFS bake-a-thon! https://nfsv4bat.org)