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

["Paul E. McKenney" <paulmck@kernel.org>]

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?

							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
>