[PATCH] x86: Support always running TSC on Intel CPUs

[PATCH] x86: Support always running TSC on Intel CPUs

[Venki Pallipadi]

Add support for CPUID_0x80000007_Bit8 on Intel CPUs as well. This bit means
that the TSC is invariant with C/P/T states and always runs at constant
frequency.

With Intel CPUs, we have 3 classes
* CPUs where TSC runs at constant rate and does not stop n C-states
* CPUs where TSC runs at constant rate, but will stop in deep C-states
* CPUs where TSC rate will vary based on P/T-states and TSC will stop in deep
  C-states.

To cover these 3, one feature bit (CONSTANT_TSC) is not enough. So, add a
second bit (NOSTOP_TSC). CONSTANT_TSC indicates that the TSC runs at
constant frequency irrespective of P/T-states, and NOSTOP_TSC indicates
that TSC does not stop in deep C-states.

CPUID_0x8000000_Bit8 indicates both these feature bit can be set.
We still have CONSTANT_TSC _set_ and NOSTOP_TSC _not_set_ on some older Intel
CPUs, based on model checks. We can use TSC on such CPUs for time, as long as
those CPUs do not support/enter deep C-states.

Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>

---
 arch/x86/include/asm/cpufeature.h |    1 +
 arch/x86/kernel/cpu/amd.c         |    9 +++++++--
 arch/x86/kernel/cpu/intel.c       |   10 ++++++++++
 arch/x86/kernel/process.c         |    2 +-
 drivers/acpi/processor_idle.c     |    6 +++---
 5 files changed, 22 insertions(+), 6 deletions(-)

Index: tip/arch/x86/kernel/cpu/amd.c
===================================================================
--- tip.orig/arch/x86/kernel/cpu/amd.c	2008-11-17 12:48:08.000000000 -0800
+++ tip/arch/x86/kernel/cpu/amd.c	2008-11-17 15:49:39.000000000 -0800
@@ -283,9 +283,14 @@ static void __cpuinit early_init_amd(str
 {
 	early_init_amd_mc(c);
 
-	/* c->x86_power is 8000_0007 edx. Bit 8 is constant TSC */
-	if (c->x86_power & (1<<8))
+	/*
+	 * c->x86_power is 8000_0007 edx. Bit 8 is TSC runs at constant rate
+	 * with P/T states and does not stop in deep C-states
+	 */
+	if (c->x86_power & (1 << 8)) {
 		set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
+		set_cpu_cap(c, X86_FEATURE_NOSTOP_TSC);
+	}
 
 #ifdef CONFIG_X86_64
 	set_cpu_cap(c, X86_FEATURE_SYSCALL32);
Index: tip/arch/x86/kernel/cpu/intel.c
===================================================================
--- tip.orig/arch/x86/kernel/cpu/intel.c	2008-11-17 12:48:08.000000000 -0800
+++ tip/arch/x86/kernel/cpu/intel.c	2008-11-17 15:49:39.000000000 -0800
@@ -41,6 +41,16 @@ static void __cpuinit early_init_intel(s
 	if (c->x86 == 15 && c->x86_cache_alignment == 64)
 		c->x86_cache_alignment = 128;
 #endif
+
+	/*
+	 * c->x86_power is 8000_0007 edx. Bit 8 is TSC runs at constant rate
+	 * with P/T states and does not stop in deep C-states
+	 */
+	if (c->x86_power & (1 << 8)) {
+		set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
+		set_cpu_cap(c, X86_FEATURE_NOSTOP_TSC);
+	}
+
 }
 
 #ifdef CONFIG_X86_32
Index: tip/drivers/acpi/processor_idle.c
===================================================================
--- tip.orig/drivers/acpi/processor_idle.c	2008-11-17 12:48:11.000000000 -0800
+++ tip/drivers/acpi/processor_idle.c	2008-11-17 15:49:39.000000000 -0800
@@ -374,15 +374,15 @@ static int tsc_halts_in_c(int state)
 {
 	switch (boot_cpu_data.x86_vendor) {
 	case X86_VENDOR_AMD:
+	case X86_VENDOR_INTEL:
 		/*
 		 * AMD Fam10h TSC will tick in all
 		 * C/P/S0/S1 states when this bit is set.
 		 */
-		if (boot_cpu_has(X86_FEATURE_CONSTANT_TSC))
+		if (boot_cpu_has(X86_FEATURE_NOSTOP_TSC))
 			return 0;
+
 		/*FALL THROUGH*/
-	case X86_VENDOR_INTEL:
-		/* Several cases known where TSC halts in C2 too */
 	default:
 		return state > ACPI_STATE_C1;
 	}
Index: tip/arch/x86/include/asm/cpufeature.h
===================================================================
--- tip.orig/arch/x86/include/asm/cpufeature.h	2008-11-17 12:48:08.000000000 -0800
+++ tip/arch/x86/include/asm/cpufeature.h	2008-11-17 15:50:27.000000000 -0800
@@ -93,6 +93,7 @@
 #define X86_FEATURE_AMDC1E	(3*32+21) /* AMD C1E detected */
 #define X86_FEATURE_XTOPOLOGY	(3*32+22) /* cpu topology enum extensions */
 #define X86_FEATURE_TSC_RELIABLE (3*32+23) /* TSC is known to be reliable */
+#define X86_FEATURE_NOSTOP_TSC	(3*32+24) /* TSC does not stop in C states */
 
 /* Intel-defined CPU features, CPUID level 0x00000001 (ecx), word 4 */
 #define X86_FEATURE_XMM3	(4*32+ 0) /* "pni" SSE-3 */
Index: tip/arch/x86/kernel/process.c
===================================================================
--- tip.orig/arch/x86/kernel/process.c	2008-11-17 12:48:08.000000000 -0800
+++ tip/arch/x86/kernel/process.c	2008-11-17 15:49:39.000000000 -0800
@@ -286,7 +286,7 @@ static void c1e_idle(void)
 		rdmsr(MSR_K8_INT_PENDING_MSG, lo, hi);
 		if (lo & K8_INTP_C1E_ACTIVE_MASK) {
 			c1e_detected = 1;
-			if (!boot_cpu_has(X86_FEATURE_CONSTANT_TSC))
+			if (!boot_cpu_has(X86_FEATURE_NOSTOP_TSC))
 				mark_tsc_unstable("TSC halt in AMD C1E");
 			printk(KERN_INFO "System has AMD C1E enabled\n");
 			set_cpu_cap(&boot_cpu_data, X86_FEATURE_AMDC1E);

Re: [PATCH] x86: Support always running TSC on Intel CPUs

[H. Peter Anvin]

Venki Pallipadi wrote:
> Add support for CPUID_0x80000007_Bit8 on Intel CPUs as well. This bit means
> that the TSC is invariant with C/P/T states and always runs at constant
> frequency.
> 
> With Intel CPUs, we have 3 classes
> * CPUs where TSC runs at constant rate and does not stop n C-states
> * CPUs where TSC runs at constant rate, but will stop in deep C-states
> * CPUs where TSC rate will vary based on P/T-states and TSC will stop in deep
>   C-states.
> 
> To cover these 3, one feature bit (CONSTANT_TSC) is not enough. So, add a
> second bit (NOSTOP_TSC). CONSTANT_TSC indicates that the TSC runs at
> constant frequency irrespective of P/T-states, and NOSTOP_TSC indicates
> that TSC does not stop in deep C-states.
> 

What is the definition of a "deep" C-state?  C2?  C3?  C4?

	-hpa

RE: [PATCH] x86: Support always running TSC on Intel CPUs

[Pallipadi, Venkatesh]

 

>-----Original Message-----
>From: H. Peter Anvin [mailto:hpa@zytor.com] 
>Sent: Monday, November 17, 2008 4:14 PM
>To: Pallipadi, Venkatesh
>Cc: Ingo Molnar; Thomas Gleixner; linux-kernel
>Subject: Re: [PATCH] x86: Support always running TSC on Intel CPUs
>
>Venki Pallipadi wrote:
>> Add support for CPUID_0x80000007_Bit8 on Intel CPUs as well. 
>This bit means
>> that the TSC is invariant with C/P/T states and always runs 
>at constant
>> frequency.
>> 
>> With Intel CPUs, we have 3 classes
>> * CPUs where TSC runs at constant rate and does not stop n C-states
>> * CPUs where TSC runs at constant rate, but will stop in 
>deep C-states
>> * CPUs where TSC rate will vary based on P/T-states and TSC 
>will stop in deep
>>   C-states.
>> 
>> To cover these 3, one feature bit (CONSTANT_TSC) is not 
>enough. So, add a
>> second bit (NOSTOP_TSC). CONSTANT_TSC indicates that the TSC runs at
>> constant frequency irrespective of P/T-states, and 
>NOSTOP_TSC indicates
>> that TSC does not stop in deep C-states.
>> 
>
>What is the definition of a "deep" C-state?  C2?  C3?  C4?
>
>	-hpa

All C-states higher than C1. 

Thanks,
Venki

Re: [PATCH] x86: Support always running TSC on Intel CPUs

[H. Peter Anvin]

Pallipadi, Venkatesh wrote:
> 
> All C-states higher than C1. 
> 

Including C2?  If so, the TSC is unusable since C2 can be invoked 
asynchronously by the chipset.

	-hpa

Re: [PATCH] x86: Support always running TSC on Intel CPUs

[Venki Pallipadi]

On Mon, Nov 17, 2008 at 04:18:16PM -0800, H. Peter Anvin wrote:
> Pallipadi, Venkatesh wrote:
> > 
> > All C-states higher than C1. 
> > 
> 
> Including C2?  If so, the TSC is unusable since C2 can be invoked 
> asynchronously by the chipset.
> 

No. Not on Intel CPUs atleast. On Intel CPUs, we enter C-states only on
hlt or mwait. C1 is always C1 or C1E, where TSC always runs.
C2, C3, ... implementation vary depending on processor and TSC may or may
not run. This 0x80000007 feature bit basically says TSC is going to run
during any C-state.

Thanks,
Venki

Re: [PATCH] x86: Support always running TSC on Intel CPUs

[H. Peter Anvin]

Venki Pallipadi wrote:
> On Mon, Nov 17, 2008 at 04:18:16PM -0800, H. Peter Anvin wrote:
>> Pallipadi, Venkatesh wrote:
>>> All C-states higher than C1. 
>>>
>> Including C2?  If so, the TSC is unusable since C2 can be invoked 
>> asynchronously by the chipset.
>>
> 
> No. Not on Intel CPUs atleast. On Intel CPUs, we enter C-states only on
> hlt or mwait.  C1 is always C1 or C1E, where TSC always runs.
> C2, C3, ... implementation vary depending on processor and TSC may or may
> not run. This 0x80000007 feature bit basically says TSC is going to run
> during any C-state.
> 

I was under the impression that C2 was invoked by the chipset on a 
thermal condition, at least on older (P3-era) processors.  Is that no 
longer true?  If what you say is above (HLT and MWAIT only), then *was* 
it ever true?

	-hpa

Re: [PATCH] x86: Support always running TSC on Intel CPUs

[Venki Pallipadi]

On Mon, Nov 17, 2008 at 04:52:21PM -0800, H. Peter Anvin wrote:
> Venki Pallipadi wrote:
> > On Mon, Nov 17, 2008 at 04:18:16PM -0800, H. Peter Anvin wrote:
> >> Pallipadi, Venkatesh wrote:
> >>> All C-states higher than C1.
> >>>
> >> Including C2?  If so, the TSC is unusable since C2 can be invoked
> >> asynchronously by the chipset.
> >>
> >
> > No. Not on Intel CPUs atleast. On Intel CPUs, we enter C-states only on
> > hlt or mwait.  C1 is always C1 or C1E, where TSC always runs.
> > C2, C3, ... implementation vary depending on processor and TSC may or may
> > not run. This 0x80000007 feature bit basically says TSC is going to run
> > during any C-state.
> >
> 
> I was under the impression that C2 was invoked by the chipset on a
> thermal condition, at least on older (P3-era) processors.  Is that no
> longer true?  If what you say is above (HLT and MWAIT only), then *was*
> it ever true?

I should also add io port based C-state to HLT and MWAIT. But, that again is
OS initiated.

I don't know of C2 invocation in thermal condition. Thermal condition, all
CPUs that I know of (P3 and beyond), use either clock modulation or frequency
changes. And on some such CPUs, where TSC runs at constant freq during such
modulation/freq change, we set CONSTANT_TSC bit based on model number check.
So, on CPUs earlier than those, we cannot use TSC or we have to scale TSC
based on freq. This patch shouldn't have any impact for those CPUs.

Thanks,
Venki

Re: [PATCH] x86: Support always running TSC on Intel CPUs

[H. Peter Anvin]

Venki Pallipadi wrote:
>>>
>> I was under the impression that C2 was invoked by the chipset on a
>> thermal condition, at least on older (P3-era) processors.  Is that no
>> longer true?  If what you say is above (HLT and MWAIT only), then *was*
>> it ever true?
> 
> I should also add io port based C-state to HLT and MWAIT. But, that again is
> OS initiated.
> 
> I don't know of C2 invocation in thermal condition. Thermal condition, all
> CPUs that I know of (P3 and beyond), use either clock modulation or frequency
> changes. And on some such CPUs, where TSC runs at constant freq during such
> modulation/freq change, we set CONSTANT_TSC bit based on model number check.
> So, on CPUs earlier than those, we cannot use TSC or we have to scale TSC
> based on freq. This patch shouldn't have any impact for those CPUs.
> 

I believe there are CPUs -- again, in the P3-era range at least -- which 
invoke C2 from the chipset on thermal conditions (and basically PWM the 
CPU.)  I'd like to get that clarified so we don't trip up on that.

	-hpa

Re: [PATCH] x86: Support always running TSC on Intel CPUs

[Arjan van de Ven]

On Mon, 17 Nov 2008 17:07:26 -0800
"H. Peter Anvin" <hpa@zytor.com> wrote:

> Venki Pallipadi wrote:
> >>>
> >> I was under the impression that C2 was invoked by the chipset on a
> >> thermal condition, at least on older (P3-era) processors.  Is that
> >> no longer true?  If what you say is above (HLT and MWAIT only),
> >> then *was* it ever true?
> > 
> > I should also add io port based C-state to HLT and MWAIT. But, that
> > again is OS initiated.
> > 
> > I don't know of C2 invocation in thermal condition. Thermal
> > condition, all CPUs that I know of (P3 and beyond), use either
> > clock modulation or frequency changes. And on some such CPUs, where
> > TSC runs at constant freq during such modulation/freq change, we
> > set CONSTANT_TSC bit based on model number check. So, on CPUs
> > earlier than those, we cannot use TSC or we have to scale TSC based
> > on freq. This patch shouldn't have any impact for those CPUs.
> > 
> 
> I believe there are CPUs -- again, in the P3-era range at least --
> which invoke C2 from the chipset on thermal conditions (and basically
> PWM the CPU.)  I'd like to get that clarified so we don't trip up on
> that.

p3-era cpus didn't stop tsc in c2 afaik.


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH] x86: Support always running TSC on Intel CPUs

[H. Peter Anvin]

Arjan van de Ven wrote:
>>>
>> I believe there are CPUs -- again, in the P3-era range at least --
>> which invoke C2 from the chipset on thermal conditions (and basically
>> PWM the CPU.)  I'd like to get that clarified so we don't trip up on
>> that.
> 
> p3-era cpus didn't stop tsc in c2 afaik.
> 

OK, that's an important distinction too, then.

	-hpa

Re: [PATCH] x86: Support always running TSC on Intel CPUs

[Ingo Molnar]

* Venki Pallipadi <venkatesh.pallipadi@intel.com> wrote:

> +	if (c->x86_power & (1 << 8)) {
>  		set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
> +		set_cpu_cap(c, X86_FEATURE_NOSTOP_TSC);
> +	}

hm, the naming is a bit confusing. We now have 3 variants:

  X86_FEATURE_TSC
  X86_FEATURE_CONSTANT_TSC
  X86_FEATURE_NOSTOP_TSC

NOSTOP_TSC is basically what CONSTANT_TSC should have been to begin 
with ;-)

i'd suggest to rename it to this:

  X86_FEATURE_TSC
  X86_FEATURE_CONSTANT_FREQ_TSC
  X86_FEATURE_STABLE_TSC

... with CONSTANT_FREQ_TSC not having any real role in the long run. 
(it's similarly problematic to a completely unstable TSC)

does this sound ok?

	Ingo

Re: [PATCH] x86: Support always running TSC on Intel CPUs

[Joe Korty]

On Tue, Nov 18, 2008 at 09:09:52AM +0100, Ingo Molnar wrote:
> 
> * Venki Pallipadi <venkatesh.pallipadi@intel.com> wrote:
> 
> > +	if (c->x86_power & (1 << 8)) {
> >  		set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
> > +		set_cpu_cap(c, X86_FEATURE_NOSTOP_TSC);
> > +	}
> 
> hm, the naming is a bit confusing. We now have 3 variants:
> 
>   X86_FEATURE_TSC
>   X86_FEATURE_CONSTANT_TSC
>   X86_FEATURE_NOSTOP_TSC
> 
> NOSTOP_TSC is basically what CONSTANT_TSC should have been to begin 
> with ;-)
> 
> i'd suggest to rename it to this:
> 
>   X86_FEATURE_TSC
>   X86_FEATURE_CONSTANT_FREQ_TSC
>   X86_FEATURE_STABLE_TSC
> 
> ... with CONSTANT_FREQ_TSC not having any real role in the long run. 
> (it's similarly problematic to a completely unstable TSC)
> 
> does this sound ok?


To me, the new naming has the same head-scratching potential
as the old....

How about:

	X86_FEATURE_TSC
	X86_FEATURE_STABLE_TSC_OBSOLETE
	X86_FEATURE_STABLE_TSC

Joe

Re: [PATCH] x86: Support always running TSC on Intel CPUs

[Ingo Molnar]

* Joe Korty <joe.korty@ccur.com> wrote:

> On Tue, Nov 18, 2008 at 09:09:52AM +0100, Ingo Molnar wrote:
> > 
> > * Venki Pallipadi <venkatesh.pallipadi@intel.com> wrote:
> > 
> > > +	if (c->x86_power & (1 << 8)) {
> > >  		set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
> > > +		set_cpu_cap(c, X86_FEATURE_NOSTOP_TSC);
> > > +	}
> > 
> > hm, the naming is a bit confusing. We now have 3 variants:
> > 
> >   X86_FEATURE_TSC
> >   X86_FEATURE_CONSTANT_TSC
> >   X86_FEATURE_NOSTOP_TSC
> > 
> > NOSTOP_TSC is basically what CONSTANT_TSC should have been to begin 
> > with ;-)
> > 
> > i'd suggest to rename it to this:
> > 
> >   X86_FEATURE_TSC
> >   X86_FEATURE_CONSTANT_FREQ_TSC
> >   X86_FEATURE_STABLE_TSC
> > 
> > ... with CONSTANT_FREQ_TSC not having any real role in the long run. 
> > (it's similarly problematic to a completely unstable TSC)
> > 
> > does this sound ok?
> 
> 
> To me, the new naming has the same head-scratching potential
> as the old....
> 
> How about:
> 
> 	X86_FEATURE_TSC
> 	X86_FEATURE_STABLE_TSC_OBSOLETE
> 	X86_FEATURE_STABLE_TSC

the _honest_ naming would be:

 	X86_FEATURE_TSC
 	X86_FEATURE_STABLE_TSC_BUT_NOT_ALWAYS
 	X86_FEATURE_STABLE_TSC

;-) 

what's head-scratching about X86_FEATURE_CONSTANT_FREQ_TSC? It's a 
limited TSC variant: it follows a reference frequency that does not 
change with cpufreq changes, but it can stop at a whim in C states. So 
it's not "stable" nor really "constant" in the everyday sense.

What is 'constant' about it is its reference frequency - hence 
X86_FEATURE_CONSTANT_FREQ_TSC.

	Ingo

RE: [PATCH] x86: Support always running TSC on Intel CPUs

[Pallipadi, Venkatesh]

>-----Original Message-----
>From: Ingo Molnar [mailto:mingo@elte.hu]
>Sent: Tuesday, November 18, 2008 8:06 AM
>To: Joe Korty
>Cc: Pallipadi, Venkatesh; H Peter Anvin; Thomas Gleixner; linux-kernel
>Subject: Re: [PATCH] x86: Support always running TSC on Intel CPUs
>
>
>* Joe Korty <joe.korty@ccur.com> wrote:
>
>> On Tue, Nov 18, 2008 at 09:09:52AM +0100, Ingo Molnar wrote:
>> >
>> > * Venki Pallipadi <venkatesh.pallipadi@intel.com> wrote:
>> >
>> > > +        if (c->x86_power & (1 << 8)) {
>> > >                  set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
>> > > +                set_cpu_cap(c, X86_FEATURE_NOSTOP_TSC);
>> > > +        }
>> >
>> > hm, the naming is a bit confusing. We now have 3 variants:
>> >
>> >   X86_FEATURE_TSC
>> >   X86_FEATURE_CONSTANT_TSC
>> >   X86_FEATURE_NOSTOP_TSC
>> >
>> > NOSTOP_TSC is basically what CONSTANT_TSC should have been
>to begin
>> > with ;-)
>> >
>> > i'd suggest to rename it to this:
>> >
>> >   X86_FEATURE_TSC
>> >   X86_FEATURE_CONSTANT_FREQ_TSC
>> >   X86_FEATURE_STABLE_TSC
>> >
>> > ... with CONSTANT_FREQ_TSC not having any real role in the
>long run.
>> > (it's similarly problematic to a completely unstable TSC)
>> >
>> > does this sound ok?
>>
>>
>> To me, the new naming has the same head-scratching potential
>> as the old....
>>
>> How about:
>>
>>      X86_FEATURE_TSC
>>      X86_FEATURE_STABLE_TSC_OBSOLETE
>>      X86_FEATURE_STABLE_TSC
>
>the _honest_ naming would be:
>
>       X86_FEATURE_TSC
>       X86_FEATURE_STABLE_TSC_BUT_NOT_ALWAYS
>       X86_FEATURE_STABLE_TSC
>
>;-)
>
>what's head-scratching about X86_FEATURE_CONSTANT_FREQ_TSC? It's a
>limited TSC variant: it follows a reference frequency that does not
>change with cpufreq changes, but it can stop at a whim in C states. So
>it's not "stable" nor really "constant" in the everyday sense.
>

I don't like STABLE_TSC_OBSOLETE as it does not say anything
descriptive about what it means and people have to look at the code
to find out.

My original intention was to split this into two features
- P/T state invariant TSC which counts at constant rate
- C-state invariant TSC that never stops
Some CPUs will have only first feature. Others may have both.

But, I agree it is confusing.

How about?
X86_FEATURE_FIXEDFREQ_MAYSTOP_TSC
X86_FEATURE_ALWAYS_RUNNING_TSC

Thanks,
Venki

Re: [PATCH] x86: Support always running TSC on Intel CPUs

[Joe Korty]

On Tue, Nov 18, 2008 at 11:05:42AM -0500, Ingo Molnar wrote:
> 
> * Joe Korty <joe.korty@ccur.com> wrote:
> 
> > On Tue, Nov 18, 2008 at 09:09:52AM +0100, Ingo Molnar wrote:
> > >
> > > * Venki Pallipadi <venkatesh.pallipadi@intel.com> wrote:
> > >
> > > > + if (c->x86_power & (1 << 8)) {
> > > >           set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
> > > > +         set_cpu_cap(c, X86_FEATURE_NOSTOP_TSC);
> > > > + }
> > >
> > > hm, the naming is a bit confusing. We now have 3 variants:
> > >
> > >   X86_FEATURE_TSC
> > >   X86_FEATURE_CONSTANT_TSC
> > >   X86_FEATURE_NOSTOP_TSC
> > >
> > > NOSTOP_TSC is basically what CONSTANT_TSC should have been to begin
> > > with ;-)
> > >
> > > i'd suggest to rename it to this:
> > >
> > >   X86_FEATURE_TSC
> > >   X86_FEATURE_CONSTANT_FREQ_TSC
> > >   X86_FEATURE_STABLE_TSC
> > >
> > > ... with CONSTANT_FREQ_TSC not having any real role in the long run.
> > > (it's similarly problematic to a completely unstable TSC)
> > >
> > > does this sound ok?
> >
> >
> > To me, the new naming has the same head-scratching potential
> > as the old....
> >
> > How about:
> >
> >       X86_FEATURE_TSC
> >       X86_FEATURE_STABLE_TSC_OBSOLETE
> >       X86_FEATURE_STABLE_TSC
> 
> the _honest_ naming would be:
> 
>         X86_FEATURE_TSC
>         X86_FEATURE_STABLE_TSC_BUT_NOT_ALWAYS
>         X86_FEATURE_STABLE_TSC
> 
> ;-)
> 
> what's head-scratching about X86_FEATURE_CONSTANT_FREQ_TSC? It's a
> limited TSC variant: it follows a reference frequency that does not
> change with cpufreq changes, but it can stop at a whim in C states. So
> it's not "stable" nor really "constant" in the everyday sense.
> 
> What is 'constant' about it is its reference frequency - hence
> X86_FEATURE_CONSTANT_FREQ_TSC.
> 
>         Ingo


A name like X86_FEATURE_CONSTANT_FREQ_TSC implies that
the result (the TSC) is constant frequency, not the input.

Re: [PATCH] x86: Support always running TSC on Intel CPUs

[Joe Korty]

On Tue, Nov 18, 2008 at 11:47:35AM -0500, Pallipadi, Venkatesh wrote:
> 
>>-----Original Message-----
>>From: Ingo Molnar [mailto:mingo@elte.hu]
>>Sent: Tuesday, November 18, 2008 8:06 AM
>>To: Joe Korty
>>Cc: Pallipadi, Venkatesh; H Peter Anvin; Thomas Gleixner; linux-kernel
>>Subject: Re: [PATCH] x86: Support always running TSC on Intel CPUs
>>
>>* Joe Korty <joe.korty@ccur.com> wrote:
>>> On Tue, Nov 18, 2008 at 09:09:52AM +0100, Ingo Molnar wrote:
>>>> * Venki Pallipadi <venkatesh.pallipadi@intel.com> wrote:
>>>>
>>>> > +        if (c->x86_power & (1 << 8)) {
>>>> >                  set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
>>>> > +                set_cpu_cap(c, X86_FEATURE_NOSTOP_TSC);
>>>> > +        }
>>>>
>>>> hm, the naming is a bit confusing. We now have 3 variants:
>>>>
>>>>   X86_FEATURE_TSC
>>>>   X86_FEATURE_CONSTANT_TSC
>>>>   X86_FEATURE_NOSTOP_TSC
>>>>
>>>> NOSTOP_TSC is basically what CONSTANT_TSC should have been
>>to begin
>>>> with ;-)
>>>>
>>>> i'd suggest to rename it to this:
>>>>
>>>>   X86_FEATURE_TSC
>>>>   X86_FEATURE_CONSTANT_FREQ_TSC
>>>>   X86_FEATURE_STABLE_TSC
>>>>
>>>> ... with CONSTANT_FREQ_TSC not having any real role in the long run.
>>>> (it's similarly problematic to a completely unstable TSC)
>>>>
>>>> does this sound ok?
>>>
>>>
>>> To me, the new naming has the same head-scratching potential
>>> as the old....
>>>
>>> How about:
>>>
>>>      X86_FEATURE_TSC
>>>      X86_FEATURE_STABLE_TSC_OBSOLETE
>>>      X86_FEATURE_STABLE_TSC
>>
>>the _honest_ naming would be:
>>
>>       X86_FEATURE_TSC
>>       X86_FEATURE_STABLE_TSC_BUT_NOT_ALWAYS
>>       X86_FEATURE_STABLE_TSC
>>
>>;-)
>>
>>what's head-scratching about X86_FEATURE_CONSTANT_FREQ_TSC? It's a
>>limited TSC variant: it follows a reference frequency that does not
>>change with cpufreq changes, but it can stop at a whim in C states. So
>>it's not "stable" nor really "constant" in the everyday sense.
>>
> 
> I don't like STABLE_TSC_OBSOLETE as it does not say anything
> descriptive about what it means and people have to look at the code
> to find out.
> 
> My original intention was to split this into two features
> - P/T state invariant TSC which counts at constant rate
> - C-state invariant TSC that never stops
> Some CPUs will have only first feature. Others may have both.
> 
> But, I agree it is confusing.
> 
> How about?
> X86_FEATURE_FIXEDFREQ_MAYSTOP_TSC
> X86_FEATURE_ALWAYS_RUNNING_TSC
> 
> Thanks,
> Venki


Much nicer...
Regards,
Joe

Re: [PATCH] x86: Support always running TSC on Intel CPUs

[H. Peter Anvin]

Pallipadi, Venkatesh wrote:
> 
> But, I agree it is confusing.
> 
> How about?
> X86_FEATURE_FIXEDFREQ_MAYSTOP_TSC
> X86_FEATURE_ALWAYS_RUNNING_TSC
> 

A bit verbose, don't you think?

Either way, there seems to be, functionally, C2_TSC and C3PLUS_TSC for 
different CPUs...

	-hpa