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

[Andreas Herrmann <andreas.herrmann3@amd.com>]

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