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

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

> 
> On Tue, Jul 07, 2026 at 01:15:10PM +0000, Zqiang wrote:
> 
> > 
> > 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.
> >  */
> > 
> Would it make sense for a variant of schedule_hrtimeout_range() to take an
> TASK_* argument and then use __set_current_state() internally? That would
> get you the performance increase across all calls, but avoid bothering
> the typical user with the difference between __set_current_state() vs.
> set_current_state().

This is a good idea.

Thanks
Zqiang

> 
>  Thanx, Paul
> 
> > 
> > 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
> >  >
> >
>