On 01/02/2019 18:01, Will Deacon wrote:
On Wed, Jan 30, 2019 at 07:09:42PM +0000, Robin Murphy wrote:
On 2019-01-30 6:21 pm, Will Deacon wrote:
[+Suzuki and Robin]
On Mon, Jan 28, 2019 at 07:19:20AM +0000, Li, Meng wrote:
When enable kernel configure CONFIG_DEBUG_ATOMIC_SLEEP, there is below trace
during pmu arm cci driver probe phase.
[ 1.983337] BUG: sleeping function called from invalid context at
kernel/locking/rtmutex.c:2004
[ 1.983340] in_atomic(): 1, irqs_disabled(): 0, pid: 1, name: swapper/0
[ 1.983342] Preemption disabled at:
[ 1.983353] [<ffffff80089801f4>] cci_pmu_probe+0x1dc/0x488
[ 1.983360] CPU: 0 PID: 1 Comm: swapper/0 Not tainted
4.18.20-rt8-yocto-preempt-rt #1
[ 1.983362] Hardware name: ZynqMP ZCU102 Rev1.0 (DT)
[ 1.983364] Call trace:
[ 1.983369] dump_backtrace+0x0/0x158
[ 1.983372] show_stack+0x24/0x30
[ 1.983378] dump_stack+0x80/0xa4
[ 1.983383] ___might_sleep+0x138/0x160
[ 1.983386] __might_sleep+0x58/0x90
[ 1.983391] __rt_mutex_lock_state+0x30/0xc0
[ 1.983395] _mutex_lock+0x24/0x30
[ 1.983400] perf_pmu_register+0x2c/0x388
[ 1.983404] cci_pmu_probe+0x2bc/0x488
[ 1.983409] platform_drv_probe+0x58/0xa8
Because get_cpu() is invoked, preempt is disable, finally, trace occurs when
call might_sleep()
Hmm, the {get,put}_cpu() usage here looks very broken to me. There's the
fact that it might sleep, but also the assignment to g_cci_pmu is done after
we've re-enabled preemption, so there's a race with CPU hotplug there too.
Hmm, looks like I failed to appreciate that particular race at the time -
indeed the global should probably be assigned immediately after
cci_pmu_init() has succeeded.
I don't think we can simply register the hotplug notifier before registering
the PMU, because we can't call into perf_pmu_migrate_context() until the PMU
has been registered. Perhaps we need to use the _cpuslocked() versions of
the hotplug notifier registration functions.
I tried looking at some other drivers, but they all look broken to me, so
there's a good chance I'm missing something. Anybody know how this is
supposed to work?
As I understand the general pattern, we register the notifier last to avoid
taking a hotplug callback with a partly-initialised PMU state, however since
the CPU we've picked is part of that PMU state, we also want to avoid
getting migrated off that CPU before the notifier is in place lest things
get out of sync, hence disabling preemption. As far as the correctness of
implementing that logic, though, it was like that when I got here so I've
always just assumed it was fine :)
I guess the question is whether we actually need to pick our nominal CPU
before perf_pmu_register(), or if something like the below would suffice -
what do you reckon?
Robin.
----->8-----
diff --git a/drivers/perf/arm-cci.c b/drivers/perf/arm-cci.c
index 1bfeb160c5b1..da9309ff80d7 100644
--- a/drivers/perf/arm-cci.c
+++ b/drivers/perf/arm-cci.c
@@ -1692,19 +1692,18 @@ static int cci_pmu_probe(struct platform_device
*pdev)
raw_spin_lock_init(&cci_pmu->hw_events.pmu_lock);
mutex_init(&cci_pmu->reserve_mutex);
atomic_set(&cci_pmu->active_events, 0);
- cci_pmu->cpu = get_cpu();
+ cci_pmu->cpu = -1; /* Avoid races until hotplug notifier is alive */
ret = cci_pmu_init(cci_pmu, pdev);
So at this point we've registered the PMU with perf, so I think we're open
to userspace. Given that things like pmu_cpumask_attr_show() call
cpumask_of(cci_pmu->cpu), having a cpu of -1 seems like a bad idea.
Why not just use the _cpuslocked() notifier registration functions so that
we don't need to disable preemption?
Because that alone doesn't necessarily help, but what I failed to grasp
is the implication that in order to do it you need to manually take the
hotplug lock, and if you do *that* in the right places, it removes the
race condition altogether. Now that I've made sense of it, I think
that's actually the only valid way to solve the problem. Let me spin a
proper patch...
Robin.
