* Re: [BUG BISECTED] Missing RCU reader in perf_event_setup_cpumask()
2024-09-13 10:47 ` Peter Zijlstra
@ 2024-09-13 12:09 ` Paul E. McKenney
2024-09-13 15:51 ` Liang, Kan
1 sibling, 0 replies; 4+ messages in thread
From: Paul E. McKenney @ 2024-09-13 12:09 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, linux-perf-users, kan.liang, mingo, acme, namhyung,
mark.rutland, alexander.shishkin, jolsa, irogers, adrian.hunter
On Fri, Sep 13, 2024 at 12:47:52PM +0200, Peter Zijlstra wrote:
> On Fri, Sep 13, 2024 at 01:00:44AM -0700, Paul E. McKenney wrote:
> > Hello!
> >
> > On next-20240912 running rcutorture scenario TREE05, I see this
> > deterministically:
> >
> > [ 32.603233] =============================
> > [ 32.604594] WARNING: suspicious RCU usage
> > [ 32.605928] 6.11.0-rc5-00040-g4ba4f1afb6a9 #55238 Not tainted
> > [ 32.607812] -----------------------------
> > [ 32.609140] kernel/events/core.c:13946 RCU-list traversed in non-reader section!!
> > [ 32.611595]
> > [ 32.611595] other info that might help us debug this:
> > [ 32.611595]
> > [ 32.614247]
> > [ 32.614247] rcu_scheduler_active = 2, debug_locks = 1
> > [ 32.616392] 3 locks held by cpuhp/4/35:
> > [ 32.617687] #0: ffffffffb666a650 (cpu_hotplug_lock){++++}-{0:0}, at: cpuhp_thread_fun+0x4e/0x200
> > [ 32.620563] #1: ffffffffb666cd20 (cpuhp_state-down){+.+.}-{0:0}, at: cpuhp_thread_fun+0x4e/0x200
> > [ 32.623412] #2: ffffffffb677c288 (pmus_lock){+.+.}-{3:3}, at: perf_event_exit_cpu_context+0x32/0x2f0
> > [ 32.626399]
> > [ 32.626399] stack backtrace:
> > [ 32.627848] CPU: 4 UID: 0 PID: 35 Comm: cpuhp/4 Not tainted 6.11.0-rc5-00040-g4ba4f1afb6a9 #55238
> > [ 32.628832] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.14.0-0-g155821a1990b-prebuilt.qemu.org 04/01/2014
> > [ 32.628832] Call Trace:
> > [ 32.628832] <TASK>
> > [ 32.628832] dump_stack_lvl+0x83/0xa0
> > [ 32.628832] lockdep_rcu_suspicious+0x143/0x1a0
> > [ 32.628832] perf_event_exit_cpu_context+0x2e5/0x2f0
> > [ 32.628832] ? __pfx_perf_event_exit_cpu+0x10/0x10
> > [ 32.628832] perf_event_exit_cpu+0x9/0x10
> > [ 32.628832] cpuhp_invoke_callback+0x130/0x2a0
> > [ 32.628832] ? lock_release+0xc7/0x290
> > [ 32.628832] ? cpuhp_thread_fun+0x4e/0x200
> > [ 32.628832] cpuhp_thread_fun+0x183/0x200
> > [ 32.628832] smpboot_thread_fn+0xd8/0x1d0
> > [ 32.628832] ? __pfx_smpboot_thread_fn+0x10/0x10
> > [ 32.628832] kthread+0xd4/0x100
> > [ 32.628832] ? __pfx_kthread+0x10/0x10
> > [ 32.628832] ret_from_fork+0x2f/0x50
> > [ 32.628832] ? __pfx_kthread+0x10/0x10
> > [ 32.628832] ret_from_fork_asm+0x1a/0x30
> > [ 32.628832] </TASK>
> >
> > I bisected this to:
> >
> > 4ba4f1afb6a9 ("perf: Generic hotplug support for a PMU with a scope")
> >
> > This adds a perf_event_setup_cpumask() function that uses
> > list_for_each_entry_rcu() without an obvious RCU read-side critical
> > section, so the fix might be as simple as adding rcu_read_lock() and
> > rcu_read_unlock(). In the proper places, of course. ;-)
>
> IIRC that condition should be:
>
> lockdep_is_held(&pmus_srcu) || lockdep_is_held(&pmus_lock)
>
> And at this pooint we actually do hold pmus_lock.
>
> But that all begs the question why we're using RCU iteration here to
> begin with, as this code seems to be only called from this context.
>
> Kan, is the simple fix to do:
>
> - list_for_each_entry_rcu(pmu, &pmus, entry, lockdep_is_held(&pmus_srcu)) {
> + list_for_each_entry(pmu, &pmus, entry) {
>
> ?
It does pass a quick test, for whatever that might be worth. ;-)
So if this turns out to be the right approach:
Tested-by: Paul E. McKenney <paulmck@kernel.org>
Thanx, Paul
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [BUG BISECTED] Missing RCU reader in perf_event_setup_cpumask()
2024-09-13 10:47 ` Peter Zijlstra
2024-09-13 12:09 ` Paul E. McKenney
@ 2024-09-13 15:51 ` Liang, Kan
1 sibling, 0 replies; 4+ messages in thread
From: Liang, Kan @ 2024-09-13 15:51 UTC (permalink / raw)
To: Peter Zijlstra, Paul E. McKenney
Cc: linux-kernel, linux-perf-users, mingo, acme, namhyung,
mark.rutland, alexander.shishkin, jolsa, irogers, adrian.hunter
On 2024-09-13 6:47 a.m., Peter Zijlstra wrote:
> On Fri, Sep 13, 2024 at 01:00:44AM -0700, Paul E. McKenney wrote:
>> Hello!
>>
>> On next-20240912 running rcutorture scenario TREE05, I see this
>> deterministically:
>>
>> [ 32.603233] =============================
>> [ 32.604594] WARNING: suspicious RCU usage
>> [ 32.605928] 6.11.0-rc5-00040-g4ba4f1afb6a9 #55238 Not tainted
>> [ 32.607812] -----------------------------
>> [ 32.609140] kernel/events/core.c:13946 RCU-list traversed in non-reader section!!
>> [ 32.611595]
>> [ 32.611595] other info that might help us debug this:
>> [ 32.611595]
>> [ 32.614247]
>> [ 32.614247] rcu_scheduler_active = 2, debug_locks = 1
>> [ 32.616392] 3 locks held by cpuhp/4/35:
>> [ 32.617687] #0: ffffffffb666a650 (cpu_hotplug_lock){++++}-{0:0}, at: cpuhp_thread_fun+0x4e/0x200
>> [ 32.620563] #1: ffffffffb666cd20 (cpuhp_state-down){+.+.}-{0:0}, at: cpuhp_thread_fun+0x4e/0x200
>> [ 32.623412] #2: ffffffffb677c288 (pmus_lock){+.+.}-{3:3}, at: perf_event_exit_cpu_context+0x32/0x2f0
>> [ 32.626399]
>> [ 32.626399] stack backtrace:
>> [ 32.627848] CPU: 4 UID: 0 PID: 35 Comm: cpuhp/4 Not tainted 6.11.0-rc5-00040-g4ba4f1afb6a9 #55238
>> [ 32.628832] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.14.0-0-g155821a1990b-prebuilt.qemu.org 04/01/2014
>> [ 32.628832] Call Trace:
>> [ 32.628832] <TASK>
>> [ 32.628832] dump_stack_lvl+0x83/0xa0
>> [ 32.628832] lockdep_rcu_suspicious+0x143/0x1a0
>> [ 32.628832] perf_event_exit_cpu_context+0x2e5/0x2f0
>> [ 32.628832] ? __pfx_perf_event_exit_cpu+0x10/0x10
>> [ 32.628832] perf_event_exit_cpu+0x9/0x10
>> [ 32.628832] cpuhp_invoke_callback+0x130/0x2a0
>> [ 32.628832] ? lock_release+0xc7/0x290
>> [ 32.628832] ? cpuhp_thread_fun+0x4e/0x200
>> [ 32.628832] cpuhp_thread_fun+0x183/0x200
>> [ 32.628832] smpboot_thread_fn+0xd8/0x1d0
>> [ 32.628832] ? __pfx_smpboot_thread_fn+0x10/0x10
>> [ 32.628832] kthread+0xd4/0x100
>> [ 32.628832] ? __pfx_kthread+0x10/0x10
>> [ 32.628832] ret_from_fork+0x2f/0x50
>> [ 32.628832] ? __pfx_kthread+0x10/0x10
>> [ 32.628832] ret_from_fork_asm+0x1a/0x30
>> [ 32.628832] </TASK>
>>
>> I bisected this to:
>>
>> 4ba4f1afb6a9 ("perf: Generic hotplug support for a PMU with a scope")
>>
>> This adds a perf_event_setup_cpumask() function that uses
>> list_for_each_entry_rcu() without an obvious RCU read-side critical
>> section, so the fix might be as simple as adding rcu_read_lock() and
>> rcu_read_unlock(). In the proper places, of course. ;-)
>
> IIRC that condition should be:
>
> lockdep_is_held(&pmus_srcu) || lockdep_is_held(&pmus_lock)
>
> And at this pooint we actually do hold pmus_lock.
>
> But that all begs the question why we're using RCU iteration here to
> begin with, as this code seems to be only called from this context.
I think I just copied and paste the PMU iterate code here, and forget to
add the srcu_read_lock(). Sorry for it.
>
> Kan, is the simple fix to do:
>
> - list_for_each_entry_rcu(pmu, &pmus, entry, lockdep_is_held(&pmus_srcu)) {
> + list_for_each_entry(pmu, &pmus, entry) {
>
> ?
>
Yes, the &pmus_lock protect is good enough. we don't need the rcu here.
I will post a patch with the suggested fix.
Thanks,
Kan
^ permalink raw reply [flat|nested] 4+ messages in thread