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 <[email protected]>
> > >
> > 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().
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
> > >
> >
