[PATCH v2 0/3] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[PATCH v2 0/3] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Srivatsa S. Bhat]

When CPU hotplug is run along with suspend/hibernate tests using
the pm_test framework, even at the freezer level, we hit task freezing
failures. One such failure was reported here:
https://lkml.org/lkml/2011/9/5/28

An excerpt of the log:

  Freezing of tasks failed after 20.01 seconds (2 tasks refusing to
  freeze, wq_busy=0):
  invert_cpu_stat D 0000000000000000  5304 20435  17329 0x00000084
   ffff8801f367bab8 0000000000000046 ffff8801f367bfd8 00000000001d3a00
   ffff8801f367a010 00000000001d3a00 00000000001d3a00 00000000001d3a00
   ffff8801f367bfd8 00000000001d3a00 ffff880414cc6840 ffff8801f36783c0
  Call Trace:
   [<ffffffff81532de5>] schedule_timeout+0x235/0x320
   [<ffffffff81532a0b>] wait_for_common+0x11b/0x170
   [<ffffffff81532b3d>] wait_for_completion+0x1d/0x20
   [<ffffffff81364486>] _request_firmware+0x156/0x2c0
   [<ffffffff81364686>] request_firmware+0x16/0x20
   [<ffffffffa01f0da0>] request_microcode_fw+0x70/0xf0 [microcode]
   [<ffffffffa01f0390>] microcode_init_cpu+0xc0/0x100 [microcode]
   [<ffffffffa01f14b4>] mc_cpu_callback+0x7c/0x11f [microcode]
   [<ffffffff815393a4>] notifier_call_chain+0x94/0xd0
   [<ffffffff8109770e>] __raw_notifier_call_chain+0xe/0x10
   [<ffffffff8106d000>] __cpu_notify+0x20/0x40
   [<ffffffff8152cf5b>] _cpu_up+0xc7/0x10e
   [<ffffffff8152d07b>] cpu_up+0xd9/0xec
   [<ffffffff8151e599>] store_online+0x99/0xd0
   [<ffffffff81355eb0>] sysdev_store+0x20/0x30
   [<ffffffff811f3096>] sysfs_write_file+0xe6/0x170
   [<ffffffff8117ee50>] vfs_write+0xd0/0x1a0
   [<ffffffff8117f024>] sys_write+0x54/0xa0
   [<ffffffff8153df02>] system_call_fastpath+0x16/0x1b


The reason behind this failure is explained below:

The x86 microcode update driver has callbacks registered for CPU hotplug
events such as a CPU getting offlined or onlined. Things go wrong when a
CPU hotplug stress test is carried out along with a suspend/resume operation
running simultaneously. Upon getting a CPU_DEAD notification (for example,
when a CPU offline occurs with tasks not frozen), the microcode callback
frees up the microcode and invalidates it. Later, when that CPU gets onlined
with tasks being frozen, the microcode callback (for the CPU_ONLINE_FROZEN
event) tries to apply the microcode to the CPU; doesn't find it and hence
depends on the (currently frozen) userspace to get the microcode again. This
leads to the numerous "WARNING"s at drivers/base/firmware_class.c which
eventually leads to task freezing failures in the suspend code path, as has
been reported.

So, this patch series addresses this issue by ensuring that CPU hotplug and
suspend/hibernate don't run in parallel, thereby fixing the task freezing
failures.

v2: Implemented mutual exclusion between CPU hotplug and suspend/hibernate.

Srivatsa S. Bhat (3):
      Introduce helper functions
      Mutually exclude cpu online and suspend/hibernate
      Update documentation

 Documentation/power/freezing-of-tasks.txt |   22 ++++++++++++++++++++++
 include/linux/suspend.h                   |   21 +++++++++++++++++++--
 kernel/cpu.c                              |   10 ++++++++++
 3 files changed, 51 insertions(+), 2 deletions(-)

[PATCH v2 1/3] Introduce helper functions

[Srivatsa S. Bhat]

Introduce helper functions used to prevent cpu hotplug (online operation) from
running in parallel with suspend or hibernate.

Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 include/linux/suspend.h |   21 +++++++++++++++++++--
 1 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 6bbcef2..89a5d27 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -276,7 +276,24 @@ static inline bool system_entering_hibernation(void) { return false; }
 #define PM_RESTORE_PREPARE	0x0005 /* Going to restore a saved image */
 #define PM_POST_RESTORE		0x0006 /* Restore failed */
 
+extern struct mutex pm_mutex;
+
 #ifdef CONFIG_PM_SLEEP
+
+/*
+ * Allow operations like CPU online to exclude suspend and hibernation
+ */
+static inline int trylock_pm_sleep(void)
+{
+	return mutex_trylock(&pm_mutex);
+}
+
+static inline void unlock_pm_sleep(void)
+{
+	mutex_unlock(&pm_mutex);
+}
+
 void save_processor_state(void);
 void restore_processor_state(void);
 
@@ -298,6 +315,8 @@ extern bool pm_get_wakeup_count(unsigned int *count);
 extern bool pm_save_wakeup_count(unsigned int count);
 #else /* !CONFIG_PM_SLEEP */
 
+static inline int trylock_pm_sleep(void) { return 1; }
+static inline void unlock_pm_sleep(void) {}
 static inline int register_pm_notifier(struct notifier_block *nb)
 {
 	return 0;
@@ -313,8 +332,6 @@ static inline int unregister_pm_notifier(struct notifier_block *nb)
 static inline bool pm_wakeup_pending(void) { return false; }
 #endif /* !CONFIG_PM_SLEEP */
 
-extern struct mutex pm_mutex;
-
 #ifndef CONFIG_HIBERNATE_CALLBACKS
 static inline void lock_system_sleep(void) {}
 static inline void unlock_system_sleep(void) {}

[PATCH v2 2/3] Mutually exclude cpu online and suspend/hibernate

[Srivatsa S. Bhat]

Don't allow cpu hotplug online operation and suspend/hibernate to run in
parallel. If suspend/hibernate has already started, fail the cpu online
operation.

Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 kernel/cpu.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/kernel/cpu.c b/kernel/cpu.c
index 12b7458..bc8c7d4 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -15,6 +15,7 @@
 #include <linux/stop_machine.h>
 #include <linux/mutex.h>
 #include <linux/gfp.h>
+#include <linux/suspend.h>
 
 #ifdef CONFIG_SMP
 /* Serializes the updates to cpu_online_mask, cpu_present_mask */
@@ -366,6 +367,14 @@ int __cpuinit cpu_up(unsigned int cpu)
 	}
 #endif
 
+	/*
+	 * Prevent cpu online and suspend/hibernate (including freezer)
+	 * operations from running in parallel. Fail cpu online if suspend or
+	 * hibernate has already started.
+	 */
+	if (!trylock_pm_sleep())
+		return -EBUSY;
+
 	cpu_maps_update_begin();
 
 	if (cpu_hotplug_disabled) {
@@ -377,6 +386,7 @@ int __cpuinit cpu_up(unsigned int cpu)
 
 out:
 	cpu_maps_update_done();
+	unlock_pm_sleep();
 	return err;
 }
 

[PATCH v2 3/3] Update documentation

[Srivatsa S. Bhat]

Update documentation about the cpu hotplug and suspend/hibernate
race condition related issue in Documentation/power directory.

Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 Documentation/power/freezing-of-tasks.txt |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/Documentation/power/freezing-of-tasks.txt b/Documentation/power/freezing-of-tasks.txt
index 38b5724..80acd30 100644
--- a/Documentation/power/freezing-of-tasks.txt
+++ b/Documentation/power/freezing-of-tasks.txt
@@ -176,3 +176,25 @@ tasks, since it generally exists anyway.
 A driver must have all firmwares it may need in RAM before suspend() is called.
 If keeping them is not practical, for example due to their size, they must be
 requested early enough using the suspend notifier API described in notifiers.txt.
+
+However, there is another problem related to request_firmware() which is not
+related to device drivers. The x86 microcode core uses request_firmware() to
+get the CPU microcode from userspace. And microcode updates can be triggered by
+various events such as loading of microcode driver module, reloading of
+microcode image by writing 1 to the sysfs file /sys/devices/system/cpu/cpu*/
+microcode/reload or by onlining a CPU during a CPU hotplug operation (in this
+case, the microcode driver's callback for CPU online event does a microcode
+update).
+In all these scenarios, due to some race condition with the freezing of tasks,
+if the userspace happens to be frozen at the moment when request_firmware() is
+called, then since the caller will be waiting for the firmware, eventually
+the freezing of tasks will fail.
+
+To solve the issue in the CPU hotplug case, synchronization has been added to
+prevent the CPU hotplug online operation from running in parallel with
+suspend/hibernate. The microcode driver module load/unload and microcode image
+reload are carried out quite infrequently in practice and hence the chances of
+these events racing with suspend/hibernation are very rare. Anyway, even those
+corner cases are expected to be handled well when microcode loading via the
+bootloader gets implemented.
+

Re: [PATCH v2 2/3] Mutually exclude cpu online and suspend/hibernate

[Srivatsa S. Bhat]

On 10/10/2011 06:03 PM, Srivatsa S. Bhat wrote:
> Don't allow cpu hotplug online operation and suspend/hibernate to run in
> parallel. If suspend/hibernate has already started, fail the cpu online
> operation.
> 
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> ---
> 
>  kernel/cpu.c |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 12b7458..bc8c7d4 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -15,6 +15,7 @@
>  #include <linux/stop_machine.h>
>  #include <linux/mutex.h>
>  #include <linux/gfp.h>
> +#include <linux/suspend.h>
> 
>  #ifdef CONFIG_SMP
>  /* Serializes the updates to cpu_online_mask, cpu_present_mask */
> @@ -366,6 +367,14 @@ int __cpuinit cpu_up(unsigned int cpu)
>  	}
>  #endif
> 
> +	/*
> +	 * Prevent cpu online and suspend/hibernate (including freezer)
> +	 * operations from running in parallel. Fail cpu online if suspend or
> +	 * hibernate has already started.
> +	 */
> +	if (!trylock_pm_sleep())

Would it be better to hook into the suspend/hibernate notifiers and
use them to exclude cpu hotplug from suspend/hibernate, instead of
trying to take pm_mutex lock like this?
Peter, I remember you pointing out in another patch's review
(http://thread.gmane.org/gmane.linux.kernel/1198312/focus=1199087)
that introducing more locks in cpu hotplug would be a bad idea. Does that
comment hold here as well, or is this fine?

Anyways, I'll start working on that other implementation that hooks onto
the suspend/hibernate notifiers and see if that works out better.

> +		return -EBUSY;
> +
>  	cpu_maps_update_begin();
> 
>  	if (cpu_hotplug_disabled) {
> @@ -377,6 +386,7 @@ int __cpuinit cpu_up(unsigned int cpu)
> 
>  out:
>  	cpu_maps_update_done();
> +	unlock_pm_sleep();
>  	return err;
>  }
> 
> 


-- 
Regards,
Srivatsa S. Bhat  <srivatsa.bhat@linux.vnet.ibm.com>
Linux Technology Center,
IBM India Systems and Technology Lab

Re: [PATCH v2 2/3] Mutually exclude cpu online and suspend/hibernate

[Peter Zijlstra]

On Mon, 2011-10-10 at 18:15 +0530, Srivatsa S. Bhat wrote:
> > +     /*
> > +      * Prevent cpu online and suspend/hibernate (including freezer)
> > +      * operations from running in parallel. Fail cpu online if suspend or
> > +      * hibernate has already started.
> > +      */
> > +     if (!trylock_pm_sleep())
> 
> Would it be better to hook into the suspend/hibernate notifiers and
> use them to exclude cpu hotplug from suspend/hibernate, instead of
> trying to take pm_mutex lock like this?
> Peter, I remember you pointing out in another patch's review
> (http://thread.gmane.org/gmane.linux.kernel/1198312/focus=1199087)
> that introducing more locks in cpu hotplug would be a bad idea. Does that
> comment hold here as well, or is this fine? 

Arguably pm_mutex is already involved in the whole hotplug dance due to
suspend using it, that said, I'm not at all familiar with the whole
suspend/hibernate side of things.

I tried having a quick look this morning but failed to find the actual
code.

I think it would be good to have an overview of the various locks and a
small description of how they interact/nest.

I just remember being very surprised about finding out the hotplug usage
of suspend/hibernate wasn't at all serialized against the regular
hotplug thingies.. (see 144060fee07e9c22e179d00819c83c86fbcbf82c).

Re: [PATCH v2 2/3] Mutually exclude cpu online and suspend/hibernate

[Srivatsa S. Bhat]

On 10/10/2011 07:56 PM, Peter Zijlstra wrote:
> On Mon, 2011-10-10 at 18:15 +0530, Srivatsa S. Bhat wrote:
>>> +     /*
>>> +      * Prevent cpu online and suspend/hibernate (including freezer)
>>> +      * operations from running in parallel. Fail cpu online if suspend or
>>> +      * hibernate has already started.
>>> +      */
>>> +     if (!trylock_pm_sleep())
>>
>> Would it be better to hook into the suspend/hibernate notifiers and
>> use them to exclude cpu hotplug from suspend/hibernate, instead of
>> trying to take pm_mutex lock like this?
>> Peter, I remember you pointing out in another patch's review
>> (http://thread.gmane.org/gmane.linux.kernel/1198312/focus=1199087)
>> that introducing more locks in cpu hotplug would be a bad idea. Does that
>> comment hold here as well, or is this fine? 
> 
> Arguably pm_mutex is already involved in the whole hotplug dance due to
> suspend using it, that said, I'm not at all familiar with the whole
> suspend/hibernate side of things.
> 
> I tried having a quick look this morning but failed to find the actual
> code.
> 
> I think it would be good to have an overview of the various locks and a
> small description of how they interact/nest.
> 

Sure. I'll put together whatever I have understood, in the form of a patch
to Documentation/power directory and post it tomorrow, for the benefit of
all.

> I just remember being very surprised about finding out the hotplug usage
> of suspend/hibernate wasn't at all serialized against the regular
> hotplug thingies.. (see 144060fee07e9c22e179d00819c83c86fbcbf82c).
> 
> 
> 

-- 
Regards,
Srivatsa S. Bhat  <srivatsa.bhat@linux.vnet.ibm.com>
Linux Technology Center,
IBM India Systems and Technology Lab

Re: [PATCH v2 0/3] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Alan Stern]

On Mon, 10 Oct 2011, Srivatsa S. Bhat wrote:

> When CPU hotplug is run along with suspend/hibernate tests using
> the pm_test framework, even at the freezer level, we hit task freezing
> failures. One such failure was reported here:
> https://lkml.org/lkml/2011/9/5/28
> 
> An excerpt of the log:
> 
>   Freezing of tasks failed after 20.01 seconds (2 tasks refusing to
>   freeze, wq_busy=0):
>   invert_cpu_stat D 0000000000000000  5304 20435  17329 0x00000084
>    ffff8801f367bab8 0000000000000046 ffff8801f367bfd8 00000000001d3a00
>    ffff8801f367a010 00000000001d3a00 00000000001d3a00 00000000001d3a00
>    ffff8801f367bfd8 00000000001d3a00 ffff880414cc6840 ffff8801f36783c0
>   Call Trace:
>    [<ffffffff81532de5>] schedule_timeout+0x235/0x320
>    [<ffffffff81532a0b>] wait_for_common+0x11b/0x170
>    [<ffffffff81532b3d>] wait_for_completion+0x1d/0x20
>    [<ffffffff81364486>] _request_firmware+0x156/0x2c0
>    [<ffffffff81364686>] request_firmware+0x16/0x20
>    [<ffffffffa01f0da0>] request_microcode_fw+0x70/0xf0 [microcode]
>    [<ffffffffa01f0390>] microcode_init_cpu+0xc0/0x100 [microcode]
>    [<ffffffffa01f14b4>] mc_cpu_callback+0x7c/0x11f [microcode]
>    [<ffffffff815393a4>] notifier_call_chain+0x94/0xd0
>    [<ffffffff8109770e>] __raw_notifier_call_chain+0xe/0x10
>    [<ffffffff8106d000>] __cpu_notify+0x20/0x40
>    [<ffffffff8152cf5b>] _cpu_up+0xc7/0x10e
>    [<ffffffff8152d07b>] cpu_up+0xd9/0xec
>    [<ffffffff8151e599>] store_online+0x99/0xd0
>    [<ffffffff81355eb0>] sysdev_store+0x20/0x30
>    [<ffffffff811f3096>] sysfs_write_file+0xe6/0x170
>    [<ffffffff8117ee50>] vfs_write+0xd0/0x1a0
>    [<ffffffff8117f024>] sys_write+0x54/0xa0
>    [<ffffffff8153df02>] system_call_fastpath+0x16/0x1b
> 
> 
> The reason behind this failure is explained below:
> 
> The x86 microcode update driver has callbacks registered for CPU hotplug
> events such as a CPU getting offlined or onlined. Things go wrong when a
> CPU hotplug stress test is carried out along with a suspend/resume operation
> running simultaneously. Upon getting a CPU_DEAD notification (for example,
> when a CPU offline occurs with tasks not frozen), the microcode callback
> frees up the microcode and invalidates it. Later, when that CPU gets onlined
> with tasks being frozen, the microcode callback (for the CPU_ONLINE_FROZEN
> event) tries to apply the microcode to the CPU; doesn't find it and hence
> depends on the (currently frozen) userspace to get the microcode again. This
> leads to the numerous "WARNING"s at drivers/base/firmware_class.c which
> eventually leads to task freezing failures in the suspend code path, as has
> been reported.
> 
> So, this patch series addresses this issue by ensuring that CPU hotplug and
> suspend/hibernate don't run in parallel, thereby fixing the task freezing
> failures.

The seems like entirely the wrong way to go about solving this problem.

The kernel shouldn't be responsible for making hotplug stress tests 
exclusive with system sleep.  Whoever is running those tests should be 
smart enough to realize what's wrong if system sleep interferes with a 
test.

Furthermore, if the entire problem is lack of CPU microcode, hasn't 
that been fixed already?  There recently was a patch to avoid releasing 
microcode after it was first loaded -- the idea being that there would 
then be no need to get the microcode from userspace again at awkward 
times while the system is resuming.

Alan Stern

Re: [PATCH v2 2/3] Mutually exclude cpu online and suspend/hibernate

[Alan Stern]

On Mon, 10 Oct 2011, Peter Zijlstra wrote:

> I just remember being very surprised about finding out the hotplug usage
> of suspend/hibernate wasn't at all serialized against the regular
> hotplug thingies.. (see 144060fee07e9c22e179d00819c83c86fbcbf82c).

It _is_ serialized against some of them... maybe not all, though.  
That's one of the purposes of the "prepare" and "complete" callback
methods in dev_pm_ops.

Alan Stern

Re: [PATCH v2 0/3] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Srivatsa S. Bhat]

On 10/10/2011 08:53 PM, Alan Stern wrote:
> On Mon, 10 Oct 2011, Srivatsa S. Bhat wrote:
> 
>> When CPU hotplug is run along with suspend/hibernate tests using
>> the pm_test framework, even at the freezer level, we hit task freezing
>> failures. One such failure was reported here:
>> https://lkml.org/lkml/2011/9/5/28
>>
>> An excerpt of the log:
>>
>>   Freezing of tasks failed after 20.01 seconds (2 tasks refusing to
>>   freeze, wq_busy=0):
>>   invert_cpu_stat D 0000000000000000  5304 20435  17329 0x00000084
>>    ffff8801f367bab8 0000000000000046 ffff8801f367bfd8 00000000001d3a00
>>    ffff8801f367a010 00000000001d3a00 00000000001d3a00 00000000001d3a00
>>    ffff8801f367bfd8 00000000001d3a00 ffff880414cc6840 ffff8801f36783c0
>>   Call Trace:
>>    [<ffffffff81532de5>] schedule_timeout+0x235/0x320
>>    [<ffffffff81532a0b>] wait_for_common+0x11b/0x170
>>    [<ffffffff81532b3d>] wait_for_completion+0x1d/0x20
>>    [<ffffffff81364486>] _request_firmware+0x156/0x2c0
>>    [<ffffffff81364686>] request_firmware+0x16/0x20
>>    [<ffffffffa01f0da0>] request_microcode_fw+0x70/0xf0 [microcode]
>>    [<ffffffffa01f0390>] microcode_init_cpu+0xc0/0x100 [microcode]
>>    [<ffffffffa01f14b4>] mc_cpu_callback+0x7c/0x11f [microcode]
>>    [<ffffffff815393a4>] notifier_call_chain+0x94/0xd0
>>    [<ffffffff8109770e>] __raw_notifier_call_chain+0xe/0x10
>>    [<ffffffff8106d000>] __cpu_notify+0x20/0x40
>>    [<ffffffff8152cf5b>] _cpu_up+0xc7/0x10e
>>    [<ffffffff8152d07b>] cpu_up+0xd9/0xec
>>    [<ffffffff8151e599>] store_online+0x99/0xd0
>>    [<ffffffff81355eb0>] sysdev_store+0x20/0x30
>>    [<ffffffff811f3096>] sysfs_write_file+0xe6/0x170
>>    [<ffffffff8117ee50>] vfs_write+0xd0/0x1a0
>>    [<ffffffff8117f024>] sys_write+0x54/0xa0
>>    [<ffffffff8153df02>] system_call_fastpath+0x16/0x1b
>>
>>
>> The reason behind this failure is explained below:
>>
>> The x86 microcode update driver has callbacks registered for CPU hotplug
>> events such as a CPU getting offlined or onlined. Things go wrong when a
>> CPU hotplug stress test is carried out along with a suspend/resume operation
>> running simultaneously. Upon getting a CPU_DEAD notification (for example,
>> when a CPU offline occurs with tasks not frozen), the microcode callback
>> frees up the microcode and invalidates it. Later, when that CPU gets onlined
>> with tasks being frozen, the microcode callback (for the CPU_ONLINE_FROZEN
>> event) tries to apply the microcode to the CPU; doesn't find it and hence
>> depends on the (currently frozen) userspace to get the microcode again. This
>> leads to the numerous "WARNING"s at drivers/base/firmware_class.c which
>> eventually leads to task freezing failures in the suspend code path, as has
>> been reported.
>>
>> So, this patch series addresses this issue by ensuring that CPU hotplug and
>> suspend/hibernate don't run in parallel, thereby fixing the task freezing
>> failures.
> 
> The seems like entirely the wrong way to go about solving this problem.
> 
> The kernel shouldn't be responsible for making hotplug stress tests 
> exclusive with system sleep.  Whoever is running those tests should be 
> smart enough to realize what's wrong if system sleep interferes with a 
> test.
> 
> Furthermore, if the entire problem is lack of CPU microcode, hasn't 
> that been fixed already?  There recently was a patch to avoid releasing 
> microcode after it was first loaded -- the idea being that there would 
> then be no need to get the microcode from userspace again at awkward 
> times while the system is resuming.
> 

Well, that was the first version of this patch itself :)
I forgot to give a link to it in the patch description:
http://thread.gmane.org/gmane.linux.kernel/1198291/focus=1200591

That was my first idea: to avoid releasing microcode after it was first
loaded. But Tejun and Borislav felt that a better way to fix the problem
would be to mutually exclude CPU hotplug and suspend/hibernate.
And later on, Borislav Acked that one-line patch on the grounds that even
though that was not the best solution for the bug, it is an optimization
in its own right.
And then I posted that one-line patch with a revised motivation:
http://thread.gmane.org/gmane.linux.kernel/1200882

-- 
Regards,
Srivatsa S. Bhat  <srivatsa.bhat@linux.vnet.ibm.com>
Linux Technology Center,
IBM India Systems and Technology Lab

Re: [PATCH v2 0/3] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Borislav Petkov]

On Mon, Oct 10, 2011 at 11:32:40AM -0400, Srivatsa S. Bhat wrote:
> > The seems like entirely the wrong way to go about solving this problem.
> > 
> > The kernel shouldn't be responsible for making hotplug stress tests 
> > exclusive with system sleep.  Whoever is running those tests should be 
> > smart enough to realize what's wrong if system sleep interferes with a 
> > test.

Yes, agreed. And more: I'm still trying to understand why a test case
like that is relevant and needs to be fixed at all. Let me re-formulate
the question: what real world scenario(s) does the case of hibernating
_while_ off- and onlining cores cover? Or are you simply doing kernel
resiliency testing and thought that offlining cores while hibernating
might make sense?

IOW, I still fail to see a strong reason for this needing fixing.

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH v2 0/3] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Tejun Heo]

Hello, Alan.

On Mon, Oct 10, 2011 at 11:23:13AM -0400, Alan Stern wrote:
> Furthermore, if the entire problem is lack of CPU microcode, hasn't 
> that been fixed already?  There recently was a patch to avoid releasing 
> microcode after it was first loaded -- the idea being that there would 
> then be no need to get the microcode from userspace again at awkward 
> times while the system is resuming.

It's a deadlock problem.  Currently, the code allows actual CPU
hotplug (which naturally requires loading microcode using userland
helper) and system suspend to race.  CPU hotplug won't be able to make
progress as microcode can't be loaded due to on-going freezing and
system suspend will eventually time out waiting for CPU hotplug to
finish.  The two operations need to be serialized.

Or are you arguing that initiating CPU hotplug operation while system
suspend is in progress is user error and we don't need to worry about
this?  Maybe that isn't such a bad idea either.

Thanks.

-- 
tejun

Re: [PATCH v2 2/3] Mutually exclude cpu online and suspend/hibernate

[Tejun Heo]

Hello,

On Mon, Oct 10, 2011 at 06:15:20PM +0530, Srivatsa S. Bhat wrote:
> Would it be better to hook into the suspend/hibernate notifiers and
> use them to exclude cpu hotplug from suspend/hibernate, instead of
> trying to take pm_mutex lock like this?

If this change is determined to be necessary, I think it would be
better to make it explicit.  Exclusion through callbacks often makes
the locking just more obscure.

Thanks.

-- 
tejun

Re: [PATCH v2 0/3] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Pavel Machek]

Hi!

> > > The seems like entirely the wrong way to go about solving this problem.
> > > 
> > > The kernel shouldn't be responsible for making hotplug stress tests 
> > > exclusive with system sleep.  Whoever is running those tests should be 
> > > smart enough to realize what's wrong if system sleep interferes with a 
> > > test.
> 
> Yes, agreed. And more: I'm still trying to understand why a test case
> like that is relevant and needs to be fixed at all. Let me re-formulate
> the question: what real world scenario(s) does the case of hibernating
> _while_ off- and onlining cores cover? Or are you simply doing kernel
> resiliency testing and thought that offlining cores while hibernating
> might make sense?

Some people want to do hibernate on battery low / UPS fail. That can
happen any time...

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v2 0/3] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Srivatsa S. Bhat]

On 10/10/2011 10:23 PM, Borislav Petkov wrote:
> On Mon, Oct 10, 2011 at 11:32:40AM -0400, Srivatsa S. Bhat wrote:
>>> The seems like entirely the wrong way to go about solving this problem.
>>>
>>> The kernel shouldn't be responsible for making hotplug stress tests 
>>> exclusive with system sleep.  Whoever is running those tests should be 
>>> smart enough to realize what's wrong if system sleep interferes with a 
>>> test.
> 
> Yes, agreed. And more: I'm still trying to understand why a test case
> like that is relevant and needs to be fixed at all. Let me re-formulate
> the question: what real world scenario(s) does the case of hibernating
> _while_ off- and onlining cores cover? Or are you simply doing kernel
> resiliency testing and thought that offlining cores while hibernating
> might make sense?
> 

Actually, my whole intention while coming up with this test case was to
test the stability/correct operation of the entire suspend/resume call
path. And since I found that cpu hotplug is used in that call path I
thought of giving it a whirl and finding out if there were any cases that
lead to freezing failures and the like. And I did uncover a couple of cases,
one after the other.
But I do agree that offlining and onlining CPUs while suspending might
not seem all that useful or even wise, but like I said, it was designed to
bring out such problematic race conditions.

So, in the interest of making the important components involved in
suspend/resume call path (namely cpu hotplug) more robust and stable,
I think it makes sense to fix any issue we hit (atleast when we
practically hit it and it is proved that such a scenario is no longer
hypothetical).

For that, we can either go with the simple one-line fix that I posted
earlier (which has got another motivation now, thanks to Borislav) or
with this elaborate solution, whichever seems better/worthwhile.

If it is still strongly felt that this "bug" is not worth fixing with such
mutual exclusion schemes, it will still get solved anyway by applying that
one-line patch.

> IOW, I still fail to see a strong reason for this needing fixing.
> 
> Thanks.
> 


-- 
Regards,
Srivatsa S. Bhat  <srivatsa.bhat@linux.vnet.ibm.com>
Linux Technology Center,
IBM India Systems and Technology Lab

Re: [PATCH v2 0/3] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Borislav Petkov]

On Mon, Oct 10, 2011 at 01:30:34PM -0400, Srivatsa S. Bhat wrote:
> But I do agree that offlining and onlining CPUs while suspending might
> not seem all that useful or even wise, but like I said, it was designed to
> bring out such problematic race conditions.
> 
> So, in the interest of making the important components involved in
> suspend/resume call path (namely cpu hotplug) more robust and stable,
> I think it makes sense to fix any issue we hit (atleast when we
> practically hit it and it is proved that such a scenario is no longer
> hypothetical).
> 
> For that, we can either go with the simple one-line fix that I posted
> earlier (which has got another motivation now, thanks to Borislav) or
> with this elaborate solution, whichever seems better/worthwhile.
>
> If it is still strongly felt that this "bug" is not worth fixing with such
> mutual exclusion schemes, it will still get solved anyway by applying that
> one-line patch.

Well, this is easy: the oneliner is needed anyway for removing
unnecessary ucode reloading and since it fixes your test cases _and_ is
_simpler_, the whole deal is a no brainer.

And although Pavel had a valid concern there about suspending when low
on battery/failing UPSs and, AFAIUI, it might happen that the machine
automatically suspends while you offline one of your cores, I can't find
it in me to care about a case like that, sorry.

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH v2 0/3] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[tj]

Hello,

On Mon, Oct 10, 2011 at 07:53:36PM +0200, Borislav Petkov wrote:
> On Mon, Oct 10, 2011 at 01:30:34PM -0400, Srivatsa S. Bhat wrote:
> > But I do agree that offlining and onlining CPUs while suspending might
> > not seem all that useful or even wise, but like I said, it was designed to
> > bring out such problematic race conditions.
> > 
> > So, in the interest of making the important components involved in
> > suspend/resume call path (namely cpu hotplug) more robust and stable,
> > I think it makes sense to fix any issue we hit (atleast when we
> > practically hit it and it is proved that such a scenario is no longer
> > hypothetical).
> > 
> > For that, we can either go with the simple one-line fix that I posted
> > earlier (which has got another motivation now, thanks to Borislav) or
> > with this elaborate solution, whichever seems better/worthwhile.
> >
> > If it is still strongly felt that this "bug" is not worth fixing with such
> > mutual exclusion schemes, it will still get solved anyway by applying that
> > one-line patch.
> 
> Well, this is easy: the oneliner is needed anyway for removing
> unnecessary ucode reloading and since it fixes your test cases _and_ is
> _simpler_, the whole deal is a no brainer.

Maybe I'm confused but is that patch correct for actual CPU hotplug
case?  If not, what's the point in doing that?  What are we gonna do
after six month some people come up with "CPU hotplug fails to load
new microcode for the new CPU"?  The invalidation code is there for a
reason.  The CPU is going away and the microcode tied to the CPU
should go away too.  If somebody is sure that microcode don't need to
be changed once loaded, then all's good and dandy but that's not the
case here, right?

If you want to optimize away microcode unloading during
suspend/resume, the RTTD is doing revalidation / reload during
CPU_ONLINE as necessary.

If this use case doesn't really matter too much to anyone, just
leaving it alone would be better than adding band aid which can lead
to very obscure issues down the road (oooh, that microcode shouldn't
have been loaded to that cpu).

Thanks.

-- 
tejun

Re: [PATCH v2 0/3] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Borislav Petkov]

Hi Tejun,

On Mon, Oct 10, 2011 at 02:08:48PM -0400, tj@kernel.org wrote:
> Maybe I'm confused but is that patch correct for actual CPU hotplug
> case?  If not, what's the point in doing that?  What are we gonna do
> after six month some people come up with "CPU hotplug fails to load
> new microcode for the new CPU"?

Ok, first of all, we still will load ucode on the onlining path - we're
simply not going to reload it when the CPU has gone offline and onlined
again. For that case people should simply reload the module so that
ucode on _all_ CPUs is updated pretty much at same time.

Now, actually, the ucode driver shouldn't even exist as it is - ucode
should be loaded much much earlier when the cores are being trampolined.
Normally, ucode is loaded by BIOS and I imagine this facility here
exists for the sole purpose to apply ucode only when there's no new BIOS
available.

So, here's what ucode handling should actually be, IMHO:

* load ucode very early during booting of each CPU

* keep ucode in memory in case not all cores have been onlined from the
get-go to apply when they're onlined later

* do NOT reapply it when cores are off-/onlined because there's no need
(cores retain ucode when we offline them (basically, they enter C1,
probably C3 on Intel or similar so no problem)).

* when you have new ucode image, trigger a replacement of the image in
memory and subsequently trigger a re-application of the new ucode (sysfs
write, whatever).

> The invalidation code is there for a reason.

... and that reason being?

> The CPU is going away and the microcode tied to the CPU should go away
> too.

This is what I don't understand - as I said earlier, ucode is not
something you get on a monthly basis: you get only a very few updates
and that's it in the majority of the cases. So users going the trouble
of reloading the microcode.ko module a very few times (if ever!) during
a system's lifetime shouldn't be an issue.

> If somebody is sure that microcode don't need to be changed once
> loaded, then all's good and dandy but that's not the case here, right?

Well, basically the current situation didn't change the ucode - it
simply reloaded the same image from before going offline.

See, there's this another problem with what we have right now: imagine
you've just updated the ucode image on disk and offline only a subset of
the cores. Then you online them again and they now get the newer ucode
image while the others still run the old ucode. This could explode or
could not, one thing's for sure: all bets are off. If we don't reload it
on hotplug, we're fine - only module reload triggers the ucode update in
a fairly synchronized manner.

> If you want to optimize away microcode unloading during
> suspend/resume, the RTTD is doing revalidation / reload during
> CPU_ONLINE as necessary.

see above.

> If this use case doesn't really matter too much to anyone, just
> leaving it alone would be better than adding band aid which can lead
> to very obscure issues down the road (oooh, that microcode shouldn't
> have been loaded to that cpu).

I'd like to actually hear someone justify such a requirement.

I hope I'm making some sense here.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH v2 0/3] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Srivatsa S. Bhat]

On 10/11/2011 12:04 AM, Borislav Petkov wrote:
> Hi Tejun,
> 
> On Mon, Oct 10, 2011 at 02:08:48PM -0400, tj@kernel.org wrote:
>> Maybe I'm confused but is that patch correct for actual CPU hotplug
>> case?  If not, what's the point in doing that?  What are we gonna do
>> after six month some people come up with "CPU hotplug fails to load
>> new microcode for the new CPU"?
> 
[...]
> 
>> If somebody is sure that microcode don't need to be changed once
>> loaded, then all's good and dandy but that's not the case here, right?
> 
> Well, basically the current situation didn't change the ucode - it
> simply reloaded the same image from before going offline.
> 
> See, there's this another problem with what we have right now: imagine
> you've just updated the ucode image on disk and offline only a subset of
> the cores. Then you online them again and they now get the newer ucode
> image while the others still run the old ucode. This could explode or
> could not, one thing's for sure: all bets are off. If we don't reload it
> on hotplug, we're fine - only module reload triggers the ucode update in
> a fairly synchronized manner.
> 

This one makes a lot of sense to me. I hadn't thought of it before.

>> If you want to optimize away microcode unloading during
>> suspend/resume, the RTTD is doing revalidation / reload during
>> CPU_ONLINE as necessary.
> 
> see above.
> 
>> If this use case doesn't really matter too much to anyone, just
>> leaving it alone would be better than adding band aid which can lead
>> to very obscure issues down the road (oooh, that microcode shouldn't
>> have been loaded to that cpu).
> 
> I'd like to actually hear someone justify such a requirement.
> 
> I hope I'm making some sense here.
> 


-- 
Regards,
Srivatsa S. Bhat  <srivatsa.bhat@linux.vnet.ibm.com>
Linux Technology Center,
IBM India Systems and Technology Lab

Re: [PATCH v2 0/3] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[tj]

Hello,

On Mon, Oct 10, 2011 at 08:34:43PM +0200, Borislav Petkov wrote:
> On Mon, Oct 10, 2011 at 02:08:48PM -0400, tj@kernel.org wrote:
> > Maybe I'm confused but is that patch correct for actual CPU hotplug
> > case?  If not, what's the point in doing that?  What are we gonna do
> > after six month some people come up with "CPU hotplug fails to load
> > new microcode for the new CPU"?
> 
> Ok, first of all, we still will load ucode on the onlining path - we're
> simply not going to reload it when the CPU has gone offline and onlined
> again. For that case people should simply reload the module so that
> ucode on _all_ CPUs is updated pretty much at same time.

I was thinking about hot-swap.  It might be pretty unlikely at this
point but I don't think excluding that is a good idea.  x86 is used in
pretty highend too these days.  Again, I don't know much about how
ucodes are supposed to be managed and maybe it's true that we don't
need new one at all even after hotswap.  If that's the case, state it
clearly and it's all fine.

> > The invalidation code is there for a reason.
> 
> ... and that reason being?

Again, the CPU for the microcode is going away?  It's something tied
to a device and the device is going away.  It's a basic correctness
issue.  It at least needs to be revalidated.

> > If somebody is sure that microcode don't need to be changed once
> > loaded, then all's good and dandy but that's not the case here, right?
> 
> Well, basically the current situation didn't change the ucode - it
> simply reloaded the same image from before going offline.
> 
> See, there's this another problem with what we have right now: imagine
> you've just updated the ucode image on disk and offline only a subset of
> the cores. Then you online them again and they now get the newer ucode
> image while the others still run the old ucode. This could explode or
> could not, one thing's for sure: all bets are off. If we don't reload it
> on hotplug, we're fine - only module reload triggers the ucode update in
> a fairly synchronized manner.

Yeah, loading different ucodes to different cores sounds pretty scary.
I suppose we'll need to distinguish physical hotplugs from logical
ones.

Hmm... is it possible to tell whether the core coming online is the
same one as the last time?  If that's possible, the problem becomes
pretty simple and we can simply tell people who are mixing
suspend/hibernate with physical hotplug that they're crazy.

Thanks.

-- 
tejun

Re: [PATCH v2 0/3] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Srivatsa S. Bhat]

On 10/11/2011 12:23 AM, tj@kernel.org wrote:
> Hello,
> 
> On Mon, Oct 10, 2011 at 08:34:43PM +0200, Borislav Petkov wrote:
>> On Mon, Oct 10, 2011 at 02:08:48PM -0400, tj@kernel.org wrote:
>>> Maybe I'm confused but is that patch correct for actual CPU hotplug
>>> case?  If not, what's the point in doing that?  What are we gonna do
>>> after six month some people come up with "CPU hotplug fails to load
>>> new microcode for the new CPU"?
>>
>> Ok, first of all, we still will load ucode on the onlining path - we're
>> simply not going to reload it when the CPU has gone offline and onlined
>> again. For that case people should simply reload the module so that
>> ucode on _all_ CPUs is updated pretty much at same time.
> 
> I was thinking about hot-swap.  It might be pretty unlikely at this
> point but I don't think excluding that is a good idea.  x86 is used in
> pretty highend too these days.  Again, I don't know much about how
> ucodes are supposed to be managed and maybe it's true that we don't
> need new one at all even after hotswap.  If that's the case, state it
> clearly and it's all fine.
> 
>>> The invalidation code is there for a reason.
>>
>> ... and that reason being?
> 
> Again, the CPU for the microcode is going away?  It's something tied
> to a device and the device is going away.  It's a basic correctness
> issue.  It at least needs to be revalidated.
> 
>>> If somebody is sure that microcode don't need to be changed once
>>> loaded, then all's good and dandy but that's not the case here, right?
>>
>> Well, basically the current situation didn't change the ucode - it
>> simply reloaded the same image from before going offline.
>>
>> See, there's this another problem with what we have right now: imagine
>> you've just updated the ucode image on disk and offline only a subset of
>> the cores. Then you online them again and they now get the newer ucode
>> image while the others still run the old ucode. This could explode or
>> could not, one thing's for sure: all bets are off. If we don't reload it
>> on hotplug, we're fine - only module reload triggers the ucode update in
>> a fairly synchronized manner.
> 
> Yeah, loading different ucodes to different cores sounds pretty scary.
> I suppose we'll need to distinguish physical hotplugs from logical
> ones.
> 
> Hmm... is it possible to tell whether the core coming online is the
> same one as the last time?  If that's possible, the problem becomes
> pretty simple and we can simply tell people who are mixing
> suspend/hibernate with physical hotplug that they're crazy.
> 

I think that is pretty easy, atleast from a microcode revision standpoint:
the collect_cpu_info() function (defined in arch/x86/kernel/microcode_core.c
and arch/x86/kernel/microcode_intel.c or ..._amd.c) can be used for that
purpose. Am I right Boris?

-- 
Regards,
Srivatsa S. Bhat  <srivatsa.bhat@linux.vnet.ibm.com>
Linux Technology Center,
IBM India Systems and Technology Lab

Re: [PATCH v2 0/3] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Borislav Petkov]

On Mon, Oct 10, 2011 at 03:00:18PM -0400, Srivatsa S. Bhat wrote:
> > Hmm... is it possible to tell whether the core coming online is the
> > same one as the last time?  If that's possible, the problem becomes
> > pretty simple and we can simply tell people who are mixing
> > suspend/hibernate with physical hotplug that they're crazy.
> > 
> 
> I think that is pretty easy, atleast from a microcode revision standpoint:
> the collect_cpu_info() function (defined in arch/x86/kernel/microcode_core.c
> and arch/x86/kernel/microcode_intel.c or ..._amd.c) can be used for that
> purpose. Am I right Boris?

Correct, on AMD the PATCH_LEVEL MSR (0x8b) contains the ... doh, current
patch level of the core, i.e. the applied ucode version.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH v2 0/3] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[tj]

On Mon, Oct 10, 2011 at 10:29:13PM +0200, Borislav Petkov wrote:
> > Hmm... is it possible to tell whether the core coming online is the
> > same one as the last time?  If that's possible, the problem becomes
> > pretty simple and we can simply tell people who are mixing
> > suspend/hibernate with physical hotplug that they're crazy.
> 
> It is possible to compare ucode revisions of the core coming online and
> what we have in memory and do the update only if it is necessary. We
> still can keep the ucode image without the need for revalidation (and
> reloading).
> 
> Better?

Yeah yeah, I don't know how ucode is matched to specific CPU but just
store that identification of the last CPU and on ONLINE if it stays
the same, use the existing one.  If not, request new one.  It
shouldn't be difficult && solves the issue at hand while not breaking
hot-swap case.

Thanks.

-- 
tejun

Re: [PATCH v2 0/3] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Peter Zijlstra]

On Mon, 2011-10-10 at 18:53 +0200, Borislav Petkov wrote:
> 
> Yes, agreed. And more: I'm still trying to understand why a test case
> like that is relevant and needs to be fixed at all. Let me re-formulate
> the question: what real world scenario(s) does the case of hibernating
> _while_ off- and onlining cores cover? Or are you simply doing kernel
> resiliency testing and thought that offlining cores while hibernating
> might make sense? 

It can happen, therefore it will. We had better deal with it.

Re: [PATCH v2 2/3] Mutually exclude cpu online and suspend/hibernate

[Peter Zijlstra]

On Mon, 2011-10-10 at 10:00 -0700, Tejun Heo wrote:

> On Mon, Oct 10, 2011 at 06:15:20PM +0530, Srivatsa S. Bhat wrote:
> > Would it be better to hook into the suspend/hibernate notifiers and
> > use them to exclude cpu hotplug from suspend/hibernate, instead of
> > trying to take pm_mutex lock like this?
> 
> If this change is determined to be necessary, I think it would be
> better to make it explicit.  Exclusion through callbacks often makes
> the locking just more obscure.

Agreed.

Re: [PATCH v2 2/3] Mutually exclude cpu online and suspend/hibernate

[Srivatsa S. Bhat]

On 10/11/2011 02:48 PM, Peter Zijlstra wrote:
> On Mon, 2011-10-10 at 10:00 -0700, Tejun Heo wrote:
> 
>> On Mon, Oct 10, 2011 at 06:15:20PM +0530, Srivatsa S. Bhat wrote:
>>> Would it be better to hook into the suspend/hibernate notifiers and
>>> use them to exclude cpu hotplug from suspend/hibernate, instead of
>>> trying to take pm_mutex lock like this?
>>
>> If this change is determined to be necessary, I think it would be
>> better to make it explicit.  Exclusion through callbacks often makes
>> the locking just more obscure.
> 
> Agreed.
> 

That change is not exactly necessary. I was only wondering if that would be
a cleaner way of dealing with this problem, in the sense that instead of
making this look like a workaround to some bug, we could make it look well-
designed - the PM notifies that its going to suspend/hibernate; so whoever
wants to take actions is free to do so - and it so happens that the cpu
hotplug wants to get mutually excluded from suspend/hibernate and it
implements that exclusion ie., disables further cpu hotplug from happening
till suspend/hibernate is complete and blocks suspend/hibernate till it is
done with an ongoing cpu hotplug operation. And then the suspend/hibernate
continues normally, after returning from the notification path...

But now I see that, apart from probably making the locking more obscure, it
has another disadvantage: the code will get bloated unnecessarily, considering:

    - defining a callback for suspend/hibernate notifier in the cpu hotplug
      code
    - registering/unregistering it with the PM code
    - then implementing the mutual exclusion in that callback

And this current patchset achieves the same end goal without bloating the
code and without obscuring the locking.

So, probably its not worth hooking into the notifiers after all...
Hence we can go with this patchset itself.

-- 
Regards,
Srivatsa S. Bhat  <srivatsa.bhat@linux.vnet.ibm.com>
Linux Technology Center,
IBM India Systems and Technology Lab

Re: [PATCH v2 2/3] Mutually exclude cpu online and suspend/hibernate

[Srivatsa S. Bhat]

On 10/10/2011 08:46 PM, Srivatsa S. Bhat wrote:
> On 10/10/2011 07:56 PM, Peter Zijlstra wrote:
>> On Mon, 2011-10-10 at 18:15 +0530, Srivatsa S. Bhat wrote:
>>>> +     /*
>>>> +      * Prevent cpu online and suspend/hibernate (including freezer)
>>>> +      * operations from running in parallel. Fail cpu online if suspend or
>>>> +      * hibernate has already started.
>>>> +      */
>>>> +     if (!trylock_pm_sleep())
>>>
>>> Would it be better to hook into the suspend/hibernate notifiers and
>>> use them to exclude cpu hotplug from suspend/hibernate, instead of
>>> trying to take pm_mutex lock like this?
>>> Peter, I remember you pointing out in another patch's review
>>> (http://thread.gmane.org/gmane.linux.kernel/1198312/focus=1199087)
>>> that introducing more locks in cpu hotplug would be a bad idea. Does that
>>> comment hold here as well, or is this fine? 
>>
>> Arguably pm_mutex is already involved in the whole hotplug dance due to
>> suspend using it, that said, I'm not at all familiar with the whole
>> suspend/hibernate side of things.
>>
>> I tried having a quick look this morning but failed to find the actual
>> code.
>>
>> I think it would be good to have an overview of the various locks and a
>> small description of how they interact/nest.
>>
> 
> Sure. I'll put together whatever I have understood, in the form of a patch
> to Documentation/power directory and post it tomorrow, for the benefit of
> all.
> 

Here it is, just as promised :-)
http://lkml.org/lkml/2011/10/11/393

>> I just remember being very surprised about finding out the hotplug usage
>> of suspend/hibernate wasn't at all serialized against the regular
>> hotplug thingies.. (see 144060fee07e9c22e179d00819c83c86fbcbf82c).
>>
>>
>>
> 


-- 
Regards,
Srivatsa S. Bhat  <srivatsa.bhat@linux.vnet.ibm.com>
Linux Technology Center,
IBM India Systems and Technology Lab

Re: [PATCH v2 2/3] Mutually exclude cpu online and suspend/hibernate

[Rafael J. Wysocki]

On Tuesday, October 11, 2011, Srivatsa S. Bhat wrote:
> On 10/10/2011 08:46 PM, Srivatsa S. Bhat wrote:
> > On 10/10/2011 07:56 PM, Peter Zijlstra wrote:
> >> On Mon, 2011-10-10 at 18:15 +0530, Srivatsa S. Bhat wrote:
> >>>> +     /*
> >>>> +      * Prevent cpu online and suspend/hibernate (including freezer)
> >>>> +      * operations from running in parallel. Fail cpu online if suspend or
> >>>> +      * hibernate has already started.
> >>>> +      */
> >>>> +     if (!trylock_pm_sleep())
> >>>
> >>> Would it be better to hook into the suspend/hibernate notifiers and
> >>> use them to exclude cpu hotplug from suspend/hibernate, instead of
> >>> trying to take pm_mutex lock like this?
> >>> Peter, I remember you pointing out in another patch's review
> >>> (http://thread.gmane.org/gmane.linux.kernel/1198312/focus=1199087)
> >>> that introducing more locks in cpu hotplug would be a bad idea. Does that
> >>> comment hold here as well, or is this fine? 
> >>
> >> Arguably pm_mutex is already involved in the whole hotplug dance due to
> >> suspend using it, that said, I'm not at all familiar with the whole
> >> suspend/hibernate side of things.
> >>
> >> I tried having a quick look this morning but failed to find the actual
> >> code.
> >>
> >> I think it would be good to have an overview of the various locks and a
> >> small description of how they interact/nest.
> >>
> > 
> > Sure. I'll put together whatever I have understood, in the form of a patch
> > to Documentation/power directory and post it tomorrow, for the benefit of
> > all.
> > 
> 
> Here it is, just as promised :-)
> http://lkml.org/lkml/2011/10/11/393

Well, I have an idea.

Why don't we make drivers/base/cpu.c:store_online() take pm_mutex
in addition to calling cpu_hotplug_driver_lock()?  This at least
will make the interface mutually exclusive with suspend/hibernation.

Thanks,
Rafael

Re: [PATCH v2 2/3] Mutually exclude cpu online and suspend/hibernate

[Srivatsa S. Bhat]

On 10/12/2011 03:26 AM, Rafael J. Wysocki wrote:
> On Tuesday, October 11, 2011, Srivatsa S. Bhat wrote:
>> On 10/10/2011 08:46 PM, Srivatsa S. Bhat wrote:
>>> On 10/10/2011 07:56 PM, Peter Zijlstra wrote:
>>>> On Mon, 2011-10-10 at 18:15 +0530, Srivatsa S. Bhat wrote:
>>>>>> +     /*
>>>>>> +      * Prevent cpu online and suspend/hibernate (including freezer)
>>>>>> +      * operations from running in parallel. Fail cpu online if suspend or
>>>>>> +      * hibernate has already started.
>>>>>> +      */
>>>>>> +     if (!trylock_pm_sleep())
>>>>>
>>>>> Would it be better to hook into the suspend/hibernate notifiers and
>>>>> use them to exclude cpu hotplug from suspend/hibernate, instead of
>>>>> trying to take pm_mutex lock like this?
>>>>> Peter, I remember you pointing out in another patch's review
>>>>> (http://thread.gmane.org/gmane.linux.kernel/1198312/focus=1199087)
>>>>> that introducing more locks in cpu hotplug would be a bad idea. Does that
>>>>> comment hold here as well, or is this fine? 
>>>>
>>>> Arguably pm_mutex is already involved in the whole hotplug dance due to
>>>> suspend using it, that said, I'm not at all familiar with the whole
>>>> suspend/hibernate side of things.
>>>>
>>>> I tried having a quick look this morning but failed to find the actual
>>>> code.
>>>>
>>>> I think it would be good to have an overview of the various locks and a
>>>> small description of how they interact/nest.
>>>>
>>>
>>> Sure. I'll put together whatever I have understood, in the form of a patch
>>> to Documentation/power directory and post it tomorrow, for the benefit of
>>> all.
>>>
>>
>> Here it is, just as promised :-)
>> http://lkml.org/lkml/2011/10/11/393
> 
> Well, I have an idea.
> 
> Why don't we make drivers/base/cpu.c:store_online() take pm_mutex
> in addition to calling cpu_hotplug_driver_lock()?  This at least
> will make the interface mutually exclusive with suspend/hibernation.
> 

Oh, no no.. We shouldn't be doing that even though it seems very
innocuous, because of a subtle reason: the memory hotplug code called
in cpu_up() tries to acquire pm_mutex! So we will end up deadlocking
ourselves, due to recursive locking!
See kernel/cpu.c: cpu_up() calls mem_online_node() [defined in
mm/memory_hotplug.c/mem_online_node() which calls lock_memory_hotplug()
which internally calls lock_system_sleep(), which is where it tries
to get pm_mutex].

So this patchset implements the mutual exclusion in the cpu_up() function
(i.e., a little bit deeper down the road than store_online() ) and solves
the problem.

I thought I had put a comment there in my code warning about this locking
order thing, but I must have removed it in the last minute, just before
posting it....

-- 
Regards,
Srivatsa S. Bhat  <srivatsa.bhat@linux.vnet.ibm.com>
Linux Technology Center,
IBM India Systems and Technology Lab

Re: [PATCH v2 2/3] Mutually exclude cpu online and suspend/hibernate

[Rafael J. Wysocki]

On Wednesday, October 12, 2011, Srivatsa S. Bhat wrote:
> On 10/12/2011 03:26 AM, Rafael J. Wysocki wrote:
> > On Tuesday, October 11, 2011, Srivatsa S. Bhat wrote:
> >> On 10/10/2011 08:46 PM, Srivatsa S. Bhat wrote:
> >>> On 10/10/2011 07:56 PM, Peter Zijlstra wrote:
> >>>> On Mon, 2011-10-10 at 18:15 +0530, Srivatsa S. Bhat wrote:
> >>>>>> +     /*
> >>>>>> +      * Prevent cpu online and suspend/hibernate (including freezer)
> >>>>>> +      * operations from running in parallel. Fail cpu online if suspend or
> >>>>>> +      * hibernate has already started.
> >>>>>> +      */
> >>>>>> +     if (!trylock_pm_sleep())
> >>>>>
> >>>>> Would it be better to hook into the suspend/hibernate notifiers and
> >>>>> use them to exclude cpu hotplug from suspend/hibernate, instead of
> >>>>> trying to take pm_mutex lock like this?
> >>>>> Peter, I remember you pointing out in another patch's review
> >>>>> (http://thread.gmane.org/gmane.linux.kernel/1198312/focus=1199087)
> >>>>> that introducing more locks in cpu hotplug would be a bad idea. Does that
> >>>>> comment hold here as well, or is this fine? 
> >>>>
> >>>> Arguably pm_mutex is already involved in the whole hotplug dance due to
> >>>> suspend using it, that said, I'm not at all familiar with the whole
> >>>> suspend/hibernate side of things.
> >>>>
> >>>> I tried having a quick look this morning but failed to find the actual
> >>>> code.
> >>>>
> >>>> I think it would be good to have an overview of the various locks and a
> >>>> small description of how they interact/nest.
> >>>>
> >>>
> >>> Sure. I'll put together whatever I have understood, in the form of a patch
> >>> to Documentation/power directory and post it tomorrow, for the benefit of
> >>> all.
> >>>
> >>
> >> Here it is, just as promised :-)
> >> http://lkml.org/lkml/2011/10/11/393
> > 
> > Well, I have an idea.
> > 
> > Why don't we make drivers/base/cpu.c:store_online() take pm_mutex
> > in addition to calling cpu_hotplug_driver_lock()?  This at least
> > will make the interface mutually exclusive with suspend/hibernation.
> > 
> 
> Oh, no no.. We shouldn't be doing that even though it seems very
> innocuous, because of a subtle reason: the memory hotplug code called
> in cpu_up() tries to acquire pm_mutex! So we will end up deadlocking
> ourselves, due to recursive locking!

So, you're referring to mem_online_node().  Is it actually used
from any place other than cpu_up()?  Perhaps we can remove the
lock_system_sleep() from lock_memory_hotplug() if store_online()
acquires pm_mutex?  And if we can't, then why exactly?

> See kernel/cpu.c: cpu_up() calls mem_online_node() [defined in
> mm/memory_hotplug.c/mem_online_node() which calls lock_memory_hotplug()
> which internally calls lock_system_sleep(), which is where it tries
> to get pm_mutex].
> 
> So this patchset implements the mutual exclusion in the cpu_up() function
> (i.e., a little bit deeper down the road than store_online() ) and solves
> the problem.

Which is not exactly the right place to do that as Peter has already
indicated.  What you want is really the CPU hotplug interface to be
mutually exclusive with the suspend/hibernation interface, not the
CPU hotplug itself to be mutually exclusive with suspend/hibernation.

Thanks,
Rafael

Re: [PATCH v2 2/3] Mutually exclude cpu online and suspend/hibernate

[Srivatsa S. Bhat]

On 10/13/2011 01:01 AM, Rafael J. Wysocki wrote:
> On Wednesday, October 12, 2011, Srivatsa S. Bhat wrote:
>> On 10/12/2011 03:26 AM, Rafael J. Wysocki wrote:
>>> On Tuesday, October 11, 2011, Srivatsa S. Bhat wrote:
>>>> On 10/10/2011 08:46 PM, Srivatsa S. Bhat wrote:
>>>>> On 10/10/2011 07:56 PM, Peter Zijlstra wrote:
>>>>>> On Mon, 2011-10-10 at 18:15 +0530, Srivatsa S. Bhat wrote:
>>>>>>>> +     /*
>>>>>>>> +      * Prevent cpu online and suspend/hibernate (including freezer)
>>>>>>>> +      * operations from running in parallel. Fail cpu online if suspend or
>>>>>>>> +      * hibernate has already started.
>>>>>>>> +      */
>>>>>>>> +     if (!trylock_pm_sleep())
>>>>>>>
>>>>>>> Would it be better to hook into the suspend/hibernate notifiers and
>>>>>>> use them to exclude cpu hotplug from suspend/hibernate, instead of
>>>>>>> trying to take pm_mutex lock like this?
>>>>>>> Peter, I remember you pointing out in another patch's review
>>>>>>> (http://thread.gmane.org/gmane.linux.kernel/1198312/focus=1199087)
>>>>>>> that introducing more locks in cpu hotplug would be a bad idea. Does that
>>>>>>> comment hold here as well, or is this fine? 
>>>>>>
>>>>>> Arguably pm_mutex is already involved in the whole hotplug dance due to
>>>>>> suspend using it, that said, I'm not at all familiar with the whole
>>>>>> suspend/hibernate side of things.
>>>>>>
>>>>>> I tried having a quick look this morning but failed to find the actual
>>>>>> code.
>>>>>>
>>>>>> I think it would be good to have an overview of the various locks and a
>>>>>> small description of how they interact/nest.
>>>>>>
>>>>>
>>>>> Sure. I'll put together whatever I have understood, in the form of a patch
>>>>> to Documentation/power directory and post it tomorrow, for the benefit of
>>>>> all.
>>>>>
>>>>
>>>> Here it is, just as promised :-)
>>>> http://lkml.org/lkml/2011/10/11/393
>>>
>>> Well, I have an idea.
>>>
>>> Why don't we make drivers/base/cpu.c:store_online() take pm_mutex
>>> in addition to calling cpu_hotplug_driver_lock()?  This at least
>>> will make the interface mutually exclusive with suspend/hibernation.
>>>
>>
>> Oh, no no.. We shouldn't be doing that even though it seems very
>> innocuous, because of a subtle reason: the memory hotplug code called
>> in cpu_up() tries to acquire pm_mutex! So we will end up deadlocking
>> ourselves, due to recursive locking!
> 
> So, you're referring to mem_online_node().  Is it actually used
> from any place other than cpu_up()?  Perhaps we can remove the
> lock_system_sleep() from lock_memory_hotplug() if store_online()
> acquires pm_mutex?  And if we can't, then why exactly?
> 

I didn't find any other place where mem_online_node() is called. But I'll
check it more thoroughly to be sure. If cpu_up() is the only place where
it is called, then I think we can do what you said above. But doesn't it
seem to be more intrusive than what my patch does? I mean, what if something
breaks because of that avoidable modification to memory hotplug code?

>> See kernel/cpu.c: cpu_up() calls mem_online_node() [defined in
>> mm/memory_hotplug.c/mem_online_node() which calls lock_memory_hotplug()
>> which internally calls lock_system_sleep(), which is where it tries
>> to get pm_mutex].
>>
>> So this patchset implements the mutual exclusion in the cpu_up() function
>> (i.e., a little bit deeper down the road than store_online() ) and solves
>> the problem.
> 
> Which is not exactly the right place to do that as Peter has already
> indicated.  What you want is really the CPU hotplug interface to be
> mutually exclusive with the suspend/hibernation interface, not the
> CPU hotplug itself to be mutually exclusive with suspend/hibernation.
> 

Oh, is that what Peter meant? I didn't derive that meaning from what
he said...
Making the interfaces themselves mutually exclusive is also a good idea.
But please see my concerns above, about its implementation aspect.

Since my patch solves the problem with less intrusion, and produces
roughly the same effect as this interface exclusion idea, why not go with
it?

Or am I missing something here?

-- 
Regards,
Srivatsa S. Bhat  <srivatsa.bhat@linux.vnet.ibm.com>
Linux Technology Center,
IBM India Systems and Technology Lab

Re: [PATCH v2 2/3] Mutually exclude cpu online and suspend/hibernate

[Rafael J. Wysocki]

On Wednesday, October 12, 2011, Srivatsa S. Bhat wrote:
> On 10/13/2011 01:01 AM, Rafael J. Wysocki wrote:
> > On Wednesday, October 12, 2011, Srivatsa S. Bhat wrote:
> >> On 10/12/2011 03:26 AM, Rafael J. Wysocki wrote:
> >>> On Tuesday, October 11, 2011, Srivatsa S. Bhat wrote:
> >>>> On 10/10/2011 08:46 PM, Srivatsa S. Bhat wrote:
> >>>>> On 10/10/2011 07:56 PM, Peter Zijlstra wrote:
> >>>>>> On Mon, 2011-10-10 at 18:15 +0530, Srivatsa S. Bhat wrote:
> >>>>>>>> +     /*
> >>>>>>>> +      * Prevent cpu online and suspend/hibernate (including freezer)
> >>>>>>>> +      * operations from running in parallel. Fail cpu online if suspend or
> >>>>>>>> +      * hibernate has already started.
> >>>>>>>> +      */
> >>>>>>>> +     if (!trylock_pm_sleep())
> >>>>>>>
> >>>>>>> Would it be better to hook into the suspend/hibernate notifiers and
> >>>>>>> use them to exclude cpu hotplug from suspend/hibernate, instead of
> >>>>>>> trying to take pm_mutex lock like this?
> >>>>>>> Peter, I remember you pointing out in another patch's review
> >>>>>>> (http://thread.gmane.org/gmane.linux.kernel/1198312/focus=1199087)
> >>>>>>> that introducing more locks in cpu hotplug would be a bad idea. Does that
> >>>>>>> comment hold here as well, or is this fine? 
> >>>>>>
> >>>>>> Arguably pm_mutex is already involved in the whole hotplug dance due to
> >>>>>> suspend using it, that said, I'm not at all familiar with the whole
> >>>>>> suspend/hibernate side of things.
> >>>>>>
> >>>>>> I tried having a quick look this morning but failed to find the actual
> >>>>>> code.
> >>>>>>
> >>>>>> I think it would be good to have an overview of the various locks and a
> >>>>>> small description of how they interact/nest.
> >>>>>>
> >>>>>
> >>>>> Sure. I'll put together whatever I have understood, in the form of a patch
> >>>>> to Documentation/power directory and post it tomorrow, for the benefit of
> >>>>> all.
> >>>>>
> >>>>
> >>>> Here it is, just as promised :-)
> >>>> http://lkml.org/lkml/2011/10/11/393
> >>>
> >>> Well, I have an idea.
> >>>
> >>> Why don't we make drivers/base/cpu.c:store_online() take pm_mutex
> >>> in addition to calling cpu_hotplug_driver_lock()?  This at least
> >>> will make the interface mutually exclusive with suspend/hibernation.
> >>>
> >>
> >> Oh, no no.. We shouldn't be doing that even though it seems very
> >> innocuous, because of a subtle reason: the memory hotplug code called
> >> in cpu_up() tries to acquire pm_mutex! So we will end up deadlocking
> >> ourselves, due to recursive locking!
> > 
> > So, you're referring to mem_online_node().  Is it actually used
> > from any place other than cpu_up()?  Perhaps we can remove the
> > lock_system_sleep() from lock_memory_hotplug() if store_online()
> > acquires pm_mutex?  And if we can't, then why exactly?
> > 
> 
> I didn't find any other place where mem_online_node() is called. But I'll
> check it more thoroughly to be sure. If cpu_up() is the only place where
> it is called, then I think we can do what you said above. But doesn't it
> seem to be more intrusive than what my patch does? I mean, what if something
> breaks because of that avoidable modification to memory hotplug code?

We'll need to fix it. :-)

The current locking in the memory hotplug code and the way it's used
by the CPU hotplug code seems to be a bit too ad hoc to me, so IMHO
it makes sense to make it more straightforward anyway.

> >> See kernel/cpu.c: cpu_up() calls mem_online_node() [defined in
> >> mm/memory_hotplug.c/mem_online_node() which calls lock_memory_hotplug()
> >> which internally calls lock_system_sleep(), which is where it tries
> >> to get pm_mutex].
> >>
> >> So this patchset implements the mutual exclusion in the cpu_up() function
> >> (i.e., a little bit deeper down the road than store_online() ) and solves
> >> the problem.
> > 
> > Which is not exactly the right place to do that as Peter has already
> > indicated.  What you want is really the CPU hotplug interface to be
> > mutually exclusive with the suspend/hibernation interface, not the
> > CPU hotplug itself to be mutually exclusive with suspend/hibernation.
> > 
> 
> Oh, is that what Peter meant? I didn't derive that meaning from what
> he said...
> Making the interfaces themselves mutually exclusive is also a good idea.
> But please see my concerns above, about its implementation aspect.
> 
> Since my patch solves the problem with less intrusion, and produces
> roughly the same effect as this interface exclusion idea, why not go with
> it?
> 
> Or am I missing something here?

Your approach may seem to be less intrusive, but it adds locking rules
to the CPU hotplug code whose locking is complicated enough already.
IMO it's better to avoid that.

Thanks,
Rafael

Re: [PATCH v2 2/3] Mutually exclude cpu online and suspend/hibernate

[Srivatsa S. Bhat]

On 10/13/2011 03:39 AM, Rafael J. Wysocki wrote:
> On Wednesday, October 12, 2011, Srivatsa S. Bhat wrote:
>> On 10/13/2011 01:01 AM, Rafael J. Wysocki wrote:
>>> On Wednesday, October 12, 2011, Srivatsa S. Bhat wrote:
>>>> On 10/12/2011 03:26 AM, Rafael J. Wysocki wrote:
>>>>> On Tuesday, October 11, 2011, Srivatsa S. Bhat wrote:
>>>>>> On 10/10/2011 08:46 PM, Srivatsa S. Bhat wrote:
>>>>>>> On 10/10/2011 07:56 PM, Peter Zijlstra wrote:
>>>>>>>> On Mon, 2011-10-10 at 18:15 +0530, Srivatsa S. Bhat wrote:
>>>>>>>>>> +     /*
>>>>>>>>>> +      * Prevent cpu online and suspend/hibernate (including freezer)
>>>>>>>>>> +      * operations from running in parallel. Fail cpu online if suspend or
>>>>>>>>>> +      * hibernate has already started.
>>>>>>>>>> +      */
>>>>>>>>>> +     if (!trylock_pm_sleep())
>>>>>>>>>
>>>>>>>>> Would it be better to hook into the suspend/hibernate notifiers and
>>>>>>>>> use them to exclude cpu hotplug from suspend/hibernate, instead of
>>>>>>>>> trying to take pm_mutex lock like this?
>>>>>>>>> Peter, I remember you pointing out in another patch's review
>>>>>>>>> (http://thread.gmane.org/gmane.linux.kernel/1198312/focus=1199087)
>>>>>>>>> that introducing more locks in cpu hotplug would be a bad idea. Does that
>>>>>>>>> comment hold here as well, or is this fine? 
>>>>>>>>
>>>>>>>> Arguably pm_mutex is already involved in the whole hotplug dance due to
>>>>>>>> suspend using it, that said, I'm not at all familiar with the whole
>>>>>>>> suspend/hibernate side of things.
>>>>>>>>
>>>>>>>> I tried having a quick look this morning but failed to find the actual
>>>>>>>> code.
>>>>>>>>
>>>>>>>> I think it would be good to have an overview of the various locks and a
>>>>>>>> small description of how they interact/nest.
>>>>>>>>
>>>>>>>
>>>>>>> Sure. I'll put together whatever I have understood, in the form of a patch
>>>>>>> to Documentation/power directory and post it tomorrow, for the benefit of
>>>>>>> all.
>>>>>>>
>>>>>>
>>>>>> Here it is, just as promised :-)
>>>>>> http://lkml.org/lkml/2011/10/11/393
>>>>>
>>>>> Well, I have an idea.
>>>>>
>>>>> Why don't we make drivers/base/cpu.c:store_online() take pm_mutex
>>>>> in addition to calling cpu_hotplug_driver_lock()?  This at least
>>>>> will make the interface mutually exclusive with suspend/hibernation.
>>>>>
>>>>
>>>> Oh, no no.. We shouldn't be doing that even though it seems very
>>>> innocuous, because of a subtle reason: the memory hotplug code called
>>>> in cpu_up() tries to acquire pm_mutex! So we will end up deadlocking
>>>> ourselves, due to recursive locking!
>>>
>>> So, you're referring to mem_online_node().  Is it actually used
>>> from any place other than cpu_up()?  Perhaps we can remove the
>>> lock_system_sleep() from lock_memory_hotplug() if store_online()
>>> acquires pm_mutex?  And if we can't, then why exactly?
>>>
>>
>> I didn't find any other place where mem_online_node() is called. But I'll
>> check it more thoroughly to be sure. If cpu_up() is the only place where
>> it is called, then I think we can do what you said above. But doesn't it
>> seem to be more intrusive than what my patch does? I mean, what if something
>> breaks because of that avoidable modification to memory hotplug code?
> 
> We'll need to fix it. :-)
> 
> The current locking in the memory hotplug code and the way it's used
> by the CPU hotplug code seems to be a bit too ad hoc to me, so IMHO
> it makes sense to make it more straightforward anyway.
> 
>>>> See kernel/cpu.c: cpu_up() calls mem_online_node() [defined in
>>>> mm/memory_hotplug.c/mem_online_node() which calls lock_memory_hotplug()
>>>> which internally calls lock_system_sleep(), which is where it tries
>>>> to get pm_mutex].
>>>>
>>>> So this patchset implements the mutual exclusion in the cpu_up() function
>>>> (i.e., a little bit deeper down the road than store_online() ) and solves
>>>> the problem.
>>>
>>> Which is not exactly the right place to do that as Peter has already
>>> indicated.  What you want is really the CPU hotplug interface to be
>>> mutually exclusive with the suspend/hibernation interface, not the
>>> CPU hotplug itself to be mutually exclusive with suspend/hibernation.
>>>
>>
>> Oh, is that what Peter meant? I didn't derive that meaning from what
>> he said...
>> Making the interfaces themselves mutually exclusive is also a good idea.
>> But please see my concerns above, about its implementation aspect.
>>
>> Since my patch solves the problem with less intrusion, and produces
>> roughly the same effect as this interface exclusion idea, why not go with
>> it?
>>
>> Or am I missing something here?
> 
> Your approach may seem to be less intrusive, but it adds locking rules
> to the CPU hotplug code whose locking is complicated enough already.
> IMO it's better to avoid that.
> 

Hi,

Warning: What I am going to say next might be confusing, because I am trying
to map different solutions to different problems, because I think its worth
it, considering the current situation!

Given that the microcode update hotplug optimization is going upstream,
(https://lkml.org/lkml/2011/10/13/258), we know that whether we want to call
it a bugfix or optimization, either way it *is* going to fix this bug.
And this current patchset's mutual exclusion approach was also aimed at fixing
the same bug since at the time it was written, discussion was still going on
about which solution would be better.

So considering the current situation, do we really need 2 solutions for the
same problem? I don't think so.
That said, this patchset is not going to go waste. There is another problem
with cpu hotplug path when freezer is involved, which I was trying to solve
in another thread [1],[2]. Based on Peter's suggestion [3] to avoid introducing
yet another new mutex to the cpu hotplug path, this current patchset, with
minor modifications, seems to be a possible solution to that problem.

So, to summarize, since the objective of this current patchset has been
already taken care of by the microcode update optimization, it doesn't make
much sense to push this patchset with the same old motivation.
However, it so happens that this patchset is useful to solve another problem.
So I feel adapting this patchset to that problem would be a good way to go
forward.

Ideas/comments?

[1] http://thread.gmane.org/gmane.linux.kernel/1198312/focus=1198312
[2] http://thread.gmane.org/gmane.linux.kernel/1198312/focus=1200691
[3] http://thread.gmane.org/gmane.linux.kernel/1198312/focus=1201141

-- 
Regards,
Srivatsa S. Bhat  <srivatsa.bhat@linux.vnet.ibm.com>
Linux Technology Center,
IBM India Systems and Technology Lab

Re: [PATCH v2 2/3] Mutually exclude cpu online and suspend/hibernate

[Tejun Heo]

Hello,

On Thu, Oct 13, 2011 at 09:12:16PM +0530, Srivatsa S. Bhat wrote:
> Given that the microcode update hotplug optimization is going upstream,
> (https://lkml.org/lkml/2011/10/13/258), we know that whether we want to call
> it a bugfix or optimization, either way it *is* going to fix this bug.
> And this current patchset's mutual exclusion approach was also aimed at fixing
> the same bug since at the time it was written, discussion was still going on
> about which solution would be better.

I hate to sound like a broken recorder but the above patch isn't
strictly correct for hot-swap cases, right?  Let's please add
revalidation before pushing that upstream.  Rafael, did you already
take that patch?

Thanks.

-- 
tejun

Re: [PATCH v2 2/3] Mutually exclude cpu online and suspend/hibernate

[Borislav Petkov]

On Thu, Oct 13, 2011 at 12:06:02PM -0400, Tejun Heo wrote:
> I hate to sound like a broken recorder but the above patch isn't
> strictly correct for hot-swap cases, right?

hpa is working on a ucode loading solution which will take care of your
hotswap case too.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH v2 2/3] Mutually exclude cpu online and suspend/hibernate

[Srivatsa S. Bhat]

On 10/13/2011 10:31 PM, Borislav Petkov wrote:
> On Thu, Oct 13, 2011 at 12:06:02PM -0400, Tejun Heo wrote:
>> I hate to sound like a broken recorder but the above patch isn't
>> strictly correct for hot-swap cases, right?
> 
> hpa is working on a ucode loading solution which will take care of your
> hotswap case too.
> 

Tejun, I have written a patch below (untested) that does what you said.
So Boris, hpa's work would make such a patch unnecessary is it?

Thanks,
Srivatsa S. Bhat

---

 arch/x86/kernel/microcode_core.c |   33 +++++++++++++++++++++++++++++++++
 1 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
index f924280..849ae2d 100644
--- a/arch/x86/kernel/microcode_core.c
+++ b/arch/x86/kernel/microcode_core.c
@@ -155,6 +155,27 @@ static int collect_cpu_info(int cpu)
 	return ret;
 }
 
+/*
+ * Compare the microcode revision that the kernel has in memory
+ * for this cpu and the microcode revision that we need to apply
+ * on this cpu. If they match, return 0, else return -1.
+ */
+static int compare_cpu_with_microcode(int cpu)
+{
+	struct ucode_cpu_info *uci_mem = ucode_cpu_info + cpu;
+	struct ucode_cpu_info uci_cpu;
+	int ret;
+
+	ret = collect_cpu_info_on_target(cpu, &uci_cpu->cpu_sig);
+	if(!ret) {
+		if (!(uci_mem->cpu_sig->sig == uci_cpu->cpu_sig->sig &&
+		      uci_mem->cpu_sig->pf == uci_cpu->cpu_sig->pf &&
+		      uci_mem->cpu_sig->rev == uci_cpu->cpu_sig->rev))
+			ret = -1;
+	}
+	return ret;
+}
+
 struct apply_microcode_ctx {
 	int err;
 };
@@ -397,6 +418,18 @@ static enum ucode_state microcode_update_cpu(int cpu)
 	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
 	enum ucode_state ustate;
 
+	/*
+	 * If the CPU on which we are going to update the
+	 * microcode and the microcode which we currently
+	 * have in kernel memory are incompatible, then
+	 * invalidate the microcode that we have (and also
+	 * free its memory), so that we can get the required
+	 * microcode afresh.
+	 */
+	if (compare_cpu_with_microcode(cpu)) {
+		microcode_fini_cpu(cpu);
+	}
+
 	if (uci->valid)
 		ustate = microcode_resume_cpu(cpu);
 	else

Re: [PATCH v2 2/3] Mutually exclude cpu online and suspend/hibernate

[Alan Stern]

On Thu, 13 Oct 2011, Borislav Petkov wrote:

> On Thu, Oct 13, 2011 at 12:06:02PM -0400, Tejun Heo wrote:
> > I hate to sound like a broken recorder but the above patch isn't
> > strictly correct for hot-swap cases, right?
> 
> hpa is working on a ucode loading solution which will take care of your
> hotswap case too.

You know, it would be a good idea to take a step back and think about a 
broader picture.  Suspend and hibernate need to be mutually exclusive 
with other things as well.  The two examples which people have been 
talking about are firmware updates (if the computer goes to sleep 
while the firmware is being changed, it ends up as a brick) and battery 
recharging (the CPU needs to respond quickly to temperature and charger 
events).

There ought to be a general-purpose "prevent system sleep" mechanism 
usable both from within the kernel and from userspace.

Alan Stern

Re: [PATCH v2 2/3] Mutually exclude cpu online and suspend/hibernate

[Rafael J. Wysocki]

On Thursday, October 13, 2011, Alan Stern wrote:
> On Thu, 13 Oct 2011, Borislav Petkov wrote:
> 
> > On Thu, Oct 13, 2011 at 12:06:02PM -0400, Tejun Heo wrote:
> > > I hate to sound like a broken recorder but the above patch isn't
> > > strictly correct for hot-swap cases, right?
> > 
> > hpa is working on a ucode loading solution which will take care of your
> > hotswap case too.
> 
> You know, it would be a good idea to take a step back and think about a 
> broader picture.  Suspend and hibernate need to be mutually exclusive 
> with other things as well.  The two examples which people have been 
> talking about are firmware updates (if the computer goes to sleep 
> while the firmware is being changed, it ends up as a brick) and battery 
> recharging (the CPU needs to respond quickly to temperature and charger 
> events).
> 
> There ought to be a general-purpose "prevent system sleep" mechanism 
> usable both from within the kernel and from userspace.

I have a patch for that ready, will post it later today.

Thanks,
Rafael

Re: [PATCH v2 2/3] Mutually exclude cpu online and suspend/hibernate

[Rafael J. Wysocki]

On Thursday, October 13, 2011, Tejun Heo wrote:
> Hello,
> 
> On Thu, Oct 13, 2011 at 09:12:16PM +0530, Srivatsa S. Bhat wrote:
> > Given that the microcode update hotplug optimization is going upstream,
> > (https://lkml.org/lkml/2011/10/13/258), we know that whether we want to call
> > it a bugfix or optimization, either way it *is* going to fix this bug.
> > And this current patchset's mutual exclusion approach was also aimed at fixing
> > the same bug since at the time it was written, discussion was still going on
> > about which solution would be better.
> 
> I hate to sound like a broken recorder but the above patch isn't
> strictly correct for hot-swap cases, right?  Let's please add
> revalidation before pushing that upstream.  Rafael, did you already
> take that patch?

No, I didn't, but I'm not sure about the x86 maintainers.

Thanks,
Rafael

Re: [PATCH v2 2/3] Mutually exclude cpu online and suspend/hibernate

[Srivatsa S. Bhat]

On 10/13/2011 10:59 PM, Srivatsa S. Bhat wrote:
> On 10/13/2011 10:31 PM, Borislav Petkov wrote:
>> On Thu, Oct 13, 2011 at 12:06:02PM -0400, Tejun Heo wrote:
>>> I hate to sound like a broken recorder but the above patch isn't
>>> strictly correct for hot-swap cases, right?
>>
>> hpa is working on a ucode loading solution which will take care of your
>> hotswap case too.
>>
> 
> Tejun, I have written a patch below (untested) that does what you said.
> So Boris, hpa's work would make such a patch unnecessary is it?
> 

I have posted this patch (tested/corrected and with changelog), in case
it is useful/necessary. Link: https://lkml.org/lkml/2011/10/19/295

> ---
> 
>  arch/x86/kernel/microcode_core.c |   33 +++++++++++++++++++++++++++++++++
>  1 files changed, 33 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
> index f924280..849ae2d 100644
> --- a/arch/x86/kernel/microcode_core.c
> +++ b/arch/x86/kernel/microcode_core.c
> @@ -155,6 +155,27 @@ static int collect_cpu_info(int cpu)
>  	return ret;
>  }
>  
> +/*
> + * Compare the microcode revision that the kernel has in memory
> + * for this cpu and the microcode revision that we need to apply
> + * on this cpu. If they match, return 0, else return -1.
> + */
> +static int compare_cpu_with_microcode(int cpu)
> +{
> +	struct ucode_cpu_info *uci_mem = ucode_cpu_info + cpu;
> +	struct ucode_cpu_info uci_cpu;
> +	int ret;
> +
> +	ret = collect_cpu_info_on_target(cpu, &uci_cpu->cpu_sig);
> +	if(!ret) {
> +		if (!(uci_mem->cpu_sig->sig == uci_cpu->cpu_sig->sig &&
> +		      uci_mem->cpu_sig->pf == uci_cpu->cpu_sig->pf &&
> +		      uci_mem->cpu_sig->rev == uci_cpu->cpu_sig->rev))
> +			ret = -1;
> +	}
> +	return ret;
> +}
> +
>  struct apply_microcode_ctx {
>  	int err;
>  };
> @@ -397,6 +418,18 @@ static enum ucode_state microcode_update_cpu(int cpu)
>  	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
>  	enum ucode_state ustate;
>  
> +	/*
> +	 * If the CPU on which we are going to update the
> +	 * microcode and the microcode which we currently
> +	 * have in kernel memory are incompatible, then
> +	 * invalidate the microcode that we have (and also
> +	 * free its memory), so that we can get the required
> +	 * microcode afresh.
> +	 */
> +	if (compare_cpu_with_microcode(cpu)) {
> +		microcode_fini_cpu(cpu);
> +	}
> +
>  	if (uci->valid)
>  		ustate = microcode_resume_cpu(cpu);
>  	else
>