[BUGFIX][PATCH] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[BUGFIX][PATCH] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Srivatsa S. Bhat]

This patch addresses the warnings found in the logs in the
task freezing failure bug reported in https://lkml.org/lkml/2011/9/5/28

The warnings appear because of the reason explained below:

There are microcode callbacks registered for CPU hotplug events such
as a CPU getting offlined or onlined. When a CPU is offlined
with tasks being frozen (as in the case of disabling the non-boot CPUs
while preparing for a system suspend operation), the CPU_DEAD_FROZEN
notification is sent, for which the microcode callback does not
do anything. In particular, it does not free or invalidate the CPU
microcode which it had got from userspace earlier. Hence when that CPU
comes back online with tasks being frozen (as in the case of re-enabling
the non-boot CPUs during a resume operation after suspend), the microcode
callback applies the microcode (which it already possesses) to that CPU.

However, during a pure CPU hotplug operation, tasks are not frozen and
hence the CPU_DEAD notification is sent. Upon this event notification,
the microcode callback frees the copy of microcode it has and
invalidates it. And during a CPU online, it tries to apply the microcode
to the CPU, but since it doesn't have the copy of the microcode, it depends
on a userspace utility to get the microcode. This is perfectly fine when
doing plain CPU hotplug operations alone.

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 addresses this issue by ensuring that microcode is not freed
from kernel memory, nor invalidated when a CPU goes offline. Thus once the
kernel gets the microcode during boot-up, it will never have to depend on
userspace ever again to get microcode, since it never releases the copy it
already has. So every run of the microcode callback for CPU online event will
now succeed irrespective of whether userspace is frozen or not. As a result,
this fixes the task freezing failure encountered while running CPU hotplug
stress test along with suspend/resume operations simultaneously.

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

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

diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
index f924280..cd7ef2f 100644
--- a/arch/x86/kernel/microcode_core.c
+++ b/arch/x86/kernel/microcode_core.c
@@ -483,7 +483,15 @@ mc_cpu_callback(struct notifier_block *nb, unsigned long action, void *hcpu)
 		sysfs_remove_group(&sys_dev->kobj, &mc_attr_group);
 		pr_debug("CPU%d removed\n", cpu);
 		break;
-	case CPU_DEAD:
+
+	/*
+	 * Do not invalidate the microcode if a CPU goes offline,
+	 * because it would be impossible to get the microcode again
+	 * from userspace when the CPU comes back up, if the userspace
+	 * happens to be frozen at that moment by the freezer subsystem,
+	 * for example, due to a suspend operation in progress.
+	 */
+
 	case CPU_UP_CANCELED_FROZEN:
 		/* The CPU refused to come up during a system resume */
 		microcode_fini_cpu(cpu);

Re: [BUGFIX][PATCH] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Rafael J. Wysocki]

Hi,

Thanks for the fix.

On Sunday, October 02, 2011, Srivatsa S. Bhat wrote:
> This patch addresses the warnings found in the logs in the
> task freezing failure bug reported in https://lkml.org/lkml/2011/9/5/28
> 
> The warnings appear because of the reason explained below:
> 
> There are microcode callbacks registered for CPU hotplug events such
> as a CPU getting offlined or onlined. When a CPU is offlined
> with tasks being frozen (as in the case of disabling the non-boot CPUs
> while preparing for a system suspend operation), the CPU_DEAD_FROZEN
> notification is sent, for which the microcode callback does not
> do anything. In particular, it does not free or invalidate the CPU
> microcode which it had got from userspace earlier. Hence when that CPU
> comes back online with tasks being frozen (as in the case of re-enabling
> the non-boot CPUs during a resume operation after suspend), the microcode
> callback applies the microcode (which it already possesses) to that CPU.
> 
> However, during a pure CPU hotplug operation, tasks are not frozen and
> hence the CPU_DEAD notification is sent. Upon this event notification,
> the microcode callback frees the copy of microcode it has and
> invalidates it. And during a CPU online, it tries to apply the microcode
> to the CPU, but since it doesn't have the copy of the microcode, it depends
> on a userspace utility to get the microcode. This is perfectly fine when
> doing plain CPU hotplug operations alone.
> 
> 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 addresses this issue by ensuring that microcode is not freed
> from kernel memory, nor invalidated when a CPU goes offline. Thus once the
> kernel gets the microcode during boot-up, it will never have to depend on
> userspace ever again to get microcode, since it never releases the copy it
> already has. So every run of the microcode callback for CPU online event will
> now succeed irrespective of whether userspace is frozen or not. As a result,
> this fixes the task freezing failure encountered while running CPU hotplug
> stress test along with suspend/resume operations simultaneously.
> 
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> ---

Thanks for the fix.  I'd like to push it for 3.2 and possibly -stable.

Does anyone have any objections?

Rafael


>  arch/x86/kernel/microcode_core.c |   10 +++++++++-
>  1 files changed, 9 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
> index f924280..cd7ef2f 100644
> --- a/arch/x86/kernel/microcode_core.c
> +++ b/arch/x86/kernel/microcode_core.c
> @@ -483,7 +483,15 @@ mc_cpu_callback(struct notifier_block *nb, unsigned long action, void *hcpu)
>  		sysfs_remove_group(&sys_dev->kobj, &mc_attr_group);
>  		pr_debug("CPU%d removed\n", cpu);
>  		break;
> -	case CPU_DEAD:
> +
> +	/*
> +	 * Do not invalidate the microcode if a CPU goes offline,
> +	 * because it would be impossible to get the microcode again
> +	 * from userspace when the CPU comes back up, if the userspace
> +	 * happens to be frozen at that moment by the freezer subsystem,
> +	 * for example, due to a suspend operation in progress.
> +	 */
> +
>  	case CPU_UP_CANCELED_FROZEN:
>  		/* The CPU refused to come up during a system resume */
>  		microcode_fini_cpu(cpu);
> 
> 
> 
> 

Re: [BUGFIX][PATCH] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Tejun Heo]

Hello,

On Mon, Oct 03, 2011 at 12:35:04AM +0530, Srivatsa S. Bhat wrote:
> So, this patch addresses this issue by ensuring that microcode is not freed
> from kernel memory, nor invalidated when a CPU goes offline. Thus once the
> kernel gets the microcode during boot-up, it will never have to depend on
> userspace ever again to get microcode, since it never releases the copy it
> already has. So every run of the microcode callback for CPU online event will
> now succeed irrespective of whether userspace is frozen or not. As a result,
> this fixes the task freezing failure encountered while running CPU hotplug
> stress test along with suspend/resume operations simultaneously.

I'm not familiar with how microcode is supposed to be managed but is
it impossible for the newly hotplugged CPU (an actual hot unplug /
plug) may not like the microcode loaded for the previous CPU?  Isn't
that why CPU_DEAD was invalidating the microcode?

Thanks.

-- 
tejun

Re: [BUGFIX][PATCH] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Srivatsa S. Bhat]

Hi,

On 10/03/2011 01:20 AM, Tejun Heo wrote:
> Hello,
> 
> On Mon, Oct 03, 2011 at 12:35:04AM +0530, Srivatsa S. Bhat wrote:
>> So, this patch addresses this issue by ensuring that microcode is not freed
>> from kernel memory, nor invalidated when a CPU goes offline. Thus once the
>> kernel gets the microcode during boot-up, it will never have to depend on
>> userspace ever again to get microcode, since it never releases the copy it
>> already has. So every run of the microcode callback for CPU online event will
>> now succeed irrespective of whether userspace is frozen or not. As a result,
>> this fixes the task freezing failure encountered while running CPU hotplug
>> stress test along with suspend/resume operations simultaneously.
> 
> I'm not familiar with how microcode is supposed to be managed but is
> it impossible for the newly hotplugged CPU (an actual hot unplug /
> plug) may not like the microcode loaded for the previous CPU?  Isn't
> that why CPU_DEAD was invalidating the microcode?
> 

Actually, looking at the code, I found that a copy of the microcode
is maintained by the kernel for every CPU. 
The relevant lines are:

arch/x86/include/asm/microcode.h:

struct ucode_cpu_info {
        struct cpu_signature    cpu_sig;
        int                     valid;
        void                    *mc;
};
extern struct ucode_cpu_info ucode_cpu_info[];


arch/x86/kernel/microcode_core.h:

struct ucode_cpu_info           ucode_cpu_info[NR_CPUS];


So when a CPU goes offline and comes back online, I don't see why the kernel
should not reuse the microcode that it already has. Anyhow the microcode will
not change. The same microcode would be requested from userspace again if the
kernel has freed its copy.

So what I feel is, earlier, the kernel used to invalidate the microcode for the CPU_DEAD
notification may be just to free the kernel's copy of the microcode as a memory
optimization (thinking that the microcode is not needed any more in kernel memory,
atleast for now).

This is my understanding. Please enlighten me if I am wrong.

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

Re: [BUGFIX][PATCH] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Tejun Heo]

Hello,

On Mon, Oct 03, 2011 at 01:34:36AM +0530, Srivatsa S. Bhat wrote:
> So when a CPU goes offline and comes back online, I don't see why the kernel
> should not reuse the microcode that it already has. Anyhow the microcode will
> not change. The same microcode would be requested from userspace again if the
> kernel has freed its copy.

Can't one take a cpu offline, hot unplug it from the board and put in
a new one and then bring it online?  That's usually what "hot
[un]plug" stands for.  I don't think one would be able to put in a
very different processor but maybe, say, revision difference is
allowed and microcode should be looked up again?  Assuming that the
same microcode can always be applied after the actual CPU is swapped
could be dangerous.  Again, I'm not sure about how this is supposed to
be managed so I could be wrong.

If I'm not wrong, can't we add synchronization between cpu hotplug and
freezer so that they don't happen in parallel?

Thanks.

-- 
tejun

Re: [BUGFIX][PATCH] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Srivatsa S. Bhat]

Hi,

First of all, thank you for your invaluable inputs.

On 10/03/2011 06:10 AM, Tejun Heo wrote:
> Hello,
> 
> On Mon, Oct 03, 2011 at 01:34:36AM +0530, Srivatsa S. Bhat wrote:
>> So when a CPU goes offline and comes back online, I don't see why the kernel
>> should not reuse the microcode that it already has. Anyhow the microcode will
>> not change. The same microcode would be requested from userspace again if the
>> kernel has freed its copy.
> 
> Can't one take a cpu offline, hot unplug it from the board and put in
> a new one and then bring it online?  That's usually what "hot
> [un]plug" stands for.  I don't think one would be able to put in a
> very different processor but maybe, say, revision difference is
> allowed and microcode should be looked up again?  Assuming that the
> same microcode can always be applied after the actual CPU is swapped
> could be dangerous.  Again, I'm not sure about how this is supposed to
> be managed so I could be wrong.
>

I see your point here. However it is also true that quite a lot of CPU hotplug
operations are done with use-cases other than physically plugging out and plugging in
CPUs. For example, the disabling and enabling of CPUs during suspend and resume,
the offlining of CPUs when the system is idle (in some architectures) to reduce
idle power consumption etc. Moreover physically hotplugging CPUs also demands that
the electrical wiring to the CPUs are specially designed in such a way as to support
physical CPU hotplugging - a feature which is supported by only very few hardware
manufacturers, if what I have heard is right.

Another related fact to consider would be how the disabling and enabling of non-boot
CPUs are done during system suspend and resume. Since the tasks are frozen at those
points, a different notification is sent i.e.,CPU_[ONLINE|DEAD]_FROZEN instead of the
usual CPU_[ONLINE|DEAD] notification. 
And upon the CPU_DEAD_FROZEN notification, the microcode core doesn't invalidate the
microcode! Hence, it won't have trouble applying the microcode to the CPU during the
phase of enabling the non-boot CPUs. But a valid point here would be that during suspend
and resume, nobody expects to plug out and plug in a new CPU!

But in any case, suppose we need a revised microcode (due to the usecase you have
described, for example), then I agree that it would definitely be not good to forcefully
apply the same old microcode that the kernel has.

> If I'm not wrong, can't we add synchronization between cpu hotplug and
> freezer so that they don't happen in parallel?
> 

I have posted another patch related to CPU hotplug and freezer here:
https://lkml.org/lkml/2011/10/2/163
That patch aims to sync up the CPU hotplug infrastructure with the activity of the 
freezer and hence ensure that the right notifications are sent. Maybe I could easily
modify that patch to disable CPU hotplugging when the freezer is active.

But still, that would not solve our problem due to the following possible scenario:

[Let us assume that we go ahead and invalidate the microcode upon a CPU_DEAD notification,
like you suggested.] Then, consider this scenario:

* Take a CPU offline (a pure CPU hotplug operation, not related to suspend).
  Let us call this CPU X.
  - This would involve a CPU_DEAD notification, upon which the microcode is invalidated.

* Start freezing tasks (due to a suspend operation in progress). Now we disable
  CPU hotplugging.
 
* Disable (offline) the non-boot CPUs as part of suspend operation. This sends the
CPU_DEAD_FROZEN notification, upon which the microcode is NOT invalidated for the other CPUs.

* Enable (online) the non-boot CPUs as part of resume operation. Please note that the tasks
are still frozen. This is where we hit the same issue again! When trying to bring that CPU X
back online, it finds that the microcode has been invalidated and hence tries to request the
userspace for the microcode, but alas, the userspace is still frozen at that moment.
This is the exact problem we were trying to solve earlier, which remains unsolved by disabling
CPU hotplug during freezer operations!


So my one-liner patch here tries to solve this problem by not invalidating the microcode at all,
to handle the corner case of a CPU hotplug going on in parallel with freezer operations.
But it creates issues with the usecase you described.

At the moment I can't think of any clean solution that solves both these problems - 
  a. The problem of requesting microcode from userspace when it is frozen (which can be individually
     solved by my patch).
  b. The possible need for a revised microcode during CPU online (due to the remote usecase of 
     physically plugging-out and plugging-in a new CPU), when the tasks are frozen.

To address both problems 'a' and 'b' in one shot, I can think of a rather ugly workaround such as:
* Go ahead and invalidate the microcode upon a CPU_DEAD notification.
* During CPU onlining, if we find that the userspace is frozen, defer applying microcode for now
  and register a callback function to be executed immediately when the userspace gets thawed.
  Also, prevent any task from executing on this CPU until the proper microcode is applied to it.
* When the userspace finally gets thawed, run the registered callback function, get the revised
  microcode from userspace, apply it to the CPU and then go ahead and run tasks on that CPU.

Any ideas?

Summary:
My patch solves the problem that is most likely hit, while there are some corner cases
which it doesn't handle, such as physical CPU hotplugging which might require revised microcode to be
loaded. The idea proposed above could solve both issues, but it doesn't seem very elegant, or does it?

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

Re: [BUGFIX][PATCH] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Borislav Petkov]

Hi Srivatsa,

On Mon, Oct 03, 2011 at 11:21:49AM +0530, Srivatsa S. Bhat wrote:
> So my one-liner patch here tries to solve this problem by not
> invalidating the microcode at all, to handle the corner case of a CPU
> hotplug going on in parallel with freezer operations. But it creates
> issues with the usecase you described.

I think your patch makes sense because re-loading the ucode during
a suspend/resume cycle is unnecessary. If one wants to update the
microcode, it should happen later when the box is resumed again: you
simply put the new microcode image in /lib/firmware/... and on AMD
unload/reload the microcode module and on Intel you do either that or
use the deprecated microcode_ctl.

However, let me test it on a couple of AMD boxes tomorrow to verify.

> At the moment I can't think of any clean solution that solves both these problems - 
>   a. The problem of requesting microcode from userspace when it is frozen (which can be individually
>      solved by my patch).
>   b. The possible need for a revised microcode during CPU online (due to the remote usecase of 
>      physically plugging-out and plugging-in a new CPU), when the tasks are frozen.

Yeah, let me remind you guys, we're talking about x86 CPUs here. AFAICT,
I don't think that physical unplug is supported by any vendor yet but
I'd welcome surprises :-).

Thanks.

-- 
Regards/Gruss,
    Boris.

Re: [BUGFIX][PATCH] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Tejun Heo]

Hello,

On Mon, Oct 03, 2011 at 10:47:54AM +0200, Borislav Petkov wrote:
> I think your patch makes sense because re-loading the ucode during
> a suspend/resume cycle is unnecessary. If one wants to update the
> microcode, it should happen later when the box is resumed again: you
> simply put the new microcode image in /lib/firmware/... and on AMD
> unload/reload the microcode module and on Intel you do either that or
> use the deprecated microcode_ctl.

I don't think it changes anything for suspend/resume cycles.  They're
different hooks.  The proposed patch changes actual cpu hotplug paths.

> However, let me test it on a couple of AMD boxes tomorrow to verify.
> 
> > At the moment I can't think of any clean solution that solves both these problems - 
> >   a. The problem of requesting microcode from userspace when it is frozen (which can be individually
> >      solved by my patch).
> >   b. The possible need for a revised microcode during CPU online (due to the remote usecase of 
> >      physically plugging-out and plugging-in a new CPU), when the tasks are frozen.
> 
> Yeah, let me remind you guys, we're talking about x86 CPUs here. AFAICT,
> I don't think that physical unplug is supported by any vendor yet but
> I'd welcome surprises :-).

I recall hearing about someone experimenting with actual hotplug a
while ago.  Dunno whether that was production or not tho.  Even if
not, given the continuing penetration of x86 into highend, I don't
think it would take too long to see physical CPU hotplug in
production.  Also, it just is a poor engineering.

Thanks.

--
tejun

Re: [BUGFIX][PATCH] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Srivatsa S. Bhat]

On 10/04/2011 12:45 PM, Tejun Heo wrote:
> Hello,
> 
> On Mon, Oct 03, 2011 at 10:47:54AM +0200, Borislav Petkov wrote:
>> I think your patch makes sense because re-loading the ucode during
>> a suspend/resume cycle is unnecessary. If one wants to update the
>> microcode, it should happen later when the box is resumed again: you
>> simply put the new microcode image in /lib/firmware/... and on AMD
>> unload/reload the microcode module and on Intel you do either that or
>> use the deprecated microcode_ctl.
> 
> I don't think it changes anything for suspend/resume cycles.  They're
> different hooks.  The proposed patch changes actual cpu hotplug paths.
> 

Hi,
I agree with you that my patch modifies the actual cpu hotplug path, which is
not desirable if we are going to do physical cpu hotplug because, even when the
freezer is not active, my patch would prevent us from revising the microcode
even during a pure cpu hotplug operation.

I would like to propose a modified solution to the problem:

Taking a CPU offline:
* Upon a CPU_DEAD notification, just like the code originally did, we free
  the kernel's copy of the microcode and invalidate it. So no changes here.
 
Bringing a CPU online:
* When a CPU_ONLINE or CPU_ONLINE_FROZEN notification is received, 
  a. If the userspace is not frozen, we request microcode from userspace and
     apply it to the cpu.

  b. However if we find that the userspace is frozen at that moment, we defer
     applying microcode now and register a callback function to be executed
     immediately when the userspace gets thawed. This callback function would
     request microcode from userspace and apply it to the cpu.

The advantage of this approach over the previous idea I proposed is that we don't
prevent the kernel from invalidating the microcode, thereby ensuring that we
don't break any possible assumptions about microcode.
So, every cpu will get a fresh copy of microcode from userspace, either immediately
or after a while, depending on whether the userspace is frozen or not at that instant.
As a consequence, physical cpu hotplug would also work just fine with this approach.

How does this sound? I'll write up a patch for this and post it for review soon.

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

Re: [BUGFIX][PATCH] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Borislav Petkov]

On Tue, Oct 04, 2011 at 06:45:12PM +0530, Srivatsa S. Bhat wrote:
> I would like to propose a modified solution to the problem:
> 
> Taking a CPU offline:
> * Upon a CPU_DEAD notification, just like the code originally did, we free
>   the kernel's copy of the microcode and invalidate it. So no changes here.
>  
> Bringing a CPU online:
> * When a CPU_ONLINE or CPU_ONLINE_FROZEN notification is received, 
>   a. If the userspace is not frozen, we request microcode from userspace and
>      apply it to the cpu.
> 
>   b. However if we find that the userspace is frozen at that moment, we defer
>      applying microcode now and register a callback function to be executed
>      immediately when the userspace gets thawed. This callback function would
>      request microcode from userspace and apply it to the cpu.

No need for that if we can drop the whole re-requesting of ucode on
CPU_ONLINE* (see my other mail). Let me run some tests before though.

Thanks.

-- 
Regards/Gruss,
Boris.

Re: [BUGFIX][PATCH] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Borislav Petkov]

On Tue, Oct 04, 2011 at 03:46:53PM +0200, Borislav Petkov wrote:
> On Tue, Oct 04, 2011 at 06:45:12PM +0530, Srivatsa S. Bhat wrote:
> > I would like to propose a modified solution to the problem:
> > 
> > Taking a CPU offline:
> > * Upon a CPU_DEAD notification, just like the code originally did, we free
> >   the kernel's copy of the microcode and invalidate it. So no changes here.
> >  
> > Bringing a CPU online:
> > * When a CPU_ONLINE or CPU_ONLINE_FROZEN notification is received, 
> >   a. If the userspace is not frozen, we request microcode from userspace and
> >      apply it to the cpu.
> > 
> >   b. However if we find that the userspace is frozen at that moment, we defer
> >      applying microcode now and register a callback function to be executed
> >      immediately when the userspace gets thawed. This callback function would
> >      request microcode from userspace and apply it to the cpu.
> 
> No need for that if we can drop the whole re-requesting of ucode on
> CPU_ONLINE* (see my other mail). Let me run some tests before though.

Ok, it looks good. I had one issue with what happens when there's no
ucode image but the ucode driver is a bit-hmm... and that case actually
magically works.

So you can have my Acked- and Tested-by:'s for the AMD side - you still
need to test it on Intel with both microcode_ctl and the module un- and
loading so that you make sure you're not introducing regressions, if you
haven't done so yet, of course.

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: [BUGFIX][PATCH] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Rafael J. Wysocki]

On Tuesday, October 04, 2011, Borislav Petkov wrote:
> On Tue, Oct 04, 2011 at 03:46:53PM +0200, Borislav Petkov wrote:
> > On Tue, Oct 04, 2011 at 06:45:12PM +0530, Srivatsa S. Bhat wrote:
> > > I would like to propose a modified solution to the problem:
> > > 
> > > Taking a CPU offline:
> > > * Upon a CPU_DEAD notification, just like the code originally did, we free
> > >   the kernel's copy of the microcode and invalidate it. So no changes here.
> > >  
> > > Bringing a CPU online:
> > > * When a CPU_ONLINE or CPU_ONLINE_FROZEN notification is received, 
> > >   a. If the userspace is not frozen, we request microcode from userspace and
> > >      apply it to the cpu.
> > > 
> > >   b. However if we find that the userspace is frozen at that moment, we defer
> > >      applying microcode now and register a callback function to be executed
> > >      immediately when the userspace gets thawed. This callback function would
> > >      request microcode from userspace and apply it to the cpu.
> > 
> > No need for that if we can drop the whole re-requesting of ucode on
> > CPU_ONLINE* (see my other mail). Let me run some tests before though.
> 
> Ok, it looks good. I had one issue with what happens when there's no
> ucode image but the ucode driver is a bit-hmm... and that case actually
> magically works.
> 
> So you can have my Acked- and Tested-by:'s for the AMD side - you still
> need to test it on Intel with both microcode_ctl and the module un- and
> loading so that you make sure you're not introducing regressions, if you
> haven't done so yet, of course.

Cool, thanks.

I'd like to hear a voice from the Intel side too.

Thanks,
Rafael

Re: [BUGFIX][PATCH] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Srivatsa S. Bhat]

On 10/04/2011 10:44 PM, Borislav Petkov wrote:
> On Tue, Oct 04, 2011 at 03:46:53PM +0200, Borislav Petkov wrote:
>> On Tue, Oct 04, 2011 at 06:45:12PM +0530, Srivatsa S. Bhat wrote:
>>> I would like to propose a modified solution to the problem:
>>>
>>> Taking a CPU offline:
>>> * Upon a CPU_DEAD notification, just like the code originally did, we free
>>>   the kernel's copy of the microcode and invalidate it. So no changes here.
>>>  
>>> Bringing a CPU online:
>>> * When a CPU_ONLINE or CPU_ONLINE_FROZEN notification is received, 
>>>   a. If the userspace is not frozen, we request microcode from userspace and
>>>      apply it to the cpu.
>>>
>>>   b. However if we find that the userspace is frozen at that moment, we defer
>>>      applying microcode now and register a callback function to be executed
>>>      immediately when the userspace gets thawed. This callback function would
>>>      request microcode from userspace and apply it to the cpu.
>>
>> No need for that if we can drop the whole re-requesting of ucode on
>> CPU_ONLINE* (see my other mail). Let me run some tests before though.
> 
> Ok, it looks good. I had one issue with what happens when there's no
> ucode image but the ucode driver is a bit-hmm... and that case actually
> magically works.
> 
> So you can have my Acked- and Tested-by:'s for the AMD side - you still
> need to test it on Intel with both microcode_ctl and the module un- and
> loading so that you make sure you're not introducing regressions, if you
> haven't done so yet, of course.

Hi Borislav,
I went through all your mails. Thank you very much for your reviews, Ack
and for testing my patch.

But I am still a bit concerned about the following issues with my patch:

1. Since we never invalidate the microcode once we get it from userspace, it
   also means that we will never be able to update the microcode for that cpu
   ever again! (since we will continue to reuse the same old microcode over and
   over again on every cpu online operation for that cpu).
   This restriction introduced by my patch seems bad, isn't it?

2. Suppose we have a 16 cpu machine and we boot it with only 8 cpus (ie., we online
   only 8 of the 16 cpus while booting). So it means that the kernel gets a copy
   of the microcode for each of these 8 cpus, but not for the ones that were not
   onlined while booting.
   [Let us assume that cpu number 10 was one among the 8 cpus that were not onlined
    while booting].

   Later on, let's say we start our cpu hotplug + suspend/resume tests simultaneously.
   Now consider this possible scenario:
   
   * Userspace is not frozen
   * We initiate a cpu online operation on cpu 10. At the same time, since suspend
     is in progress, lets say the freezing begins.
   * Just before cpu 10 could be brought up online, userspace gets frozen.
   * Now while bringing up cpu 10, due to the CPU_ONLINE_FROZEN notification, the
     microcode core tries to apply the microcode to the cpu. But unfortunately, it
     doesn't have the microcode! (because this cpu is coming up for the first time
     and hence we never got its microcode from userspace...)

     Now, again the same problem ensues: microcode core calls request_firmware and
     depends on the (frozen) userspace to get the microcode.



Let us now consider other possible solutions:
To implement our intent of not depending on userspace when it is frozen, another
approach would be to modify the code that is run in that scenario : namely CPU_ONLINE_FROZEN.
Do nothing upon a CPU_ONLINE_FROZEN notification (while still invalidating the
microcode upon CPU_DEAD just as in the original code):

--- microcode_core.c	2011-08-23 18:52:07.062729098 +0530
+++ microcode_core.c	2011-10-05 01:41:59.024888447 +0530
@@ -469,7 +469,6 @@ mc_cpu_callback(struct notifier_block *n
 	sys_dev = get_cpu_sysdev(cpu);
 	switch (action) {
 	case CPU_ONLINE:
-	case CPU_ONLINE_FROZEN:
 		microcode_update_cpu(cpu);
 	case CPU_DOWN_FAILED:
 	case CPU_DOWN_FAILED_FROZEN:

However, this also has problems:
1. Consider scenario 2 mentioned above.
   In that scenario, while bringing up cpu 10, since userspace is frozen, we get
   a CPU_ONLINE_FROZEN notification and hence we do nothing: in particular, we don't even
   try to get microcode (please note that we have never got microcode for this cpu from
   userspace at all, till now). What is worse, suppose we stop cpu hotplugging after that,
   the kernel would never ever get or apply microcode on that cpu and still continue to run
   tasks on that cpu!

[In the usual cases, doing nothing upon CPU_ONLINE_FROZEN (ie., not applying the microcode
when the cpu comes online) won't do harm since a cpu hotplug operation doesn't actually
remove the microcode from the cpu since the cpu is not electrically powered off during
cpu hotplug, unless we do physical cpu hotplug.]

2. Suppose upon a CPU_ONLINE notification, we try to get microcode from userspace. Now,
   just before we call request_firmware, lets say the userspace got frozen due to suspend
   operation in progress. Now again, we hit the same problem: request_firmware fails!

[By the way, as Tejun suggested in one of the previous mails, if we add synchronization
 between cpu hotplug and freezing so that they don't happen in parallel, then this problem
 will not arise because while cpu onlining is going on, freezing will never occur.]

So this approach of removing the 'case CPU_ONLINE_FROZEN' statement doesn't seem right as well.

I am still wondering if the approach I proposed earlier (the one in which we defer applying
microcode and queue up a callback function etc) could solve all these issues. I am also playing
around with the idea of coupling that with mutual exclusion between cpu hotplug and freezer to
handle any problematic scenarios.

On a slightly different note, I am also working on fixing the CPU hotplug notifications
here: http://comments.gmane.org/gmane.linux.kernel/1198312
Ultimately that fix might also become necessary to make this whole thing work reliably.

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

Re: [BUGFIX][PATCH] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Borislav Petkov]

On Tue, Oct 04, 2011 at 04:57:10PM -0400, Srivatsa S. Bhat wrote:
> 1. Since we never invalidate the microcode once we get it from userspace, it
>    also means that we will never be able to update the microcode for that cpu
>    ever again! (since we will continue to reuse the same old microcode over and
>    over again on every cpu online operation for that cpu).
>    This restriction introduced by my patch seems bad, isn't it?

Well, if you have a new microcode image, you are supposed to place it
under /lib/firmware/.. or where the kernel has been configured to find
it and then reload the microcode module.

> 2. Suppose we have a 16 cpu machine and we boot it with only 8 cpus (ie., we online
>    only 8 of the 16 cpus while booting). So it means that the kernel gets a copy
>    of the microcode for each of these 8 cpus, but not for the ones that were not
>    onlined while booting.
>    [Let us assume that cpu number 10 was one among the 8 cpus that were not onlined
>     while booting].
> 
>    Later on, let's say we start our cpu hotplug + suspend/resume tests simultaneously.
>    Now consider this possible scenario:
>    
>    * Userspace is not frozen
>    * We initiate a cpu online operation on cpu 10. At the same time, since suspend
>      is in progress, lets say the freezing begins.
>    * Just before cpu 10 could be brought up online, userspace gets frozen.
>    * Now while bringing up cpu 10, due to the CPU_ONLINE_FROZEN notification, the
>      microcode core tries to apply the microcode to the cpu. But unfortunately, it
>      doesn't have the microcode! (because this cpu is coming up for the first time
>      and hence we never got its microcode from userspace...)
> 
>      Now, again the same problem ensues: microcode core calls request_firmware and
>      depends on the (frozen) userspace to get the microcode.

Ok, but is this a real-life scenario you expect to happen somewhere or
is it something that happens only during test? IOW, if you have root
there are many ways to shoot yourself in the foot, right?

[..]

> I am still wondering if the approach I proposed earlier (the one in
> which we defer applying microcode and queue up a callback function
> etc) could solve all these issues. I am also playing around with the
> idea of coupling that with mutual exclusion between cpu hotplug and
> freezer to handle any problematic scenarios.

Well, all those solutions seem like they're not worth the trouble and
complexity if those cases are only conjecture - if you still trigger
them during your testing then probably mutually excluding freezer and
CPU hotplug is something I would lean towards but I could be wrong.

There's of course a much better fix which has been on the table for a
while now involving loading the ucode from the bootloader and applying
it much earlier than what we have now and keeping the ucode image in
memory. This would solve the CPU hotplug deal completely. Maybe it's
time I looked into it :-).

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: [BUGFIX][PATCH] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Srivatsa S. Bhat]

On 10/05/2011 12:51 PM, Borislav Petkov wrote:
> On Tue, Oct 04, 2011 at 04:57:10PM -0400, Srivatsa S. Bhat wrote:
>> 1. Since we never invalidate the microcode once we get it from userspace, it
>>    also means that we will never be able to update the microcode for that cpu
>>    ever again! (since we will continue to reuse the same old microcode over and
>>    over again on every cpu online operation for that cpu).
>>    This restriction introduced by my patch seems bad, isn't it?
> 
> Well, if you have a new microcode image, you are supposed to place it
> under /lib/firmware/.. or where the kernel has been configured to find
> it and then reload the microcode module.
>
Oh well, then we can update the microcode after all...
 
>> 2. Suppose we have a 16 cpu machine and we boot it with only 8 cpus (ie., we online
>>    only 8 of the 16 cpus while booting). So it means that the kernel gets a copy
>>    of the microcode for each of these 8 cpus, but not for the ones that were not
>>    onlined while booting.
>>    [Let us assume that cpu number 10 was one among the 8 cpus that were not onlined
>>     while booting].
>>
>>    Later on, let's say we start our cpu hotplug + suspend/resume tests simultaneously.
>>    Now consider this possible scenario:
>>    
>>    * Userspace is not frozen
>>    * We initiate a cpu online operation on cpu 10. At the same time, since suspend
>>      is in progress, lets say the freezing begins.
>>    * Just before cpu 10 could be brought up online, userspace gets frozen.
>>    * Now while bringing up cpu 10, due to the CPU_ONLINE_FROZEN notification, the
>>      microcode core tries to apply the microcode to the cpu. But unfortunately, it
>>      doesn't have the microcode! (because this cpu is coming up for the first time
>>      and hence we never got its microcode from userspace...)
>>
>>      Now, again the same problem ensues: microcode core calls request_firmware and
>>      depends on the (frozen) userspace to get the microcode.
> 
> Ok, but is this a real-life scenario you expect to happen somewhere or
> is it something that happens only during test? IOW, if you have root
> there are many ways to shoot yourself in the foot, right?
> 

Well, honestly I was just trying to see in which all scenarios the patch
would probably not work well... In real-life I don't expect to hit such
a corner case!

> [..]
> 
>> I am still wondering if the approach I proposed earlier (the one in
>> which we defer applying microcode and queue up a callback function
>> etc) could solve all these issues. I am also playing around with the
>> idea of coupling that with mutual exclusion between cpu hotplug and
>> freezer to handle any problematic scenarios.
> 
> Well, all those solutions seem like they're not worth the trouble and
> complexity if those cases are only conjecture - if you still trigger
> them during your testing then probably mutually excluding freezer and
> CPU hotplug is something I would lean towards but I could be wrong.
>

Even I felt the same (moreover, that complex solution was not foolproof
either!). Please see my other mail which talks about how just mutually
excluding freezer and cpu hotplugging would solve everything.
 
> There's of course a much better fix which has been on the table for a
> while now involving loading the ucode from the bootloader and applying
> it much earlier than what we have now and keeping the ucode image in
> memory. This would solve the CPU hotplug deal completely. Maybe it's
> time I looked into it :-).
> 

Assuming I understood this correctly, I can see some issues in this
approach as well (since it is quite similar to the approach used in my
one-line patch), but yeah, definitely they are all very much corner
cases...

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

Re: [BUGFIX][PATCH] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Rafael J. Wysocki]

On Wednesday, October 05, 2011, Srivatsa S. Bhat wrote:
> On 10/05/2011 12:51 PM, Borislav Petkov wrote:
> > On Tue, Oct 04, 2011 at 04:57:10PM -0400, Srivatsa S. Bhat wrote:
> >> 1. Since we never invalidate the microcode once we get it from userspace, it
> >>    also means that we will never be able to update the microcode for that cpu
> >>    ever again! (since we will continue to reuse the same old microcode over and
> >>    over again on every cpu online operation for that cpu).
> >>    This restriction introduced by my patch seems bad, isn't it?
> > 
> > Well, if you have a new microcode image, you are supposed to place it
> > under /lib/firmware/.. or where the kernel has been configured to find
> > it and then reload the microcode module.
> >
> Oh well, then we can update the microcode after all...
>  
> >> 2. Suppose we have a 16 cpu machine and we boot it with only 8 cpus (ie., we online
> >>    only 8 of the 16 cpus while booting). So it means that the kernel gets a copy
> >>    of the microcode for each of these 8 cpus, but not for the ones that were not
> >>    onlined while booting.
> >>    [Let us assume that cpu number 10 was one among the 8 cpus that were not onlined
> >>     while booting].
> >>
> >>    Later on, let's say we start our cpu hotplug + suspend/resume tests simultaneously.
> >>    Now consider this possible scenario:
> >>    
> >>    * Userspace is not frozen
> >>    * We initiate a cpu online operation on cpu 10. At the same time, since suspend
> >>      is in progress, lets say the freezing begins.
> >>    * Just before cpu 10 could be brought up online, userspace gets frozen.
> >>    * Now while bringing up cpu 10, due to the CPU_ONLINE_FROZEN notification, the
> >>      microcode core tries to apply the microcode to the cpu. But unfortunately, it
> >>      doesn't have the microcode! (because this cpu is coming up for the first time
> >>      and hence we never got its microcode from userspace...)
> >>
> >>      Now, again the same problem ensues: microcode core calls request_firmware and
> >>      depends on the (frozen) userspace to get the microcode.
> > 
> > Ok, but is this a real-life scenario you expect to happen somewhere or
> > is it something that happens only during test? IOW, if you have root
> > there are many ways to shoot yourself in the foot, right?
> > 
> 
> Well, honestly I was just trying to see in which all scenarios the patch
> would probably not work well... In real-life I don't expect to hit such
> a corner case!
> 
> > [..]
> > 
> >> I am still wondering if the approach I proposed earlier (the one in
> >> which we defer applying microcode and queue up a callback function
> >> etc) could solve all these issues. I am also playing around with the
> >> idea of coupling that with mutual exclusion between cpu hotplug and
> >> freezer to handle any problematic scenarios.
> > 
> > Well, all those solutions seem like they're not worth the trouble and
> > complexity if those cases are only conjecture - if you still trigger
> > them during your testing then probably mutually excluding freezer and
> > CPU hotplug is something I would lean towards but I could be wrong.
> >
> 
> Even I felt the same (moreover, that complex solution was not foolproof
> either!). Please see my other mail which talks about how just mutually
> excluding freezer and cpu hotplugging would solve everything.
>  
> > There's of course a much better fix which has been on the table for a
> > while now involving loading the ucode from the bootloader and applying
> > it much earlier than what we have now and keeping the ucode image in
> > memory. This would solve the CPU hotplug deal completely. Maybe it's
> > time I looked into it :-).
> > 
> 
> Assuming I understood this correctly, I can see some issues in this
> approach as well (since it is quite similar to the approach used in my
> one-line patch), but yeah, definitely they are all very much corner
> cases...

OK, can you please repost the patch with Borislav's Acked-by and Tested-by
and add some more Intel people to the CC list?

Rafael

Re: [BUGFIX][PATCH] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Srivatsa S. Bhat]

On 10/06/2011 01:56 AM, Rafael J. Wysocki wrote:
> 
> OK, can you please repost the patch with Borislav's Acked-by and Tested-by
> and add some more Intel people to the CC list?
> 

Sure, I'll do that. Thank you.
But are we not going to consider a cleaner/correct solution such as the
one proposed here:
http://permalink.gmane.org/gmane.linux.kernel/1199494

Well, honestly I don't mean to be stubborn, but somehow, knowing that
there are issues with my patch doesn't make me feel very comfortable
going with it, especially when there is another approach, which I
believe can fix the issue properly, without undesirable side-effects.

I agree that the issues are mostly some corner cases, so if you want a
quick fix for now, I guess we can go with this patch and then later on
follow-up with a proper solution to this whole problem.

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

Re: [BUGFIX][PATCH] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Rafael J. Wysocki]

On Wednesday, October 05, 2011, Srivatsa S. Bhat wrote:
> On 10/06/2011 01:56 AM, Rafael J. Wysocki wrote:
> > 
> > OK, can you please repost the patch with Borislav's Acked-by and Tested-by
> > and add some more Intel people to the CC list?
> > 
> 
> Sure, I'll do that. Thank you.
> But are we not going to consider a cleaner/correct solution such as the
> one proposed here:
> http://permalink.gmane.org/gmane.linux.kernel/1199494
> 
> Well, honestly I don't mean to be stubborn, but somehow, knowing that
> there are issues with my patch doesn't make me feel very comfortable
> going with it, especially when there is another approach, which I
> believe can fix the issue properly, without undesirable side-effects.
> 
> I agree that the issues are mostly some corner cases, so if you want a
> quick fix for now, I guess we can go with this patch and then later on
> follow-up with a proper solution to this whole problem.

It ultimately is your call.  If you feel more comfortable with the
alternative, just post that one instead.

Thanks,
Rafael

Re: [BUGFIX][PATCH] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Srivatsa S. Bhat]

On 10/06/2011 04:13 AM, Rafael J. Wysocki wrote:
> On Wednesday, October 05, 2011, Srivatsa S. Bhat wrote:
>> On 10/06/2011 01:56 AM, Rafael J. Wysocki wrote:
>>>
>>> OK, can you please repost the patch with Borislav's Acked-by and Tested-by
>>> and add some more Intel people to the CC list?
>>>
>>
>> Sure, I'll do that. Thank you.
>> But are we not going to consider a cleaner/correct solution such as the
>> one proposed here:
>> http://permalink.gmane.org/gmane.linux.kernel/1199494
>>
>> Well, honestly I don't mean to be stubborn, but somehow, knowing that
>> there are issues with my patch doesn't make me feel very comfortable
>> going with it, especially when there is another approach, which I
>> believe can fix the issue properly, without undesirable side-effects.
>>
>> I agree that the issues are mostly some corner cases, so if you want a
>> quick fix for now, I guess we can go with this patch and then later on
>> follow-up with a proper solution to this whole problem.
> 
> It ultimately is your call.  If you feel more comfortable with the
> alternative, just post that one instead.
> 
Cool! I am working on implementing that other solution. I'll post it as soon
as I am done writing and testing that patch.

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

Re: [BUGFIX][PATCH] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Borislav Petkov]

On Thu, Oct 06, 2011 at 02:50:46AM -0400, Srivatsa S. Bhat wrote:
> On 10/06/2011 04:13 AM, Rafael J. Wysocki wrote:
> > On Wednesday, October 05, 2011, Srivatsa S. Bhat wrote:
> >> On 10/06/2011 01:56 AM, Rafael J. Wysocki wrote:
> >>>
> >>> OK, can you please repost the patch with Borislav's Acked-by and Tested-by
> >>> and add some more Intel people to the CC list?
> >>>
> >>
> >> Sure, I'll do that. Thank you.
> >> But are we not going to consider a cleaner/correct solution such as the
> >> one proposed here:
> >> http://permalink.gmane.org/gmane.linux.kernel/1199494
> >>
> >> Well, honestly I don't mean to be stubborn, but somehow, knowing that
> >> there are issues with my patch doesn't make me feel very comfortable
> >> going with it, especially when there is another approach, which I
> >> believe can fix the issue properly, without undesirable side-effects.
> >>
> >> I agree that the issues are mostly some corner cases, so if you want a
> >> quick fix for now, I guess we can go with this patch and then later on
> >> follow-up with a proper solution to this whole problem.
> > 
> > It ultimately is your call.  If you feel more comfortable with the
> > alternative, just post that one instead.
> > 
> Cool! I am working on implementing that other solution. I'll post it as soon
> as I am done writing and testing that patch.

Please test your other patch which removes the CPU_DEAD line from the
microcode CPU hotplug callback on an Intel box with microcode too and
submit it. This fix makes sense irrespective of a suspend/resume fix
because reloading the ucode when onlining the CPU is clearly unneeded.

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: [BUGFIX][PATCH] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Srivatsa S. Bhat]

On 10/06/2011 02:04 PM, Borislav Petkov wrote:
> On Thu, Oct 06, 2011 at 02:50:46AM -0400, Srivatsa S. Bhat wrote:
>> On 10/06/2011 04:13 AM, Rafael J. Wysocki wrote:
>>> On Wednesday, October 05, 2011, Srivatsa S. Bhat wrote:
>>>> On 10/06/2011 01:56 AM, Rafael J. Wysocki wrote:
>>>>>
>>>>> OK, can you please repost the patch with Borislav's Acked-by and Tested-by
>>>>> and add some more Intel people to the CC list?
>>>>>
>>>>
>>>> Sure, I'll do that. Thank you.
>>>> But are we not going to consider a cleaner/correct solution such as the
>>>> one proposed here:
>>>> http://permalink.gmane.org/gmane.linux.kernel/1199494
>>>>
>>>> Well, honestly I don't mean to be stubborn, but somehow, knowing that
>>>> there are issues with my patch doesn't make me feel very comfortable
>>>> going with it, especially when there is another approach, which I
>>>> believe can fix the issue properly, without undesirable side-effects.
>>>>
>>>> I agree that the issues are mostly some corner cases, so if you want a
>>>> quick fix for now, I guess we can go with this patch and then later on
>>>> follow-up with a proper solution to this whole problem.
>>>
>>> It ultimately is your call.  If you feel more comfortable with the
>>> alternative, just post that one instead.
>>>
>> Cool! I am working on implementing that other solution. I'll post it as soon
>> as I am done writing and testing that patch.
> 
> Please test your other patch which removes the CPU_DEAD line from the
> microcode CPU hotplug callback on an Intel box with microcode too and
> submit it. This fix makes sense irrespective of a suspend/resume fix
> because reloading the ucode when onlining the CPU is clearly unneeded.
> 

Ok, I tested both these scenarios on Intel boxes:
1. cpu hotplug stress test + pm_test in parallel
2. loading/unloading microcode etc. 
They all work fine. I'll post that one-line patch with your Acked-by and Tested-by.
Thank you very much.

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

Re: [BUGFIX][PATCH] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Srivatsa S. Bhat]

On 10/06/2011 09:17 PM, Srivatsa S. Bhat wrote:
> On 10/06/2011 02:04 PM, Borislav Petkov wrote:
>> On Thu, Oct 06, 2011 at 02:50:46AM -0400, Srivatsa S. Bhat wrote:
>>> On 10/06/2011 04:13 AM, Rafael J. Wysocki wrote:
>>>> On Wednesday, October 05, 2011, Srivatsa S. Bhat wrote:
>>>>> On 10/06/2011 01:56 AM, Rafael J. Wysocki wrote:
>>>>>>
>>>>>> OK, can you please repost the patch with Borislav's Acked-by and Tested-by
>>>>>> and add some more Intel people to the CC list?
>>>>>>
>>>>>
>>>>> Sure, I'll do that. Thank you.
>>>>> But are we not going to consider a cleaner/correct solution such as the
>>>>> one proposed here:
>>>>> http://permalink.gmane.org/gmane.linux.kernel/1199494
>>>>>
>>>>> Well, honestly I don't mean to be stubborn, but somehow, knowing that
>>>>> there are issues with my patch doesn't make me feel very comfortable
>>>>> going with it, especially when there is another approach, which I
>>>>> believe can fix the issue properly, without undesirable side-effects.
>>>>>
>>>>> I agree that the issues are mostly some corner cases, so if you want a
>>>>> quick fix for now, I guess we can go with this patch and then later on
>>>>> follow-up with a proper solution to this whole problem.
>>>>
>>>> It ultimately is your call.  If you feel more comfortable with the
>>>> alternative, just post that one instead.
>>>>
>>> Cool! I am working on implementing that other solution. I'll post it as soon
>>> as I am done writing and testing that patch.
>>
>> Please test your other patch which removes the CPU_DEAD line from the
>> microcode CPU hotplug callback on an Intel box with microcode too and
>> submit it. This fix makes sense irrespective of a suspend/resume fix
>> because reloading the ucode when onlining the CPU is clearly unneeded.
>>
> 
> Ok, I tested both these scenarios on Intel boxes:
> 1. cpu hotplug stress test + pm_test in parallel
> 2. loading/unloading microcode etc. 
> They all work fine. I'll post that one-line patch with your Acked-by and Tested-by.
> Thank you very much.
> 

Well, unfortunately the following test case fails (not because of my patch, but rather
because my patch does not fix the root cause of the entire issue).

Wildly loading and unloading microcode driver and simultaneously running
pm_test(even at freezer level). "WARNING"s at drivers/base/firmware_class.c
appear similar to what we have seen before, just that the call stack is slightly
different. But we already know the precise reason why we hit this!

kernel: [  271.552553] microcode: CPU7 sig=0x206c2, pf=0x1, revision=0x13
kernel: [  271.552557] ------------[ cut here ]------------
kernel: [  271.552560] WARNING: at drivers/base/firmware_class.c:537 _request_firmware+0x423/0x440()
kernel: [  271.552562] Hardware name: BladeCenter HS22V -[7871G2A]-
kernel: [  271.552563] Modules linked in: microcode(+) ipmi_devintf ipmi_si ipmi_msghandler ipv6 cpufreq_conservative cpufreq_userspace cpufreq_powersave acpi_cpufreq mperf fuse loop dm_mod ioatdma tpm_tis serio_raw pcspkr sg tpm qla2xxx shpchp pci_hotplug i2c_i801 bnx2 dca scsi_transport_fc iTCO_wdt i2c_core iTCO_vendor_support mptctl scsi_tgt tpm_bios button uhci_hcd ehci_hcd usbcore sd_mod crc_t10dif edd ext3 mbcache jbd fan processor mptsas mptscsih mptbase scsi_transport_sas scsi_mod thermal thermal_sys hwmon [last unloaded: microcode]
kernel: [  271.552590] Pid: 19050, comm: modprobe Tainted: G        W   3.1.0-rc8-mc-notifier-fix-0.7-default #2
kernel: [  271.552592] Call Trace:
firmware.sh[19229]: Cannot find  firmware file 'intel-ucode/06-2c-02'
kernel: [  271.552595]  [<ffffffff812a34e3>] ? _request_firmware+0x423/0x440
kernel: [  271.552598]  [<ffffffff8104cbda>] warn_slowpath_common+0x7a/0xb0
kernel: [  271.552601]  [<ffffffff8104cc25>] warn_slowpath_null+0x15/0x20
kernel: [  271.552604]  [<ffffffff812a34e3>] _request_firmware+0x423/0x440
kernel: [  271.552607]  [<ffffffff812a3591>] request_firmware+0x11/0x20
kernel: [  271.552612]  [<ffffffffa1aa2d1c>] request_microcode_fw+0x5c/0xd0 [microcode]
kernel: [  271.552617]  [<ffffffffa1aa2368>] microcode_init_cpu+0xc8/0x120 [microcode]
kernel: [  271.552622]  [<ffffffffa1aa242a>] mc_sysdev_add+0x6a/0xa0 [microcode]
kernel: [  271.552626]  [<ffffffff812954b6>] sysdev_driver_register+0xc6/0x160
firmware.sh[19234]: Cannot find  firmware file 'intel-ucode/06-2c-02'
kernel: [  271.552630]  [<ffffffffa1abf000>] ? 0xffffffffa1abefff
kernel: [  271.552634]  [<ffffffffa1abf094>] microcode_init+0x94/0x15c [microcode]
kernel: [  271.552637]  [<ffffffff810001ce>] do_one_initcall+0x3e/0x180
kernel: [  271.552640]  [<ffffffff810855f9>] sys_init_module+0x89/0x1e0
kernel: [  271.552643]  [<ffffffff813c1452>] system_call_fastpath+0x16/0x1b
kernel: [  271.552646] ---[ end trace 1cfc5940e70d5532 ]---
kernel: [  271.552648] platform microcode: firmware: intel-ucode/06-2c-02 will not be loaded


Since I had never tried this scenario before, I missed this one.
But at least we now know that cpu hotplugging was just *one* of the call paths
that could trigger this issue... and that my patch took care of only that call path alone
and didn't really fix the root cause.

Probably we should add synchronization between microcode and freezer, to prevent the
relevant microcode call paths and the freezer from running in parallel.

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

[BUGFIX][PATCH RESEND] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Srivatsa S. Bhat]

This patch addresses the warnings found in the logs in the
task freezing failure bug reported in https://lkml.org/lkml/2011/9/5/28

The warnings appear because of the reason explained below:

There are microcode callbacks registered for CPU hotplug events such
as a CPU getting offlined or onlined. When a CPU is offlined
with tasks being frozen (as in the case of disabling the non-boot CPUs
while preparing for a system suspend operation), the CPU_DEAD_FROZEN
notification is sent, for which the microcode callback does not
do anything. In particular, it does not free or invalidate the CPU
microcode which it had got from userspace earlier. Hence when that CPU
comes back online with tasks being frozen (as in the case of re-enabling
the non-boot CPUs during a resume operation after suspend), the microcode
callback applies the microcode (which it already possesses) to that CPU.

However, during a pure CPU hotplug operation, tasks are not frozen and
hence the CPU_DEAD notification is sent. Upon this event notification,
the microcode callback frees the copy of microcode it has and
invalidates it. And during a CPU online, it tries to apply the microcode
to the CPU, but since it doesn't have the copy of the microcode, it depends
on a userspace utility to get the microcode. This is perfectly fine when
doing plain CPU hotplug operations alone.

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 addresses this issue by ensuring that the microcode is not
freed from kernel memory, nor invalidated when a CPU goes offline. So every
run of the microcode callback for CPU online event will now succeed
irrespective of whether the userspace is frozen or not, since the kernel will
reuse the microcode copy it already has, without depending on userspace. As a
result, this fixes the task freezing failure encountered while running CPU
hotplug stress test along with suspend/resume operations simultaneously.

Tested-by: Borislav Petkov <bp@amd64.org>
Acked-by: Borislav Petkov <bp@amd64.org>
Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

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

diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
index f924280..cd7ef2f 100644
--- a/arch/x86/kernel/microcode_core.c
+++ b/arch/x86/kernel/microcode_core.c
@@ -483,7 +483,15 @@ mc_cpu_callback(struct notifier_block *nb, unsigned long action, void *hcpu)
 		sysfs_remove_group(&sys_dev->kobj, &mc_attr_group);
 		pr_debug("CPU%d removed\n", cpu);
 		break;
-	case CPU_DEAD:
+
+	/*
+	 * Do not invalidate the microcode if a CPU goes offline,
+	 * because it would be impossible to get the microcode again
+	 * from userspace when the CPU comes back up, if the userspace
+	 * happens to be frozen at that moment by the freezer subsystem,
+	 * for example, due to a suspend operation in progress.
+	 */
+
 	case CPU_UP_CANCELED_FROZEN:
 		/* The CPU refused to come up during a system resume */
 		microcode_fini_cpu(cpu);

Re: [BUGFIX][PATCH RESEND] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Tejun Heo]

Hello,

On Fri, Oct 07, 2011 at 02:05:49AM +0530, Srivatsa S. Bhat wrote:
...
> diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
> index f924280..cd7ef2f 100644
> --- a/arch/x86/kernel/microcode_core.c
> +++ b/arch/x86/kernel/microcode_core.c
> @@ -483,7 +483,15 @@ mc_cpu_callback(struct notifier_block *nb, unsigned long action, void *hcpu)
>  		sysfs_remove_group(&sys_dev->kobj, &mc_attr_group);
>  		pr_debug("CPU%d removed\n", cpu);
>  		break;
> -	case CPU_DEAD:
> +
> +	/*
> +	 * Do not invalidate the microcode if a CPU goes offline,
> +	 * because it would be impossible to get the microcode again
> +	 * from userspace when the CPU comes back up, if the userspace
> +	 * happens to be frozen at that moment by the freezer subsystem,
> +	 * for example, due to a suspend operation in progress.
> +	 */
> +

This still looks like a bandaid to me.  The exclusion approach didn't
pan out?

Thank you.

-- 
tejun

Re: [BUGFIX][PATCH RESEND] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Borislav Petkov]

On Thu, Oct 06, 2011 at 06:13:34PM -0400, Tejun Heo wrote:
> Hello,
> 
> On Fri, Oct 07, 2011 at 02:05:49AM +0530, Srivatsa S. Bhat wrote:
> ...
> > diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
> > index f924280..cd7ef2f 100644
> > --- a/arch/x86/kernel/microcode_core.c
> > +++ b/arch/x86/kernel/microcode_core.c
> > @@ -483,7 +483,15 @@ mc_cpu_callback(struct notifier_block *nb, unsigned long action, void *hcpu)
> >  		sysfs_remove_group(&sys_dev->kobj, &mc_attr_group);
> >  		pr_debug("CPU%d removed\n", cpu);
> >  		break;
> > -	case CPU_DEAD:
> > +
> > +	/*
> > +	 * Do not invalidate the microcode if a CPU goes offline,
> > +	 * because it would be impossible to get the microcode again
> > +	 * from userspace when the CPU comes back up, if the userspace
> > +	 * happens to be frozen at that moment by the freezer subsystem,
> > +	 * for example, due to a suspend operation in progress.
> > +	 */
> > +
> 
> This still looks like a bandaid to me.  The exclusion approach didn't
> pan out?

Well, this saves us the needless reloading of the ucode image when the
CPU comes back online and is an annoyance fix and onlining path speedup
in its own right. I, as you, thought that the exclusion approach should
be the proper fix. Srivatsa?

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: [BUGFIX][PATCH RESEND] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Srivatsa S. Bhat]

On 10/07/2011 04:04 AM, Borislav Petkov wrote:
> On Thu, Oct 06, 2011 at 06:13:34PM -0400, Tejun Heo wrote:
>> Hello,
>>
>> On Fri, Oct 07, 2011 at 02:05:49AM +0530, Srivatsa S. Bhat wrote:
>> ...
>>> diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
>>> index f924280..cd7ef2f 100644
>>> --- a/arch/x86/kernel/microcode_core.c
>>> +++ b/arch/x86/kernel/microcode_core.c
>>> @@ -483,7 +483,15 @@ mc_cpu_callback(struct notifier_block *nb, unsigned long action, void *hcpu)
>>>  		sysfs_remove_group(&sys_dev->kobj, &mc_attr_group);
>>>  		pr_debug("CPU%d removed\n", cpu);
>>>  		break;
>>> -	case CPU_DEAD:
>>> +
>>> +	/*
>>> +	 * Do not invalidate the microcode if a CPU goes offline,
>>> +	 * because it would be impossible to get the microcode again
>>> +	 * from userspace when the CPU comes back up, if the userspace
>>> +	 * happens to be frozen at that moment by the freezer subsystem,
>>> +	 * for example, due to a suspend operation in progress.
>>> +	 */
>>> +
>>
>> This still looks like a bandaid to me.  The exclusion approach didn't
>> pan out?
> 
> Well, this saves us the needless reloading of the ucode image when the
> CPU comes back online and is an annoyance fix and onlining path speedup
> in its own right. I, as you, thought that the exclusion approach should
> be the proper fix. Srivatsa?
> 

Boris, it is only now (after you explained) that I really understood why you
saw value in this patch (even though it was not the proper fix). So actually
this patch is just a good-to-have cpu hotplug optimization, but the real fix
would be the exclusion approach. More than that, this patch has got nothing
intentional to do with freezer, but its motivation is just to avoid doing
something needless in the cpu hotplug path. And an entirely different patch
(having the exclusion stuff) is needed to properly fix the problem we are
facing. This is what you mean right?

If so, then in a way we are trying to reposition why we need this patch. And
since we don't want to position this as a fix to this problem, I should
probably submit this patch with a different patch description and subject,
to explain the new usecase/motivation for this patch. Am I right?


By the way, even I believe that the exclusion approach is the best fix to the
problem. (I have been mulling about this in some of my previous mails as well).
At least we can see 3 call paths that get into trouble when racing with
freezer:
1. CPU hotplug.
2. Microcode module load/unload.
3. Reloading the microcode by controlling the sysfs file
/sys/devices/system/cpu/cpu*/microcode/reload. See below for log for this new
scenario.

At least this is what I got from looking at the microcode call paths that
involve a call to request_firmware.

I am still working on implementing the mutual exclusion at appropriate
places. However, since any of this would involve locking, with the
freezer/suspend involved as well (and especially since cpu hotplug is
used by the suspend code itself), I am trying to tread cautiously
(read: needing more time) to ensure that I don't introduce incorrect locking
scenarios and hence task freezing failures myself, while intending to fix it.


Log (warnings while reloading microcode by writing 1 to reload file in sysfs
while simultaneously running freezer):

[58247.407637] ------------[ cut here ]------------^M
[58247.407642] WARNING: at drivers/base/firmware_class.c:537 _request_firmware+0x423/0x440()^M
[58247.407644] Hardware name: BladeCenter HS22V -[7871G2A]-^M
[58247.407646] Modules linked in: ipmi_devintf ipmi_si ipmi_msghandler ipv6 cpufreq_conservative cpufreq_userspace cpufreq_powersave acpi_cpufreq mperf microcode fuse loop dm_mod sg qla2xxx tpm_tis ioatdma i2c_i801 shpchp serio_raw pcspkr iTCO_wdt i2c_core bnx2 pci_hotplug scsi_transport_fc tpm iTCO_vendor_support mptctl dca scsi_tgt tpm_bios button uhci_hcd ehci_hcd usbcore sd_mod crc_t10dif edd ext3 mbcache jbd fan processor mptsas mptscsih mptbase scsi_transport_sas scsi_mod thermal thermal_sys hwmon^M [58247.407682] Pid: 16294, comm: reload_microcod Tainted: G        W   3.1.0-rc8-mc-notifier-fix-0.7-default #2^M
[58247.407685] Call Trace:^M
[58247.407689]  [<ffffffff812a34e3>] ? _request_firmware+0x423/0x440^M
[58247.407693]  [<ffffffff8104cbda>] warn_slowpath_common+0x7a/0xb0^M
[58247.407697]  [<ffffffff8104cc25>] warn_slowpath_null+0x15/0x20^M
[58247.407701]  [<ffffffff812a34e3>] _request_firmware+0x423/0x440^M
[58247.407705]  [<ffffffff812a3591>] request_firmware+0x11/0x20^M
[58247.407711]  [<ffffffffa01a0d1c>] request_microcode_fw+0x5c/0xd0 [microcode]^M
[58247.407716]  [<ffffffffa01a0250>] reload_store+0xc0/0x110 [microcode]^M
[58247.407722]  [<ffffffff81294fbb>] sysdev_store+0x1b/0x20^M
[58247.407726]  [<ffffffff81178222>] sysfs_write_file+0xc2/0x130^M
[58247.407731]  [<ffffffff8110fdab>] vfs_write+0xcb/0x180^M
[58247.407735]  [<ffffffff8110ff50>] sys_write+0x50/0x90^M
[58247.407739]  [<ffffffff813c1452>] system_call_fastpath+0x16/0x1b^M
[58247.407742] ---[ end trace 87b5d9a227b8fcf8 ]---^M
[58247.407744] platform microcode: firmware: intel-ucode/06-2c-02 will not be loaded^M

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

Re: [BUGFIX][PATCH RESEND] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Borislav Petkov]

On Fri, Oct 07, 2011 at 12:48:17PM -0400, Srivatsa S. Bhat wrote:
> Boris, it is only now (after you explained) that I really understood why you
> saw value in this patch (even though it was not the proper fix). So actually
> this patch is just a good-to-have cpu hotplug optimization, but the real fix
> would be the exclusion approach. More than that, this patch has got nothing
> intentional to do with freezer, but its motivation is just to avoid doing
> something needless in the cpu hotplug path. And an entirely different patch
> (having the exclusion stuff) is needed to properly fix the problem we are
> facing. This is what you mean right?
> 
> If so, then in a way we are trying to reposition why we need this patch. And
> since we don't want to position this as a fix to this problem, I should
> probably submit this patch with a different patch description and subject,
> to explain the new usecase/motivation for this patch. Am I right?

Absolutely, right on the money. Just write a short commit message
explaining why it does what it does and send it to x86 guys. Thanks for
that.

> By the way, even I believe that the exclusion approach is the best fix to the
> problem. (I have been mulling about this in some of my previous mails as well).
> At least we can see 3 call paths that get into trouble when racing with
> freezer:
> 1. CPU hotplug.
> 2. Microcode module load/unload.
> 3. Reloading the microcode by controlling the sysfs file
> /sys/devices/system/cpu/cpu*/microcode/reload. See below for log for this new
> scenario.

Please note that microcode is not supposed to be reloaded that often and
the box suspended at the same time as your test does. So I don't really
consider it relevant case - normally, you either update your microcode
XOR hibernate the box. Besides, microcode is not something you get on a
monthly basis to require such often updates.

> At least this is what I got from looking at the microcode call paths that
> involve a call to request_firmware.
> 
> I am still working on implementing the mutual exclusion at appropriate
> places. However, since any of this would involve locking, with the
> freezer/suspend involved as well (and especially since cpu hotplug is
> used by the suspend code itself), I am trying to tread cautiously
> (read: needing more time) to ensure that I don't introduce incorrect locking
> scenarios and hence task freezing failures myself, while intending to fix it.

IMO, you should concentrate on fixing _your_ use case and where your
testing fails instead of trying to cover all hypothetical failure
scenarios. Let's say that that's impossible, and also, doing the ucode
loading through the boot loader should take care of all those later.

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: [BUGFIX][PATCH] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Borislav Petkov]

On Tue, Oct 04, 2011 at 12:15:08AM -0700, Tejun Heo wrote:
> On Mon, Oct 03, 2011 at 10:47:54AM +0200, Borislav Petkov wrote:
> > I think your patch makes sense because re-loading the ucode during
> > a suspend/resume cycle is unnecessary. If one wants to update the
> > microcode, it should happen later when the box is resumed again: you
> > simply put the new microcode image in /lib/firmware/... and on AMD
> > unload/reload the microcode module and on Intel you do either that or
> > use the deprecated microcode_ctl.
> 
> I don't think it changes anything for suspend/resume cycles.  They're
> different hooks.  The proposed patch changes actual cpu hotplug paths.

Well, we're offlining the CPUs through the same hotplug path when
suspending and since currently the microcode core unnecessarily
re-requests the ucode image from userspace on resume, I still think the
patch makes sense. Especially if Srivatsa does suspend/resume and CPU
hotplugging simultaneously in a test and upon onlining a CPU, he manages
of doing request_firmware on a frozen userspace.

Srivatsa, is my understanding correct?

Thanks.

-- 
Regards/Gruss,
Boris.

Re: [BUGFIX][PATCH] Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

[Srivatsa S. Bhat]

On 10/03/2011 11:21 AM, Srivatsa S. Bhat wrote:
> Hi,
> 
> First of all, thank you for your invaluable inputs.
> 
> On 10/03/2011 06:10 AM, Tejun Heo wrote:
>> Hello,
[...]
>>
>> Can't one take a cpu offline, hot unplug it from the board and put in
>> a new one and then bring it online?  That's usually what "hot
>> [un]plug" stands for.  I don't think one would be able to put in a
>> very different processor but maybe, say, revision difference is
>> allowed and microcode should be looked up again?  Assuming that the
>> same microcode can always be applied after the actual CPU is swapped
>> could be dangerous.  Again, I'm not sure about how this is supposed to
>> be managed so I could be wrong.
>>
[...]
>> If I'm not wrong, can't we add synchronization between cpu hotplug and
>> freezer so that they don't happen in parallel?
>>
> 
> I have posted another patch related to CPU hotplug and freezer here:
> https://lkml.org/lkml/2011/10/2/163
> That patch aims to sync up the CPU hotplug infrastructure with the activity of the 
> freezer and hence ensure that the right notifications are sent. Maybe I could easily
> modify that patch to disable CPU hotplugging when the freezer is active.
> 
> But still, that would not solve our problem due to the following possible scenario:
> 
> [Let us assume that we go ahead and invalidate the microcode upon a CPU_DEAD notification,
> like you suggested.] Then, consider this scenario:
> 
> * Take a CPU offline (a pure CPU hotplug operation, not related to suspend).
>   Let us call this CPU X.
>   - This would involve a CPU_DEAD notification, upon which the microcode is invalidated.
> 
> * Start freezing tasks (due to a suspend operation in progress). Now we disable
>   CPU hotplugging.
>  
> * Disable (offline) the non-boot CPUs as part of suspend operation. This sends the
> CPU_DEAD_FROZEN notification, upon which the microcode is NOT invalidated for the other CPUs.
> 
> * Enable (online) the non-boot CPUs as part of resume operation. Please note that the tasks
> are still frozen. This is where we hit the same issue again! When trying to bring that CPU X
> back online, it finds that the microcode has been invalidated and hence tries to request the
> userspace for the microcode, but alas, the userspace is still frozen at that moment.
> This is the exact problem we were trying to solve earlier, which remains unsolved by disabling
> CPU hotplug during freezer operations!
> 

Sorry, my understanding was wrong in this part. After digging some more into
the code, I found that disable_nonboot_cpus() will only offline the currently
online cpus, and enable_nonboot_cpus() will only online those cpus that were
offlined during the disable_nonboot_cpus() phase (which it keeps track by using
a frozen_cpus mask).

So the problem I mentioned above wouldn't arise because during resume,
enable_nonboot_cpus() wouldn't try to online CPU X.

I also found that both disable_nonboot_cpus() and enable_nonboot_cpus() will
disable cpu hotplugging when they are active.

So a suspend/resume on a high level with reference to only freezer and cpu
hotplug, would look something like:


Freeze -> Disable nonboot cpus -> do suspend -> Enable nonboot cpus -> Thaw
         |<------------ CPU hotplugging disabled ----------------->|


So now as we can see, if we just prevent freezer and cpu hotplug from occurring
in parallel (like what Tejun had suggested above), we could solve this whole
issue properly. We wouldn't need anything complicated such as special callbacks
etc which I was proposing in some of my other mails (which anyway wouldn't work
as it is, due to some race conditions that I figured out later)...

So if we go by the above solution, then the situation would look like:

Freeze -> Disable nonboot cpus -> do suspend -> Enable nonboot cpus -> Thaw
|<---------------------- CPU hotplugging disabled ----------------------->|


Let me summarize how this solution works:
1. Any CPU that was offline (either because it was never brought up during
   booting or because due to a cpu hotplug operation, we had offlined it)
   before suspend starts, will never be onlined (or even tried to online)
   till resume is complete. After resume, a suitable cpu hotplug operation
   would put such a cpu online, if necessary.
   So, it doesn't matter whether the kernel has already got the microcode
   for that cpu or not, because it will be onlined only when the resume is
   complete (ie., userspace is thawed).

2. Physical cpu hotplugging or any scenario which needs us to apply a revised
   microcode:
   Here, since we hotplug cpus (online or offline) only when the freezer is not
   active, we won't have any trouble getting microcode from userspace upon cpu
   online.


But then, will introducing this restriction that "freezer and cpu hotplug must
be mutually exclusive", create any problems? (I am asking this question because
I have heard that the freezer is used in some cases other than in suspend/resume
too). I don't seem to find anything obvious... So I guess that restriction
should be quite acceptable...
Any comments?

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