[PATCH] x86: Fix thermal throttling reporting after kexec

[PATCH] x86: Fix thermal throttling reporting after kexec

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

The per CPU thermal vector init code checks if the thermal
vector is already installed and complains and bails out if
it is.

This happens after kexec, as kernel shut down does
not clear the thermal vector APIC register.

This causes two problems:

So we always do not fully initialize thermal reports
after kexec. The CPU is still likely initialized,
as the previous kernel should have done it. But
we don't set up the software pointer to the thermal
vector, so reporting may end up with a unknown thermal
interrupt message.

Also it complains for every logical CPU, even though the
value is actually derived from BP only.

The problem is that we end up with one message per CPU,
so on larger systems it becomes very noisy and messes up
the otherwise nicely formatted CPU bootup numbers in
the kernel log.

Just remove the check. I checked the code and there's
no valid code paths where the thermal init code for a CPU
could be called multiple times.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/kernel/cpu/mcheck/therm_throt.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
index 1af51b1..2c5aaf8 100644
--- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
+++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
@@ -503,14 +503,6 @@ void intel_init_thermal(struct cpuinfo_x86 *c)
 		return;
 	}
 
-	/* Check whether a vector already exists */
-	if (h & APIC_VECTOR_MASK) {
-		printk(KERN_DEBUG
-		       "CPU%d: Thermal LVT vector (%#x) already installed\n",
-		       cpu, (h & APIC_VECTOR_MASK));
-		return;
-	}
-
 	/* early Pentium M models use different method for enabling TM2 */
 	if (cpu_has(c, X86_FEATURE_TM2)) {
 		if (c->x86 == 6 && (c->x86_model == 9 || c->x86_model == 13)) {
-- 
2.4.3

Re: [PATCH] x86: Fix thermal throttling reporting after kexec

[Thomas Gleixner]

On Thu, 24 Sep 2015, Andi Kleen wrote:
> The per CPU thermal vector init code checks if the thermal
> vector is already installed and complains and bails out if
> it is.
> 
> This happens after kexec, as kernel shut down does
> not clear the thermal vector APIC register.

So the obvious question is, why don't we do that.

> Just remove the check. I checked the code and there's
> no valid code paths where the thermal init code for a CPU
> could be called multiple times.

I'm not against removing that check as it does not really add value,
but we still should clear the APIC register at shut down, right?

Thanks,

	tglx

Re: [PATCH] x86: Fix thermal throttling reporting after kexec

[Andi Kleen]

On Thu, Oct 01, 2015 at 02:15:54PM +0200, Thomas Gleixner wrote:
> On Thu, 24 Sep 2015, Andi Kleen wrote:
> > The per CPU thermal vector init code checks if the thermal
> > vector is already installed and complains and bails out if
> > it is.
> > 
> > This happens after kexec, as kernel shut down does
> > not clear the thermal vector APIC register.
> 
> So the obvious question is, why don't we do that.

It wouldn't help if the previous kernel is some older kernel.

> 
> > Just remove the check. I checked the code and there's
> > no valid code paths where the thermal init code for a CPU
> > could be called multiple times.
> 
> I'm not against removing that check as it does not really add value,
> but we still should clear the APIC register at shut down, right?

The vector register is really harmless by itself and apart from
bogus checking it's not really affecting anyone. It may make
more sense to disable thermal reporting on shut down though.

I can add that, although it would only be useful for the more
theoretical case when you boot non Linux after kexec (as normal
Linux always reenables it anyways)

But the check should still be removed imho.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only

Re: [PATCH] x86: Fix thermal throttling reporting after kexec

[Thomas Gleixner]

On Thu, 1 Oct 2015, Andi Kleen wrote:
> On Thu, Oct 01, 2015 at 02:15:54PM +0200, Thomas Gleixner wrote:
> > On Thu, 24 Sep 2015, Andi Kleen wrote:
> > > The per CPU thermal vector init code checks if the thermal
> > > vector is already installed and complains and bails out if
> > > it is.
> > > 
> > > This happens after kexec, as kernel shut down does
> > > not clear the thermal vector APIC register.
> > 
> > So the obvious question is, why don't we do that.
> 
> It wouldn't help if the previous kernel is some older kernel.

I know.
 
> > 
> > > Just remove the check. I checked the code and there's
> > > no valid code paths where the thermal init code for a CPU
> > > could be called multiple times.
> > 
> > I'm not against removing that check as it does not really add value,
> > but we still should clear the APIC register at shut down, right?
> 
> The vector register is really harmless by itself and apart from
> bogus checking it's not really affecting anyone. It may make
> more sense to disable thermal reporting on shut down though.
> 
> I can add that, although it would only be useful for the more
> theoretical case when you boot non Linux after kexec (as normal
> Linux always reenables it anyways)

I see it under the correctness aspect. Mop up before you shut down.
 
> But the check should still be removed imho.

As I said before: I have no objections against that.

Thanks,

	tglx

Re: [PATCH] x86: Fix thermal throttling reporting after kexec

[Andi Kleen]

> I see it under the correctness aspect. Mop up before you shut down.

Ok. I suspect if you want to clean up all registers there's much more
to do.

BTW there's a small danger in it: if we ever crash accessing on
of those registers panic may end up looping.

-Andi

Re: [PATCH] x86: Fix thermal throttling reporting after kexec

[Andi Kleen]

On Thu, Oct 01, 2015 at 11:50:00PM +0200, Andi Kleen wrote:
> > I see it under the correctness aspect. Mop up before you shut down.
> 
> Ok. I suspect if you want to clean up all registers there's much more
> to do.
> 
> BTW there's a small danger in it: if we ever crash accessing on
> of those registers panic may end up looping.

I thought more about it. Since this is per logical CPU state
the cleanup cannot be done in a normal shutdown callback (which
only runs on one CPU), but needs some kind of global IPI/NMI.

IPI could deadlock, so it would need to be NMI.

KVM already has one, but would need to re-organize that into
first into a generic callback infrastructure.

I don't think so much change is worth it for this one
somewhat dubious case. NMI code is also tricky and it's
probably better to keep the shut down paths as simple
and reliable as possible. Do you agree?

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only

Re: [PATCH] x86: Fix thermal throttling reporting after kexec

[Thomas Gleixner]

On Thu, 1 Oct 2015, Andi Kleen wrote:
> On Thu, Oct 01, 2015 at 11:50:00PM +0200, Andi Kleen wrote:
> > > I see it under the correctness aspect. Mop up before you shut down.
> > 
> > Ok. I suspect if you want to clean up all registers there's much more
> > to do.
> > 
> > BTW there's a small danger in it: if we ever crash accessing on
> > of those registers panic may end up looping.
> 
> I thought more about it. Since this is per logical CPU state
> the cleanup cannot be done in a normal shutdown callback (which
> only runs on one CPU), but needs some kind of global IPI/NMI.
> 
> IPI could deadlock, so it would need to be NMI.
> 
> KVM already has one, but would need to re-organize that into
> first into a generic callback infrastructure.
> 
> I don't think so much change is worth it for this one
> somewhat dubious case. NMI code is also tricky and it's
> probably better to keep the shut down paths as simple
> and reliable as possible. Do you agree?

Yes. Makes sense.

Thanks for analysing it.

	tglx

Re: [PATCH] x86: Fix thermal throttling reporting after kexec

[Thomas Gleixner]

Andi,

On Fri, 2 Oct 2015, Thomas Gleixner wrote:
> On Thu, 1 Oct 2015, Andi Kleen wrote:
> > On Thu, Oct 01, 2015 at 11:50:00PM +0200, Andi Kleen wrote:
> > > > I see it under the correctness aspect. Mop up before you shut down.
> > > 
> > > Ok. I suspect if you want to clean up all registers there's much more
> > > to do.
> > > 
> > > BTW there's a small danger in it: if we ever crash accessing on
> > > of those registers panic may end up looping.
> > 
> > I thought more about it. Since this is per logical CPU state
> > the cleanup cannot be done in a normal shutdown callback (which
> > only runs on one CPU), but needs some kind of global IPI/NMI.
> > 
> > IPI could deadlock, so it would need to be NMI.
> > 
> > KVM already has one, but would need to re-organize that into
> > first into a generic callback infrastructure.
> > 
> > I don't think so much change is worth it for this one
> > somewhat dubious case. NMI code is also tricky and it's
> > probably better to keep the shut down paths as simple
> > and reliable as possible. Do you agree?
> 
> Yes. Makes sense.
> 
> Thanks for analysing it.

I wanted to pick this up, but then I noticed that it actually belongs
to X86 MCE INFRASTRUCTURE which is maintained by Tony and Borislav.

Can you please resend to them? It would be nice if you could add the
discussion about not cleaning it up in the changelog.

Thanks and sorry for noticing so late,

       tglx

Re: [PATCH] x86: Fix thermal throttling reporting after kexec

[Borislav Petkov]

On Mon, Oct 12, 2015 at 10:33:53PM +0200, Thomas Gleixner wrote:
> On Mon, 12 Oct 2015, Andi Kleen wrote:
> > From: Andi Kleen <ak@linux.intel.com>
> > 
> > The per CPU thermal vector init code checks if the thermal
> > vector is already installed and complains and bails out if
> > it is.
> > 
> > This happens after kexec, as kernel shut down does
> > not clear the thermal vector APIC register.
> > 
> > This causes two problems:
> > 
> > So we always do not fully initialize thermal reports
> > after kexec. The CPU is still likely initialized,
> > as the previous kernel should have done it. But
> > we don't set up the software pointer to the thermal
> > vector, so reporting may end up with a unknown thermal
> > interrupt message.
> > 
> > Also it complains for every logical CPU, even though the
> > value is actually derived from BP only.
> > 
> > The problem is that we end up with one message per CPU,
> > so on larger systems it becomes very noisy and messes up
> > the otherwise nicely formatted CPU bootup numbers in
> > the kernel log.
> > 
> > Just remove the check. I checked the code and there's
> > no valid code paths where the thermal init code for a CPU
> > could be called multiple times.
> > 
> > Why the kernel does not clean up this value on shutdown:
> > 
> > The thermal monitoring is controlled per logical CPU thread.
> > Normal shutdown code is just running on one CPU.
> > To disable it we would need a broadcast NMI to all CPUs
> > on shut down. That's overkill for this. So we just
> > ignore it after kexec.
> > 
> > v2: Updated commit log to discuss why the value is not
> > cleaned up on shutdown.
> > Signed-off-by: Andi Kleen <ak@linux.intel.com>
> 
> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Applied,
thanks!

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH] x86: Fix thermal throttling reporting after kexec

[Thomas Gleixner]

On Mon, 12 Oct 2015, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> The per CPU thermal vector init code checks if the thermal
> vector is already installed and complains and bails out if
> it is.
> 
> This happens after kexec, as kernel shut down does
> not clear the thermal vector APIC register.
> 
> This causes two problems:
> 
> So we always do not fully initialize thermal reports
> after kexec. The CPU is still likely initialized,
> as the previous kernel should have done it. But
> we don't set up the software pointer to the thermal
> vector, so reporting may end up with a unknown thermal
> interrupt message.
> 
> Also it complains for every logical CPU, even though the
> value is actually derived from BP only.
> 
> The problem is that we end up with one message per CPU,
> so on larger systems it becomes very noisy and messes up
> the otherwise nicely formatted CPU bootup numbers in
> the kernel log.
> 
> Just remove the check. I checked the code and there's
> no valid code paths where the thermal init code for a CPU
> could be called multiple times.
> 
> Why the kernel does not clean up this value on shutdown:
> 
> The thermal monitoring is controlled per logical CPU thread.
> Normal shutdown code is just running on one CPU.
> To disable it we would need a broadcast NMI to all CPUs
> on shut down. That's overkill for this. So we just
> ignore it after kexec.
> 
> v2: Updated commit log to discuss why the value is not
> cleaned up on shutdown.
> Signed-off-by: Andi Kleen <ak@linux.intel.com>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

[PATCH] x86: Fix thermal throttling reporting after kexec

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

The per CPU thermal vector init code checks if the thermal
vector is already installed and complains and bails out if
it is.

This happens after kexec, as kernel shut down does
not clear the thermal vector APIC register.

This causes two problems:

So we always do not fully initialize thermal reports
after kexec. The CPU is still likely initialized,
as the previous kernel should have done it. But
we don't set up the software pointer to the thermal
vector, so reporting may end up with a unknown thermal
interrupt message.

Also it complains for every logical CPU, even though the
value is actually derived from BP only.

The problem is that we end up with one message per CPU,
so on larger systems it becomes very noisy and messes up
the otherwise nicely formatted CPU bootup numbers in
the kernel log.

Just remove the check. I checked the code and there's
no valid code paths where the thermal init code for a CPU
could be called multiple times.

Why the kernel does not clean up this value on shutdown:

The thermal monitoring is controlled per logical CPU thread.
Normal shutdown code is just running on one CPU.
To disable it we would need a broadcast NMI to all CPUs
on shut down. That's overkill for this. So we just
ignore it after kexec.

v2: Updated commit log to discuss why the value is not
cleaned up on shutdown.
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/kernel/cpu/mcheck/therm_throt.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
index 1af51b1..2c5aaf8 100644
--- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
+++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
@@ -503,14 +503,6 @@ void intel_init_thermal(struct cpuinfo_x86 *c)
 		return;
 	}
 
-	/* Check whether a vector already exists */
-	if (h & APIC_VECTOR_MASK) {
-		printk(KERN_DEBUG
-		       "CPU%d: Thermal LVT vector (%#x) already installed\n",
-		       cpu, (h & APIC_VECTOR_MASK));
-		return;
-	}
-
 	/* early Pentium M models use different method for enabling TM2 */
 	if (cpu_has(c, X86_FEATURE_TM2)) {
 		if (c->x86 == 6 && (c->x86_model == 9 || c->x86_model == 13)) {
-- 
2.4.3