[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CALm+0cUuze+CWJ2YS3TGeyayxBqO-pZbK=dq0E_7rfT+Od3KHQ@mail.gmail.com>
Date: Tue, 6 May 2025 14:25:55 +0800
From: Z qiang <qiang.zhang1211@...il.com>
To: Joel Fernandes <joelagnelf@...dia.com>
Cc: Frederic Weisbecker <frederic@...nel.org>, paulmck@...nel.org, neeraj.upadhyay@...nel.org,
joel@...lfernandes.org, urezki@...il.com, boqun.feng@...il.com,
rcu@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] rcu/nocb: Add Safe checks for access offloaded rdp
>
>
>
> On 4/30/2025 12:14 PM, Joel Fernandes wrote:
> >
> >
> > 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.zhang1211@...il.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!
> >
>
> Frederic, there are a couple of ways we can move forward hear. Does the
> softirq_count() approach sound good to you? If yes, I can fixup the patch myself.
Hello, Joel
If you send a patch to fix it, I'd be happy, you can add me as the
Reported-by ;)
Thanks
Zqiang
>
> I am also Ok at this point to take it in for 6.16, though I've also stored it in
> my rcu/dev branch for Neeraj's 6.17 PR, just in case :)
>
> - Joel
>
>
Powered by blists - more mailing lists