* Re: [PATCH] cpuidle: coupled: fix sleeping while atomic in cpu notifier
2012-07-25 21:20 [PATCH] cpuidle: coupled: fix sleeping while atomic in cpu notifier Colin Cross
@ 2012-07-26 7:25 ` Shilimkar, Santosh
2012-07-26 22:54 ` Shilimkar, Santosh
2012-07-26 19:55 ` Rafael J. Wysocki
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Shilimkar, Santosh @ 2012-07-26 7:25 UTC (permalink / raw)
To: Colin Cross
Cc: Len Brown, Len Brown, Kevin Hilman, Rafael J. Wysocki,
linux-kernel
On Wed, Jul 25, 2012 at 11:20 PM, Colin Cross <ccross@android.com> wrote:
> The cpu hotplug notifier gets called in both atomic and non-atomic
> contexts, it is not always safe to lock a mutex. Filter out all events
> except the six necessary ones, which are all sleepable, before taking
> the mutex.
>
> Signed-off-by: Colin Cross <ccross@android.com>
> ---
Agree.
Have you observed any lock-up ?
For that patch itself, Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] cpuidle: coupled: fix sleeping while atomic in cpu notifier
2012-07-26 7:25 ` Shilimkar, Santosh
@ 2012-07-26 22:54 ` Shilimkar, Santosh
0 siblings, 0 replies; 13+ messages in thread
From: Shilimkar, Santosh @ 2012-07-26 22:54 UTC (permalink / raw)
To: linux-kernel
Cc: Len Brown, Len Brown, Kevin Hilman, Rafael J. Wysocki,
Colin Cross
On Thu, Jul 26, 2012 at 9:25 AM, Shilimkar, Santosh
<santosh.shilimkar@ti.com> wrote:
> On Wed, Jul 25, 2012 at 11:20 PM, Colin Cross <ccross@android.com> wrote:
>> The cpu hotplug notifier gets called in both atomic and non-atomic
>> contexts, it is not always safe to lock a mutex. Filter out all events
>> except the six necessary ones, which are all sleepable, before taking
>> the mutex.
>>
>> Signed-off-by: Colin Cross <ccross@android.com>
>> ---
> Agree.
> Have you observed any lock-up ?
>
Colin explained me about cause of the issue in an off-list discussion.
Thought of updating the thread in case some one wants to reproduce the
issue. You get a warning during cpu hotplug in suspend if you turn on
sleeping while atomic debugging option in kernel build and the patch
fixes it.
Regards
Santosh
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] cpuidle: coupled: fix sleeping while atomic in cpu notifier
2012-07-25 21:20 [PATCH] cpuidle: coupled: fix sleeping while atomic in cpu notifier Colin Cross
2012-07-26 7:25 ` Shilimkar, Santosh
@ 2012-07-26 19:55 ` Rafael J. Wysocki
2012-07-26 19:51 ` Colin Cross
2012-07-31 15:43 ` Srivatsa S. Bhat
2012-08-07 22:15 ` Rafael J. Wysocki
3 siblings, 1 reply; 13+ messages in thread
From: Rafael J. Wysocki @ 2012-07-26 19:55 UTC (permalink / raw)
To: Colin Cross
Cc: Len Brown, Len Brown, Kevin Hilman, Santosh Shilimkar,
linux-kernel, Linux PM list
On Wednesday, July 25, 2012, Colin Cross wrote:
> The cpu hotplug notifier gets called in both atomic and non-atomic
> contexts, it is not always safe to lock a mutex. Filter out all events
> except the six necessary ones, which are all sleepable, before taking
> the mutex.
I wonder what mutual exclusion mechanis we rely on when the mutex is not taken?
Rafael
> Signed-off-by: Colin Cross <ccross@android.com>
> ---
> drivers/cpuidle/coupled.c | 12 ++++++++++++
> 1 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/cpuidle/coupled.c b/drivers/cpuidle/coupled.c
> index 2c9bf26..c24dda0 100644
> --- a/drivers/cpuidle/coupled.c
> +++ b/drivers/cpuidle/coupled.c
> @@ -678,6 +678,18 @@ static int cpuidle_coupled_cpu_notify(struct notifier_block *nb,
> int cpu = (unsigned long)hcpu;
> struct cpuidle_device *dev;
>
> + switch (action & ~CPU_TASKS_FROZEN) {
> + case CPU_UP_PREPARE:
> + case CPU_DOWN_PREPARE:
> + case CPU_ONLINE:
> + case CPU_DEAD:
> + case CPU_UP_CANCELED:
> + case CPU_DOWN_FAILED:
> + break;
> + default:
> + return NOTIFY_OK;
> + }
> +
> mutex_lock(&cpuidle_lock);
>
> dev = per_cpu(cpuidle_devices, cpu);
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] cpuidle: coupled: fix sleeping while atomic in cpu notifier
2012-07-26 19:55 ` Rafael J. Wysocki
@ 2012-07-26 19:51 ` Colin Cross
2012-07-26 20:15 ` Rafael J. Wysocki
0 siblings, 1 reply; 13+ messages in thread
From: Colin Cross @ 2012-07-26 19:51 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Len Brown, Len Brown, Kevin Hilman, Santosh Shilimkar,
linux-kernel, Linux PM list
On Thu, Jul 26, 2012 at 12:55 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Wednesday, July 25, 2012, Colin Cross wrote:
>> The cpu hotplug notifier gets called in both atomic and non-atomic
>> contexts, it is not always safe to lock a mutex. Filter out all events
>> except the six necessary ones, which are all sleepable, before taking
>> the mutex.
>
> I wonder what mutual exclusion mechanis we rely on when the mutex is not taken?
We don't need any mutual exclusion because the notifier returns immediately.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] cpuidle: coupled: fix sleeping while atomic in cpu notifier
2012-07-26 19:51 ` Colin Cross
@ 2012-07-26 20:15 ` Rafael J. Wysocki
2012-07-26 20:16 ` Rafael J. Wysocki
0 siblings, 1 reply; 13+ messages in thread
From: Rafael J. Wysocki @ 2012-07-26 20:15 UTC (permalink / raw)
To: Colin Cross
Cc: Len Brown, Len Brown, Kevin Hilman, Santosh Shilimkar,
linux-kernel, Linux PM list
On Thursday, July 26, 2012, Colin Cross wrote:
> On Thu, Jul 26, 2012 at 12:55 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Wednesday, July 25, 2012, Colin Cross wrote:
> >> The cpu hotplug notifier gets called in both atomic and non-atomic
> >> contexts, it is not always safe to lock a mutex. Filter out all events
> >> except the six necessary ones, which are all sleepable, before taking
> >> the mutex.
> >
> > I wonder what mutual exclusion mechanis we rely on when the mutex is not taken?
>
> We don't need any mutual exclusion because the notifier returns immediately.
Don't we need to disable preemption even?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] cpuidle: coupled: fix sleeping while atomic in cpu notifier
2012-07-26 20:15 ` Rafael J. Wysocki
@ 2012-07-26 20:16 ` Rafael J. Wysocki
0 siblings, 0 replies; 13+ messages in thread
From: Rafael J. Wysocki @ 2012-07-26 20:16 UTC (permalink / raw)
To: Colin Cross
Cc: Len Brown, Len Brown, Kevin Hilman, Santosh Shilimkar,
linux-kernel, Linux PM list
On Thursday, July 26, 2012, Rafael J. Wysocki wrote:
> On Thursday, July 26, 2012, Colin Cross wrote:
> > On Thu, Jul 26, 2012 at 12:55 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > On Wednesday, July 25, 2012, Colin Cross wrote:
> > >> The cpu hotplug notifier gets called in both atomic and non-atomic
> > >> contexts, it is not always safe to lock a mutex. Filter out all events
> > >> except the six necessary ones, which are all sleepable, before taking
> > >> the mutex.
> > >
> > > I wonder what mutual exclusion mechanis we rely on when the mutex is not taken?
> >
> > We don't need any mutual exclusion because the notifier returns immediately.
>
> Don't we need to disable preemption even?
Sorry, scratch that. It returns NOTIFY_OK if we're not going to take the
mutex.
Thanks,
Rafael
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] cpuidle: coupled: fix sleeping while atomic in cpu notifier
2012-07-25 21:20 [PATCH] cpuidle: coupled: fix sleeping while atomic in cpu notifier Colin Cross
2012-07-26 7:25 ` Shilimkar, Santosh
2012-07-26 19:55 ` Rafael J. Wysocki
@ 2012-07-31 15:43 ` Srivatsa S. Bhat
2012-07-31 18:27 ` Colin Cross
2012-08-07 22:15 ` Rafael J. Wysocki
3 siblings, 1 reply; 13+ messages in thread
From: Srivatsa S. Bhat @ 2012-07-31 15:43 UTC (permalink / raw)
To: Colin Cross
Cc: Len Brown, Len Brown, Kevin Hilman, Santosh Shilimkar,
Rafael J. Wysocki, linux-kernel, Linux PM mailing list
On 07/26/2012 02:50 AM, Colin Cross wrote:
> The cpu hotplug notifier gets called in both atomic and non-atomic
> contexts, it is not always safe to lock a mutex. Filter out all events
> except the six necessary ones, which are all sleepable, before taking
> the mutex.
>
> Signed-off-by: Colin Cross <ccross@android.com>
> ---
> drivers/cpuidle/coupled.c | 12 ++++++++++++
> 1 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/cpuidle/coupled.c b/drivers/cpuidle/coupled.c
> index 2c9bf26..c24dda0 100644
> --- a/drivers/cpuidle/coupled.c
> +++ b/drivers/cpuidle/coupled.c
> @@ -678,6 +678,18 @@ static int cpuidle_coupled_cpu_notify(struct notifier_block *nb,
> int cpu = (unsigned long)hcpu;
> struct cpuidle_device *dev;
>
> + switch (action & ~CPU_TASKS_FROZEN) {
> + case CPU_UP_PREPARE:
> + case CPU_DOWN_PREPARE:
> + case CPU_ONLINE:
> + case CPU_DEAD:
> + case CPU_UP_CANCELED:
> + case CPU_DOWN_FAILED:
> + break;
> + default:
> + return NOTIFY_OK;
> + }
> +
Instead, wouldn't it be better to have case statements for the
2 cases that imply atomic context and return immediately?
Something like:
switch (action & ~CPU_TASKS_FROZEN) {
case CPU_STARTING:
case CPU_DYING:
return NOTIFY_OK;
}
Regards,
Srivatsa S. Bhat
> mutex_lock(&cpuidle_lock);
>
> dev = per_cpu(cpuidle_devices, cpu);
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] cpuidle: coupled: fix sleeping while atomic in cpu notifier
2012-07-31 15:43 ` Srivatsa S. Bhat
@ 2012-07-31 18:27 ` Colin Cross
2012-08-01 5:59 ` Srivatsa S. Bhat
0 siblings, 1 reply; 13+ messages in thread
From: Colin Cross @ 2012-07-31 18:27 UTC (permalink / raw)
To: Srivatsa S. Bhat
Cc: Len Brown, Len Brown, Kevin Hilman, Santosh Shilimkar,
Rafael J. Wysocki, linux-kernel, Linux PM mailing list
On Tue, Jul 31, 2012 at 8:43 AM, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:
> On 07/26/2012 02:50 AM, Colin Cross wrote:
>> The cpu hotplug notifier gets called in both atomic and non-atomic
>> contexts, it is not always safe to lock a mutex. Filter out all events
>> except the six necessary ones, which are all sleepable, before taking
>> the mutex.
>>
>> Signed-off-by: Colin Cross <ccross@android.com>
>> ---
>> drivers/cpuidle/coupled.c | 12 ++++++++++++
>> 1 files changed, 12 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/cpuidle/coupled.c b/drivers/cpuidle/coupled.c
>> index 2c9bf26..c24dda0 100644
>> --- a/drivers/cpuidle/coupled.c
>> +++ b/drivers/cpuidle/coupled.c
>> @@ -678,6 +678,18 @@ static int cpuidle_coupled_cpu_notify(struct notifier_block *nb,
>> int cpu = (unsigned long)hcpu;
>> struct cpuidle_device *dev;
>>
>> + switch (action & ~CPU_TASKS_FROZEN) {
>> + case CPU_UP_PREPARE:
>> + case CPU_DOWN_PREPARE:
>> + case CPU_ONLINE:
>> + case CPU_DEAD:
>> + case CPU_UP_CANCELED:
>> + case CPU_DOWN_FAILED:
>> + break;
>> + default:
>> + return NOTIFY_OK;
>> + }
>> +
>
> Instead, wouldn't it be better to have case statements for the
> 2 cases that imply atomic context and return immediately?
>
> Something like:
> switch (action & ~CPU_TASKS_FROZEN) {
> case CPU_STARTING:
> case CPU_DYING:
> return NOTIFY_OK;
> }
No, because then it would need updating whenever a new notification
event was added.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] cpuidle: coupled: fix sleeping while atomic in cpu notifier
2012-07-31 18:27 ` Colin Cross
@ 2012-08-01 5:59 ` Srivatsa S. Bhat
0 siblings, 0 replies; 13+ messages in thread
From: Srivatsa S. Bhat @ 2012-08-01 5:59 UTC (permalink / raw)
To: Colin Cross
Cc: Len Brown, Len Brown, Kevin Hilman, Santosh Shilimkar,
Rafael J. Wysocki, linux-kernel, Linux PM mailing list
On 07/31/2012 11:57 PM, Colin Cross wrote:
> On Tue, Jul 31, 2012 at 8:43 AM, Srivatsa S. Bhat
> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>> On 07/26/2012 02:50 AM, Colin Cross wrote:
>>> The cpu hotplug notifier gets called in both atomic and non-atomic
>>> contexts, it is not always safe to lock a mutex. Filter out all events
>>> except the six necessary ones, which are all sleepable, before taking
>>> the mutex.
>>>
>>> Signed-off-by: Colin Cross <ccross@android.com>
>>> ---
>>> drivers/cpuidle/coupled.c | 12 ++++++++++++
>>> 1 files changed, 12 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/cpuidle/coupled.c b/drivers/cpuidle/coupled.c
>>> index 2c9bf26..c24dda0 100644
>>> --- a/drivers/cpuidle/coupled.c
>>> +++ b/drivers/cpuidle/coupled.c
>>> @@ -678,6 +678,18 @@ static int cpuidle_coupled_cpu_notify(struct notifier_block *nb,
>>> int cpu = (unsigned long)hcpu;
>>> struct cpuidle_device *dev;
>>>
>>> + switch (action & ~CPU_TASKS_FROZEN) {
>>> + case CPU_UP_PREPARE:
>>> + case CPU_DOWN_PREPARE:
>>> + case CPU_ONLINE:
>>> + case CPU_DEAD:
>>> + case CPU_UP_CANCELED:
>>> + case CPU_DOWN_FAILED:
>>> + break;
>>> + default:
>>> + return NOTIFY_OK;
>>> + }
>>> +
>>
>> Instead, wouldn't it be better to have case statements for the
>> 2 cases that imply atomic context and return immediately?
>>
>> Something like:
>> switch (action & ~CPU_TASKS_FROZEN) {
>> case CPU_STARTING:
>> case CPU_DYING:
>> return NOTIFY_OK;
>> }
>
> No, because then it would need updating whenever a new notification
> event was added.
>
Hmm.. Fair enough.
Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Regards,
Srivatsa S. Bhat
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] cpuidle: coupled: fix sleeping while atomic in cpu notifier
2012-07-25 21:20 [PATCH] cpuidle: coupled: fix sleeping while atomic in cpu notifier Colin Cross
` (2 preceding siblings ...)
2012-07-31 15:43 ` Srivatsa S. Bhat
@ 2012-08-07 22:15 ` Rafael J. Wysocki
2012-08-08 0:54 ` Colin Cross
3 siblings, 1 reply; 13+ messages in thread
From: Rafael J. Wysocki @ 2012-08-07 22:15 UTC (permalink / raw)
To: Colin Cross
Cc: Len Brown, Len Brown, Kevin Hilman, Santosh Shilimkar,
linux-kernel
On Wednesday, July 25, 2012, Colin Cross wrote:
> The cpu hotplug notifier gets called in both atomic and non-atomic
> contexts, it is not always safe to lock a mutex. Filter out all events
> except the six necessary ones, which are all sleepable, before taking
> the mutex.
>
> Signed-off-by: Colin Cross <ccross@android.com>
Has this been applied already?
Rafael
> ---
> drivers/cpuidle/coupled.c | 12 ++++++++++++
> 1 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/cpuidle/coupled.c b/drivers/cpuidle/coupled.c
> index 2c9bf26..c24dda0 100644
> --- a/drivers/cpuidle/coupled.c
> +++ b/drivers/cpuidle/coupled.c
> @@ -678,6 +678,18 @@ static int cpuidle_coupled_cpu_notify(struct notifier_block *nb,
> int cpu = (unsigned long)hcpu;
> struct cpuidle_device *dev;
>
> + switch (action & ~CPU_TASKS_FROZEN) {
> + case CPU_UP_PREPARE:
> + case CPU_DOWN_PREPARE:
> + case CPU_ONLINE:
> + case CPU_DEAD:
> + case CPU_UP_CANCELED:
> + case CPU_DOWN_FAILED:
> + break;
> + default:
> + return NOTIFY_OK;
> + }
> +
> mutex_lock(&cpuidle_lock);
>
> dev = per_cpu(cpuidle_devices, cpu);
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] cpuidle: coupled: fix sleeping while atomic in cpu notifier
2012-08-07 22:15 ` Rafael J. Wysocki
@ 2012-08-08 0:54 ` Colin Cross
2012-08-15 20:16 ` Rafael J. Wysocki
0 siblings, 1 reply; 13+ messages in thread
From: Colin Cross @ 2012-08-08 0:54 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Len Brown, Len Brown, Kevin Hilman, Santosh Shilimkar,
linux-kernel
On Tue, Aug 7, 2012 at 3:15 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Wednesday, July 25, 2012, Colin Cross wrote:
>> The cpu hotplug notifier gets called in both atomic and non-atomic
>> contexts, it is not always safe to lock a mutex. Filter out all events
>> except the six necessary ones, which are all sleepable, before taking
>> the mutex.
>>
>> Signed-off-by: Colin Cross <ccross@android.com>
>
> Has this been applied already?
It's not in Linus' tree, and I haven't heard anything from Len.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] cpuidle: coupled: fix sleeping while atomic in cpu notifier
2012-08-08 0:54 ` Colin Cross
@ 2012-08-15 20:16 ` Rafael J. Wysocki
0 siblings, 0 replies; 13+ messages in thread
From: Rafael J. Wysocki @ 2012-08-15 20:16 UTC (permalink / raw)
To: Colin Cross
Cc: Len Brown, Len Brown, Kevin Hilman, Santosh Shilimkar,
linux-kernel
On Wednesday, August 08, 2012, Colin Cross wrote:
> On Tue, Aug 7, 2012 at 3:15 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Wednesday, July 25, 2012, Colin Cross wrote:
> >> The cpu hotplug notifier gets called in both atomic and non-atomic
> >> contexts, it is not always safe to lock a mutex. Filter out all events
> >> except the six necessary ones, which are all sleepable, before taking
> >> the mutex.
> >>
> >> Signed-off-by: Colin Cross <ccross@android.com>
> >
> > Has this been applied already?
>
> It's not in Linus' tree, and I haven't heard anything from Len.
Len appears to be unavailable.
I'll put it into the linux-next branch of the linux-pm.git tree and will
push it to Linus for -rc3 if Len doesn't show up till then.
Thanks,
Rafael
^ permalink raw reply [flat|nested] 13+ messages in thread