[PATCH] x86: c1e_idle: don't mark TSC unstable if CPU has invariant TSC

[PATCH] x86: c1e_idle: don't mark TSC unstable if CPU has invariant TSC

[Andreas Herrmann]

.. otherwise TSC is marked unstable on AMD family 0x10 and 0x11 CPUs.
This would be wrong because for those CPUs "invariant TSC" means:

   "The TSC counts at the same rate in all P-states, all C states, S0,
   or S1"

(See "Processor BIOS and Kernel Developer's Guides" for those CPUs.)

Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>

---
 arch/x86/kernel/process.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

Please apply for 2.6.27.

Thanks,


Andreas


diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 7fc4d5b..ba6b16b 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -265,7 +265,8 @@ static void c1e_idle(void)
 		rdmsr(MSR_K8_INT_PENDING_MSG, lo, hi);
 		if (lo & K8_INTP_C1E_ACTIVE_MASK) {
 			c1e_detected = 1;
-			mark_tsc_unstable("TSC halt in C1E");
+			if (!boot_cpu_has(X86_FEATURE_CONSTANT_TSC))
+				mark_tsc_unstable("TSC halt in C1E");
 			printk(KERN_INFO "System has C1E enabled\n");
 		}
 	}
-- 
1.6.0.1

Re: [PATCH] x86: c1e_idle: don't mark TSC unstable if CPU has invariant TSC

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 338 bytes --]

On Thu, 18 Sep 2008 21:12:10 +0200, Andreas Herrmann said:
> .. otherwise TSC is marked unstable on AMD family 0x10 and 0x11 CPUs.

OK.  I'll bite (admittedly not having looked at the actual code yet).

If the TSC is in fact invariant, what's causing the kernel to mark it as
unstable? Sounds almost like a bug being papered over here...

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [PATCH] x86: c1e_idle: don't mark TSC unstable if CPU has invariant TSC

[Andreas Herrmann]

On Thu, Sep 18, 2008 at 03:35:32PM -0400, Valdis.Kletnieks@vt.edu wrote:
> On Thu, 18 Sep 2008 21:12:10 +0200, Andreas Herrmann said:
> > .. otherwise TSC is marked unstable on AMD family 0x10 and 0x11 CPUs.
> 
> OK.  I'll bite (admittedly not having looked at the actual code yet).
> 
> If the TSC is in fact invariant, what's causing the kernel to mark it as
> unstable?

Guess, you should have a look at the code.

TSC on K8 is not P-state and C-state invariant.
Thus on K8 you'll have a TSC drift if C1E is entered.

This means that if C1E is enabled TSC should be marked unstable on
those CPUs (it shouldn't be used as a clocksource).

> Sounds almost like a bug being papered over here...

Not sure what you mean.

Currently the kernel assumes TSC is stable and there are various
places where Linux might spot when TSC is unstable. c1e_idle is one
such place. But it's wrong to mark TSC unstable for all AMD CPUs in
this function as newer CPU families have TSC's that are P- and C-state
invariant.


Regards,

Andreas

Re: [PATCH] x86: c1e_idle: don't mark TSC unstable if CPU has invariant TSC

[Thomas Gleixner]

On Fri, 19 Sep 2008, Andreas Herrmann wrote:
> TSC on K8 is not P-state and C-state invariant.
> Thus on K8 you'll have a TSC drift if C1E is entered.
> 
> This means that if C1E is enabled TSC should be marked unstable on
> those CPUs (it shouldn't be used as a clocksource).

Correct. I missed that detail with the newer CPUs when I did the C1E
idle stuff. Thanks for catching it. I'll schedule it for Linus when
I'm back from Portland.

Thanks,

	tglx

Re: [PATCH] x86: c1e_idle: don't mark TSC unstable if CPU has invariant TSC

[Ingo Molnar]

* Andreas Herrmann <andreas.herrmann3@amd.com> wrote:

> Currently the kernel assumes TSC is stable and there are various 
> places where Linux might spot when TSC is unstable. c1e_idle is one 
> such place. But it's wrong to mark TSC unstable for all AMD CPUs in 
> this function as newer CPU families have TSC's that are P- and C-state 
> invariant.

i agree with the purpose of the patch (as it flags the first really sane 
TSC implementation on x86!!!) - but it would be nice to indicate this in 
a different CPU feature bit other than X86_FEATURE_CONSTANT_TSC, to 
reduce confusion. Perhaps introduce a virtual CPU feature bit for that?

	Ingo

Re: [PATCH] x86: c1e_idle: don't mark TSC unstable if CPU has invariant TSC

[Andreas Herrmann]

On Sat, Sep 20, 2008 at 08:14:57AM +0200, Ingo Molnar wrote:
> 
> * Andreas Herrmann <andreas.herrmann3@amd.com> wrote:
> 
> > Currently the kernel assumes TSC is stable and there are various 
> > places where Linux might spot when TSC is unstable. c1e_idle is one 
> > such place. But it's wrong to mark TSC unstable for all AMD CPUs in 
> > this function as newer CPU families have TSC's that are P- and C-state 
> > invariant.
> 
> i agree with the purpose of the patch (as it flags the first really sane 
> TSC implementation on x86!!!) - but it would be nice to indicate this in 
> a different CPU feature bit other than X86_FEATURE_CONSTANT_TSC, to 
> reduce confusion. Perhaps introduce a virtual CPU feature bit for that?

Yes, I thought about it as well, but at the moment I don't see a big
benefit. I guess you mean that with a new feature bit we could skip
all those additional checks for good TSCs if the bit is set or exit
mark_tsc_unstable() early?

But I've observed on one test machine with a dual-core CPU that TSC's
were P- and C-state invariant but they also had a constant difference
which was large enough to cause a
  "Measured ... cycles TSC warp between CPUs, turning off TSC clock"
message. It was a family 0x11 CPU which has invariant TSCs and
for it the X86_FEATURE_CONSTANT_TSC bit is set. But obviously both
cores' TSCs were not correctly synced among themselves at start time.

Thus I think the current behaviour of the kernel to check for good TSC
in different places is the right thing to do because it is robust enough
to detect such unexpected behaviour.


Regards,

Andreas