Re: [PATCH] rcutorture: Use __set_current_state() set task state in rcu_nocb_toggle()

["Zqiang" <qiang.zhang@linux.dev>]

> 
> On Thu, Jul 02, 2026 at 06:11:25PM +0800, Zqiang wrote:
> 
> > 
> > In the rcu_nocb_toggle(), the schedule_hrtimeout() is called after the
> >  state assignment with no condition check. the release-acquire pair from
> >  raw_spin_unlock/lock(&cpu_base->lock), guarantee that task->__state is
> >  visible to the hrtimer callback:
> >  
> >  CPU0 CPU1
> >  __set_current_state(TASK_INTERRUPTIBLE)
> >  ->WRITE_ONCE(task->__state, TASK_INTERRUPTIBLE)
> >  schedule_hrtimeout
> >  ->hrtimer_sleeper_start_expires()
> >  ->raw_spin_lock_irqsave(&cpu_base->lock)
> >  ....
> >  ->raw_spin_unlock_irqrestore(&cpu_base->lock)
> >  
> >  hard-irq:
> >  raw_spin_lock_irqsave(&cpu_base->lock)
> >  __hrtimer_run_queues
> >  ->__run_hrtimer
> >  ->raw_spin_unlock_irqrestore(&cpu_base->lock)
> >  ->fn(timer)
> >  ->hrtimer_wakeup
> >  ->wake_up_process
> >  ->try_to_wake_up
> >  ->READ task->__state
> >  
> >  This commit therefore use the __set_current_state() to replace the
> >  set_current_state() in rcu_nocb_toggle().
> >  
> >  Signed-off-by: Zqiang <qiang.zhang@linux.dev>
> > 
> This looks correct, and either drops an smp_mb() or converts an
> xchg() to a WRITE_ONCE(), which does decrease overhead. Except that
> rcu_nocb_toggle() is invoked very infrequently and has high overhead
> that I would expect to lose this overhead decrease in the noise. And it
> forces those reading the code to go figure out what is different between
> set_current_state() and __set_current_state().
> 
> So I am not convinced to take this patch. But am I missing something here?

Nothing was missed, only from memory ordering perspective,
using set_current_state() is unnecessary. 
furthermore, in RCU tasks, we used the following code:

__set_current_state(TASK_IDLE);  // in this we use __set_current_state()
schedule_hrtimeout_range(&exp, jiffies_to_nsecs(HZ / 2), HRTIMER_MODE_REL_HARD);

Or perhaps we could also add a comment something like this(kernel/time/sleep_timeout.c):

/*
 * __set_current_state() can be used in schedule_timeout_*() functions, because
 * schedule_timeout() calls schedule() unconditionally.
 */


Thanks
Zqiang


> 
>  Thanx, Paul
> 
> > 
> > ---
> >  kernel/rcu/rcutorture.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >  
> >  diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
> >  index b1bab59efde5..a0e6901e0f90 100644
> >  --- a/kernel/rcu/rcutorture.c
> >  +++ b/kernel/rcu/rcutorture.c
> >  @@ -2947,7 +2947,7 @@ static int rcu_nocb_toggle(void *arg)
> >  atomic_long_inc(&n_nocb_deoffload);
> >  }
> >  toggle_delay = torture_random(&rand) % toggle_fuzz + toggle_interval;
> >  - set_current_state(TASK_INTERRUPTIBLE);
> >  + __set_current_state(TASK_INTERRUPTIBLE);
> >  schedule_hrtimeout(&toggle_delay, HRTIMER_MODE_REL);
> >  if (stutter_wait("rcu_nocb_toggle"))
> >  sched_set_normal(current, oldnice);
> >  -- 
> >  2.17.1
> >
>