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

[Joel Fernandes <joelagnelf@nvidia.com>]

On 4/28/2025 6:59 AM, Z qiang wrote:
>>
>> Le Mon, Apr 28, 2025 at 05:54:03PM +0800, Zqiang a écrit :
>>> For Preempt-RT kernel, when enable CONFIG_PROVE_RCU Kconfig,
>>> disable local bh in rcuc kthreads will not affect preempt_count(),
>>> 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 rdp->rcu_cpu_kthread_task check for
>>> Preempt-RT kernels.
>>>
>>> Signed-off-by: Zqiang <qiang.zhang1211@gmail.com>
>>> ---
>>>  kernel/rcu/tree_plugin.h | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
>>> index 003e549f6514..fe728eded36e 100644
>>> --- a/kernel/rcu/tree_plugin.h
>>> +++ b/kernel/rcu/tree_plugin.h
>>> @@ -31,7 +31,9 @@ static bool rcu_rdp_is_offloaded(struct rcu_data *rdp)
>>>                 lockdep_is_held(&rcu_state.nocb_mutex) ||
>>>                 (!(IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible()) &&
>>>                  rdp == this_cpu_ptr(&rcu_data)) ||
>>> -               rcu_current_is_nocb_kthread(rdp)),
>>> +               rcu_current_is_nocb_kthread(rdp) ||
>>> +               (IS_ENABLED(CONFIG_PREEMPT_RT) &&
>>> +                current == rdp->rcu_cpu_kthread_task)),
>>
>> Isn't it safe also on !CONFIG_PREEMPT_RT ?
> 
> For !CONFIG_PREEMPT_RT and  in rcuc kthreads, it's also safe,
> but the following check will passed :
> 
> (!(IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible()) &&
>           rdp == this_cpu_ptr(&rcu_data))

I think the fact that it already passes for !PREEMPT_RT does not matter, because
it simplifies the code so drop the PREEMPT_RT check?

Or will softirq_count() not work? It appears to have special casing for
PREEMPT_RT's local_bh_disable():

(   ( !(IS_ENABLED(CONFIG_PREEMPT_COUNT) && preemptible()) || softirq_count() )
   && rdp == this_cpu_ptr(&rcu_data))  )

thanks,

- Joel