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



--

[ message continues ]

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...
[ message continues ]

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

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


--

[ message continues ]

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

[ message continues ]

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

[ message continues ]

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


--

[ message continues ]