Skip to contentSkip to conversation

cpuidle: coupled: fix sleeping while atomic in cpu notifier

PATCH In review Colin Cross 4 participants 13 messages Jul 25, 2012 – Aug 15, 2012 CPU idle time management framework

lore
Share & tools
lore

Colin Cross

author
#0 lore raw

PATCH 1/1 cpuidle: coupled: fix sleeping while atomic in cpu notifier

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 <[email protected]>
---
 drivers/cpuidle/coupled.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)
drivers/cpuidle/coupled.c +12 -0
index 2c9bf26..c24dda0 100644
@@ -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);
Signature · 1 line
1.7.7.3
#1 lore raw
Quoted 7 lines · Colin Cross

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 <[email protected]>

---

Agree.
Have you observed any lock-up ?

For that patch itself, Acked-by: Santosh Shilimkar <[email protected]>

#2 lore raw
Quoted 11 lines · Santosh
Quoted 7 lines · Colin Cross

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 <[email protected]>

---

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

#3 lore raw
In reply to Colin Cross
Quoted 4 lines · Colin Cross

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

Quoted 29 lines
Signed-off-by Colin Cross <[email protected]>
---
 drivers/cpuidle/coupled.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)
drivers/cpuidle/coupled.c +12 -0
index 2c9bf26..c24dda0 100644
@@ -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);
author
#4 lore raw
Quoted 7 lines · Rafael J. Wysocki
Quoted 4 lines · Colin Cross

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.

#5 lore raw
Quoted 10 lines · Colin Cross
Quoted 7 lines · Rafael J. Wysocki
Quoted 4 lines · Colin Cross

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?

#6 lore raw
Quoted 8 lines · Rafael J. Wysocki
Quoted 7 lines · Colin Cross
Quoted 6 lines · Rafael J. Wysocki
Quoted 4 lines · Colin Cross

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.

Quoted 5 lines
Quoted 3 lines
Quoted 1 line

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

#7 lore raw
In reply to Colin Cross
Quoted 20 lines · Colin Cross

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 <[email protected]>
---
 drivers/cpuidle/coupled.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)
drivers/cpuidle/coupled.c +2 -0
index 2c9bf26..c24dda0 100644
@@ -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:

drivers/cpuidle/coupled.c static int cpuidle_coupled_cpu_notify(struct notifier_block *nb, · line 692

Quoted 10 lines
drivers/cpuidle/coupled.c +10 -0
+	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

Quoted 4 lines
drivers/cpuidle/coupled.c +0 -0
 	mutex_lock(&cpuidle_lock);
 
 	dev = per_cpu(cpuidle_devices, cpu);
author
#8 lore raw
Quoted 35 lines · Srivatsa S. Bhat
Quoted 20 lines · Colin Cross

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 <[email protected]>
---
 drivers/cpuidle/coupled.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)
drivers/cpuidle/coupled.c +2 -0
index 2c9bf26..c24dda0 100644
@@ -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:

drivers/cpuidle/coupled.c static int cpuidle_coupled_cpu_notify(struct notifier_block *nb, · line 692

Quoted 10 lines
drivers/cpuidle/coupled.c +10 -0
+     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?

Quoted 6 lines

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.

Reviewed-by
#9 lore raw
Quoted 44 lines · Colin Cross
Quoted 35 lines · Srivatsa S. Bhat
Quoted 20 lines · Colin Cross

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 <[email protected]>
---
 drivers/cpuidle/coupled.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)
drivers/cpuidle/coupled.c +2 -0
index 2c9bf26..c24dda0 100644
@@ -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:

drivers/cpuidle/coupled.c static int cpuidle_coupled_cpu_notify(struct notifier_block *nb, · line 692

Quoted 10 lines
drivers/cpuidle/coupled.c +10 -0
+     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?

Quoted 6 lines

Something like:

        switch (action & ~CPU_TASKS_FROZEN) {
        case CPU_STARTING:
        case CPU_DYING:
                return NOTIFY_OK;
        }
Quoted 3 lines

No, because then it would need updating whenever a new notification event was added.

Hmm.. Fair enough.

Reviewed-by Srivatsa S. Bhat <[email protected]>

Regards,
Srivatsa S. Bhat

#10 lore raw
In reply to Colin Cross
Quoted 6 lines · Colin Cross

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 <[email protected]>

Has this been applied already?

Rafael

Quoted 28 lines
---
 drivers/cpuidle/coupled.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)
drivers/cpuidle/coupled.c +12 -0
index 2c9bf26..c24dda0 100644
@@ -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);
author
#11 lore raw
Quoted 9 lines · Rafael J. Wysocki
Quoted 6 lines · Colin Cross

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 <[email protected]>

Has this been applied already?

It's not in Linus' tree, and I haven't heard anything from Len.

#12 lore raw
Quoted 7 lines · Colin Cross
Quoted 6 lines · Rafael J. Wysocki
Quoted 5 lines · Colin Cross

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.

Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Signed-off-by Colin Cross <[email protected]>

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