On Fri, Feb 01, 2019 at 06:42:46PM +0000, Robin Murphy wrote:
> 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...
Yeah, sorry for being unhelpfully vague there. I meant using the
_cpuslocked() calls in conjunction with cpus_read_lock().
I think at least the DSU PMU driver is also broken in this area.
Cheers,
Will
