Re: Could you please help to have a look a bug trace in pmu arm-cci.c

Re: Could you please help to have a look a bug trace in pmu arm-cci.c

[Will Deacon]

[+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.

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?

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Could you please help to have a look a bug trace in pmu arm-cci.c

[Robin Murphy]

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);
-       if (ret) {
-               put_cpu();
+       if (ret)
                 return ret;
-       }
+       g_cci_pmu = cci_pmu;

         cpuhp_setup_state_nocalls(CPUHP_AP_PERF_ARM_CCI_ONLINE,
                                   "perf/arm/cci:online", NULL,
                                   cci_pmu_offline_cpu);
-       put_cpu();
-       g_cci_pmu = cci_pmu;
+       cci_pmu->cpu = smp_processor_id();
+
         pr_info("ARM %s PMU driver probed", cci_pmu->model->name);
         return 0;
  }

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: Could you please help to have a look a bug trace in pmu arm-cci.c

[Li, Meng]

> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy@arm.com]
> Sent: Thursday, January 31, 2019 3:10 AM
> To: Will Deacon; Li, Meng
> Cc: mark.rutland@arm.com; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; suzuki.poulose@arm.com
> Subject: Re: Could you please help to have a look a bug trace in pmu arm-cci.c
> 
> 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 */

If we set cci_pmu->cpu = -1 here, I think we should modify code of cci_pmu_event_init() as below:
//////// original ////////////////////////
	if (event->cpu < 0)
		return -EINVAL;
	event->cpu = cci_pmu->cpu;
//////// original ////////////////////////
Change into
//////// new ////////////////////////
	event->cpu = cci_pmu->cpu;
	if (event->cpu < 0)
		return -EINVAL;
//////// new ////////////////////////
Move this if condition after the assignment value operation.
Otherwise, when call perf_event_open()->find_get_context(), 
int cpu = event->cpu = -1;
-1 as a parameter passed into per_cpu_ptr(), and return a wrong cpuctx,
If we continue to execute code with the wrong cpuctx, kernel may crash.

I am not sure whether my analysis is reasonable, only just for your reference.

Thanks,
Limeng

> 
>          ret = cci_pmu_init(cci_pmu, pdev);
> -       if (ret) {
> -               put_cpu();
> +       if (ret)
>                  return ret;
> -       }
> +       g_cci_pmu = cci_pmu;
> 
>          cpuhp_setup_state_nocalls(CPUHP_AP_PERF_ARM_CCI_ONLINE,
>                                    "perf/arm/cci:online", NULL,
>                                    cci_pmu_offline_cpu);
> -       put_cpu();
> -       g_cci_pmu = cci_pmu;
> +       cci_pmu->cpu = smp_processor_id();
> +
>          pr_info("ARM %s PMU driver probed", cci_pmu->model->name);
>          return 0;
>   }
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Could you please help to have a look a bug trace in pmu arm-cci.c

[Will Deacon]

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?

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Could you please help to have a look a bug trace in pmu arm-cci.c

[Robin Murphy]

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Could you please help to have a look a bug trace in pmu arm-cci.c

[Will Deacon]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel