Freezer, CPU hotplug, x86 Microcode: Fix task freezing failures

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

---

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

arch/x86/kernel/microcode_core.c +9 -1

index f924280..cd7ef2f 100644
@@ -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);

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.

Hello,

Quoted 8 lines · Srivatsa S. Bhat

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?

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.

Hello,

Quoted 4 lines · Srivatsa S. Bhat

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?

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.

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.

Quoted 5 lines

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 :-).

Hello,

Quoted 6 lines · Borislav Petkov

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.

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.

Quoted 11 lines · Srivatsa S. Bhat

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.

Quoted 4 lines

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.

Quoted 12 lines · Borislav Petkov

Quoted 11 lines · Srivatsa S. Bhat

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.

Quoted 7 lines

Quoted 4 lines

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.

Quoted 12 lines · Borislav Petkov

Quoted 11 lines · Srivatsa S. Bhat

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.

Quoted 7 lines

Quoted 4 lines

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.

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.

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.

Quoted 5 lines · Srivatsa S. Bhat

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.

Quoted 19 lines

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...)

Quoted 2 lines

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?

[..]

Quoted 5 lines

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 :-).

Quoted 11 lines · Borislav Petkov

Quoted 5 lines · Srivatsa S. Bhat

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

Quoted 19 lines

Quoted 19 lines

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...)

Quoted 7 lines

Quoted 2 lines

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!

Quoted 8 lines

[..]

Quoted 5 lines

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.

Quoted 5 lines

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.

Quoted 6 lines

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?

Quoted 4 lines · Rafael J. Wysocki

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.

Quoted 16 lines · Srivatsa S. Bhat

Quoted 4 lines · Rafael J. Wysocki

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.

Quoted 3 lines

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.

Quoted 21 lines · Rafael J. Wysocki

Quoted 19 lines · Srivatsa S. Bhat

Quoted 4 lines · Rafael J. Wysocki

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.

Quoted 3 lines

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.

Quoted 25 lines · Srivatsa S. Bhat

Quoted 24 lines · Rafael J. Wysocki

Quoted 11 lines · Srivatsa S. Bhat

Quoted 4 lines · Rafael J. Wysocki

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

Quoted 8 lines

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.

Quoted 2 lines

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.

Quoted 26 lines · Borislav Petkov

Quoted 25 lines · Srivatsa S. Bhat

Quoted 24 lines · Rafael J. Wysocki

Quoted 11 lines · Srivatsa S. Bhat

Quoted 4 lines · Rafael J. Wysocki

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

Quoted 8 lines

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.

Quoted 8 lines

Quoted 2 lines

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.

arch/x86/kernel/microcode_core.c +0 -0

index f924280..cd7ef2f 100644
@@ -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;

Hello,

On Fri, Oct 07, 2011 at 02:05:49AM +0530, Srivatsa S. Bhat wrote:
...

Quoted 13 lines

arch/x86/kernel/microcode_core.c +4 -1

index f924280..cd7ef2f 100644
@@ -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

arch/x86/kernel/microcode_core.c mc_cpu_callback(struct notifier_block *nb, unsigned long action, void *hcpu) · line 494

Quoted 5 lines

arch/x86/kernel/microcode_core.c +5 -0

+	 * 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?

Quoted 17 lines · Tejun Heo

Hello,

On Fri, Oct 07, 2011 at 02:05:49AM +0530, Srivatsa S. Bhat wrote:
...

Quoted 13 lines

arch/x86/kernel/microcode_core.c +4 -1

index f924280..cd7ef2f 100644
@@ -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

Quoted 8 lines

arch/x86/kernel/microcode_core.c mc_cpu_callback(struct notifier_block *nb, unsigned long action, void *hcpu) · line 494

Quoted 5 lines

arch/x86/kernel/microcode_core.c +5 -0

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

Quoted 6 lines · Borislav Petkov

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,

First of all, thank you for your invaluable inputs.

Quoted 1 line · Tejun Heo

Hello,

Quoted 10 lines

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.

Quoted 3 lines

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!