On 4/30/2025 10:57 AM, Z qiang wrote:
>>
>>
>>
>> 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.zhang1...@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)) )
>
> Thank you for Joel's reply, I also willing to accept such
> modifications and resend :) .
Thanks, I am Ok with either approach whichever you and Frederic together decide.
I can then pull this in for the v6.16 merge window once you resend, thanks!
- Joel
