Re: [PATCH v2] rcu/nocb: Add Safe checks for access offloaded rdp

[Joel Fernandes <joelagnelf@nvidia.com>]

On 5/7/2025 7:26 AM, Zqiang wrote:
> For built with CONFIG_PROVE_RCU=y and CONFIG_PREEMPT_RT=y kernels,
> Disable BH does not change the SOFTIRQ corresponding bits in
> preempt_count(), but change current->softirq_disable_cnt, this
> resulted in the following splat:
> 
> WARNING: suspicious RCU usage
> kernel/rcu/tree_plugin.h:36 Unsafe read of RCU_NOCB offloaded state!
> stack backtrace:
> CPU: 0 UID: 0 PID: 22 Comm: rcuc/0
> Call Trace:
> [    0.407907]  <TASK>
> [    0.407910]  dump_stack_lvl+0xbb/0xd0
> [    0.407917]  dump_stack+0x14/0x20
> [    0.407920]  lockdep_rcu_suspicious+0x133/0x210
> [    0.407932]  rcu_rdp_is_offloaded+0x1c3/0x270
> [    0.407939]  rcu_core+0x471/0x900
> [    0.407942]  ? lockdep_hardirqs_on+0xd5/0x160
> [    0.407954]  rcu_cpu_kthread+0x25f/0x870
> [    0.407959]  ? __pfx_rcu_cpu_kthread+0x10/0x10
> [    0.407966]  smpboot_thread_fn+0x34c/0xa50
> [    0.407970]  ? trace_preempt_on+0x54/0x120
> [    0.407977]  ? __pfx_smpboot_thread_fn+0x10/0x10
> [    0.407982]  kthread+0x40e/0x840
> [    0.407990]  ? __pfx_kthread+0x10/0x10
> [    0.407994]  ? rt_spin_unlock+0x4e/0xb0
> [    0.407997]  ? rt_spin_unlock+0x4e/0xb0
> [    0.408000]  ? __pfx_kthread+0x10/0x10
> [    0.408006]  ? __pfx_kthread+0x10/0x10
> [    0.408011]  ret_from_fork+0x40/0x70
> [    0.408013]  ? __pfx_kthread+0x10/0x10
> [    0.408018]  ret_from_fork_asm+0x1a/0x30
> [    0.408042]  </TASK>
> 
> Currently, triggering an rdp offloaded state change need the
> corresponding rdp's CPU goes offline, and at this time the rcuc
> kthreads has already in parking state. this means the corresponding
> rcuc kthreads can safely read offloaded state of rdp while it's
> corresponding cpu is online.
> 
> This commit therefore add softirq_count() check for
> Preempt-RT kernels.
> 
> Suggested-by: Joel Fernandes <joelagnelf@nvidia.com>
> Signed-off-by: Zqiang <qiang.zhang1211@gmail.com>
> ---
>  kernel/rcu/tree_plugin.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> index 003e549f6514..a91b2322a0cd 100644
> --- a/kernel/rcu/tree_plugin.h
> +++ b/kernel/rcu/tree_plugin.h
> @@ -29,7 +29,7 @@ static bool rcu_rdp_is_offloaded(struct rcu_data *rdp)
>  		  (IS_ENABLED(CONFIG_HOTPLUG_CPU) && lockdep_is_cpus_held()) ||
>  		  lockdep_is_held(&rdp->nocb_lock) ||
>  		  lockdep_is_held(&rcu_state.nocb_mutex) ||
> -		  (!(IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible()) &&
> +		  ((!(IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible()) || softirq_count()) &&
>  		   rdp == this_cpu_ptr(&rcu_data)) ||
This looks good to me. Frederic told me he'll further review and give final
green signal. Then I'll pull this particular one.

One thing I was wondering -- it would be really nice if preemptible() itself
checked for softirq_count() by default. Or adding something like a
really_preemptible() which checks for both CONFIG_PREEMPT_COUNT and
softirq_count() along with preemptible().  I feel like this always comes back to
bite us in different ways, and not knowing atomicity complicates various code paths.

Maybe a summer holidays project? ;)

 - Joel