Re: [PATCH] rcu-tasks: Remove smp_mb() in rcu_spawn_tasks_kthread_generic()

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

> 
> On Fri, Jul 10, 2026 at 07:50:28PM +0800, Zqiang wrote:
> 
> > 
> > The smp_mb() after kthread_run() in rcu_spawn_tasks_kthread_generic()
> >  from early 'commit eacd6f04a133 ("rcu-tasks: Move Tasks RCU to its
> >  own file")' and 'commit 84a8f446ffd7 ("rcu: Defer rcu_tasks_kthread()
> >  creation till first call_rcu_tasks()")', the pairing as follows:
> >  
> >  rcu_spawn_tasks_kthread()
> >  ->t = kthread_run(rcu_tasks_kthread, ...);
> >  ->smp_mb(); /* Ensure others see full kthread. */
> >  ->WRITE_ONCE(rcu_tasks_kthread_ptr, t);
> >  
> >  call_rcu_tasks()
> >  ->if (READ_ONCE(rcu_tasks_kthread_ptr))
> >  ->wake_up(&rcu_tasks_cbs_wq)
> >  ->try_to_wake_up()
> >  lock pi_lock
> >  ->smp_mb__after_spinlock()
> >  //see full kthread
> >  
> >  Currently, the 'commit d119357d0743 ("rcu-tasks: Treat only synchronous
> >  grace periods urgently")' moved kthread_ptr assignment into the
> >  rcu_tasks_kthread(), the following pairings are sufficient:
> >  
> >  The rq->lock/unlock from wake_up_process() in kthread_run() and
> >  __schedule() provides a memory barrier when the kthread is first
> >  scheduled, this ensures the kthread itself observes all of the
> >  kthread's initialization.
> >  
> >  The kthread's smp_store_release(&rtp->kthread_ptr, ...) in
> >  rcu_tasks_kthread() and smp_load_acquire(&rtp->kthread_ptr) in
> >  call_rcu_tasks_generic() compose release/acquire pairing, the
> >  cumulativity of smp_store_release() propagates visibility of the
> >  kthread's initialization through the scheduler chain.
> >  
> >  This commit therefore remove smp_mb() in rcu_spawn_tasks_kthread_generic().
> >  
> >  Signed-off-by: Zqiang <qiang.zhang@linux.dev>
> > 
> Very good, a removal of an smp_mb() that also simplifies analysis!
> 
> One question...
> 
> > 
> > ---
> >  kernel/rcu/tasks.h | 2 --
> >  1 file changed, 2 deletions(-)
> >  
> >  diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
> >  index 5a8db89135eb..18ab335665e3 100644
> >  --- a/kernel/rcu/tasks.h
> >  +++ b/kernel/rcu/tasks.h
> >  @@ -395,7 +395,6 @@ static void call_rcu_tasks_generic(struct rcu_head *rhp, rcu_callback_t func,
> >  raw_spin_unlock_irqrestore(&rtp->cbs_gbl_lock, flags);
> >  }
> >  rcu_read_unlock();
> >  - /* We can't create the thread unless interrupts are enabled. */
> > 
> Why remove this comment? People reading this code might wonder why we
> are not just invoking rcu_spawn_tasks_kthread_generic() to create the
> task here.
> 
> Now, if you say that the current comment is confusing because if that
> kthread already exists, we also defer the wakeup, fair enough. But in
> that case, could you please propose a replacement comment, perhaps as
> a separate patch?

How about the following comment:

/* Use irq_work do deferred wakeup to avoid potential lockdep splat */

If directly use rcuwait_wake_up() to wakup, the lockdep will possible spalt，
(for example we hook trace_sched_wakeup() tracepoint, and in our hook
func inovke call_rcu_tasks())

Thanks
Zqiang


> 
>  Thanx, Paul
> 
> > 
> > if (needwake && READ_ONCE(rtp->kthread_ptr))
> >  irq_work_queue(&rtpcp->rtp_irq_work);
> >  }
> >  @@ -681,7 +680,6 @@ static void __init rcu_spawn_tasks_kthread_generic(struct rcu_tasks *rtp)
> >  t = kthread_run(rcu_tasks_kthread, rtp, "%s_kthread", rtp->kname);
> >  if (WARN_ONCE(IS_ERR(t), "%s: Could not start %s grace-period kthread, OOM is now expected behavior\n", __func__, rtp->name))
> >  return;
> >  - smp_mb(); /* Ensure others see full kthread. */
> >  }
> >  
> >  #ifndef CONFIG_TINY_RCU
> >  -- 
> >  2.17.1
> >
>