[PATCH] x86, cpuidle: remove assertion on X86_FEATURE_TSC_RELIABLE

[PATCH] x86, cpuidle: remove assertion on X86_FEATURE_TSC_RELIABLE

[Tian, Kevin]

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

x86, cpuidle: remove assertion on X86_FEATURE_TSC_RELIABLE

23228:1329d99b4f16 disables deep cstate to avoid restoring tsc when
tsc msr is not writtable on some old platform, which however also
adds an assertion on X86_FEATURE_TSC_RELIABLE in cstate_restore_tsc.
The two don't match as tsc writtable-ness has nothing to do with
whether it's reliable. As long as Xen can use tsc as the time source
and it's writable, it should be OK to continue using deep cstate
with tsc save/restore.

Also mark tsc as reliable for X86_FEATURE_CONSTANT_TSC.

Without this fix, one of our platform hits the assertion which 
only has constant tsc feature.

Signed-off-by: Kevin Tian <kevin.tian@intel.com>

diff -r 0c446850d85e xen/arch/x86/cpu/intel.c
--- a/xen/arch/x86/cpu/intel.c	Wed May 11 12:58:04 2011 +0100
+++ b/xen/arch/x86/cpu/intel.c	Fri May 13 10:01:20 2011 +0800
@@ -221,8 +221,10 @@
 	if (c->x86 == 6) 
 		set_bit(X86_FEATURE_P3, c->x86_capability);
 	if ((c->x86 == 0xf && c->x86_model >= 0x03) ||
-		(c->x86 == 0x6 && c->x86_model >= 0x0e))
+		(c->x86 == 0x6 && c->x86_model >= 0x0e)) {
 		set_bit(X86_FEATURE_CONSTANT_TSC, c->x86_capability);
+		set_bit(X86_FEATURE_TSC_RELIABLE, c->x86_capability);
+	}
 	if (cpuid_edx(0x80000007) & (1u<<8)) {
 		set_bit(X86_FEATURE_CONSTANT_TSC, c->x86_capability);
 		set_bit(X86_FEATURE_NONSTOP_TSC, c->x86_capability);
diff -r 0c446850d85e xen/arch/x86/time.c
--- a/xen/arch/x86/time.c	Wed May 11 12:58:04 2011 +0100
+++ b/xen/arch/x86/time.c	Fri May 13 10:01:20 2011 +0800
@@ -686,8 +686,6 @@
     if ( boot_cpu_has(X86_FEATURE_NONSTOP_TSC) )
         return;
 
-    ASSERT(boot_cpu_has(X86_FEATURE_TSC_RELIABLE));
-
     write_tsc(stime2tsc(read_platform_stime()));
 }


[-- Attachment #2: 20100513_cpuidle_tsc_reliable.patch --]
[-- Type: application/octet-stream, Size: 1712 bytes --]

x86, cpuidle: remove assertion on X86_FEATURE_TSC_RELIABLE

23228:1329d99b4f16 disables deep cstate to avoid restoring tsc when
tsc msr is not writtable on some old platform, which however also
adds an assertion on X86_FEATURE_TSC_RELIABLE in cstate_restore_tsc.
The two don't match as tsc writtable-ness has nothing to do with
whether it's reliable. As long as Xen can use tsc as the time source
and it's writable, it should be OK to continue using deep cstate
with tsc save/restore.

Also mark tsc as reliable for X86_FEATURE_CONSTANT_TSC.

Without this fix, one of our platform hits the assertion which 
only has constant tsc feature.

Signed-off-by: Kevin Tian <kevin.tian@intel.com>

diff -r 0c446850d85e xen/arch/x86/cpu/intel.c
--- a/xen/arch/x86/cpu/intel.c	Wed May 11 12:58:04 2011 +0100
+++ b/xen/arch/x86/cpu/intel.c	Fri May 13 10:01:20 2011 +0800
@@ -221,8 +221,10 @@
 	if (c->x86 == 6) 
 		set_bit(X86_FEATURE_P3, c->x86_capability);
 	if ((c->x86 == 0xf && c->x86_model >= 0x03) ||
-		(c->x86 == 0x6 && c->x86_model >= 0x0e))
+		(c->x86 == 0x6 && c->x86_model >= 0x0e)) {
 		set_bit(X86_FEATURE_CONSTANT_TSC, c->x86_capability);
+		set_bit(X86_FEATURE_TSC_RELIABLE, c->x86_capability);
+	}
 	if (cpuid_edx(0x80000007) & (1u<<8)) {
 		set_bit(X86_FEATURE_CONSTANT_TSC, c->x86_capability);
 		set_bit(X86_FEATURE_NONSTOP_TSC, c->x86_capability);
diff -r 0c446850d85e xen/arch/x86/time.c
--- a/xen/arch/x86/time.c	Wed May 11 12:58:04 2011 +0100
+++ b/xen/arch/x86/time.c	Fri May 13 10:01:20 2011 +0800
@@ -686,8 +686,6 @@
     if ( boot_cpu_has(X86_FEATURE_NONSTOP_TSC) )
         return;
 
-    ASSERT(boot_cpu_has(X86_FEATURE_TSC_RELIABLE));
-
     write_tsc(stime2tsc(read_platform_stime()));
 }
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] x86, cpuidle: remove assertion on X86_FEATURE_TSC_RELIABLE

[Keir Fraser]

On 13/05/2011 03:45, "Tian, Kevin" <kevin.tian@intel.com> wrote:

> x86, cpuidle: remove assertion on X86_FEATURE_TSC_RELIABLE
> 
> 23228:1329d99b4f16 disables deep cstate to avoid restoring tsc when
> tsc msr is not writtable on some old platform, which however also
> adds an assertion on X86_FEATURE_TSC_RELIABLE in cstate_restore_tsc.
> The two don't match as tsc writtable-ness has nothing to do with
> whether it's reliable. As long as Xen can use tsc as the time source
> and it's writable, it should be OK to continue using deep cstate
> with tsc save/restore.

Looks like I just got the assertion the wrong way round, should be
ASSERT(!boot_cpu_has(X86_FEATURE_TSC_RELIABLE)).

> Also mark tsc as reliable for X86_FEATURE_CONSTANT_TSC.

Unrelated change? Also, TSC_RELIABLE should only be asserted on NONSTOP_TSC
platforms. I suspect this change is not correct.

 -- Keir

> Without this fix, one of our platform hits the assertion which
> only has constant tsc feature.
> 
> Signed-off-by: Kevin Tian <kevin.tian@intel.com>
> 
> diff -r 0c446850d85e xen/arch/x86/cpu/intel.c
> --- a/xen/arch/x86/cpu/intel.c Wed May 11 12:58:04 2011 +0100
> +++ b/xen/arch/x86/cpu/intel.c Fri May 13 10:01:20 2011 +0800
> @@ -221,8 +221,10 @@
> if (c->x86 == 6) 
> set_bit(X86_FEATURE_P3, c->x86_capability);
> if ((c->x86 == 0xf && c->x86_model >= 0x03) ||
> -  (c->x86 == 0x6 && c->x86_model >= 0x0e))
> +  (c->x86 == 0x6 && c->x86_model >= 0x0e)) {
> set_bit(X86_FEATURE_CONSTANT_TSC, c->x86_capability);
> +  set_bit(X86_FEATURE_TSC_RELIABLE, c->x86_capability);
> + }
> if (cpuid_edx(0x80000007) & (1u<<8)) {
> set_bit(X86_FEATURE_CONSTANT_TSC, c->x86_capability);
> set_bit(X86_FEATURE_NONSTOP_TSC, c->x86_capability);
> diff -r 0c446850d85e xen/arch/x86/time.c
> --- a/xen/arch/x86/time.c Wed May 11 12:58:04 2011 +0100
> +++ b/xen/arch/x86/time.c Fri May 13 10:01:20 2011 +0800
> @@ -686,8 +686,6 @@
>      if ( boot_cpu_has(X86_FEATURE_NONSTOP_TSC) )
>          return;
>  
> -    ASSERT(boot_cpu_has(X86_FEATURE_TSC_RELIABLE));
> -
>      write_tsc(stime2tsc(read_platform_stime()));
>  }
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

RE: [PATCH] x86, cpuidle: remove assertion on X86_FEATURE_TSC_RELIABLE

[Tian, Kevin]

> From: Keir Fraser [mailto:keir.xen@gmail.com]
> Sent: Friday, May 13, 2011 1:55 PM
> 
> On 13/05/2011 03:45, "Tian, Kevin" <kevin.tian@intel.com> wrote:
> 
> > x86, cpuidle: remove assertion on X86_FEATURE_TSC_RELIABLE
> >
> > 23228:1329d99b4f16 disables deep cstate to avoid restoring tsc when
> > tsc msr is not writtable on some old platform, which however also adds
> > an assertion on X86_FEATURE_TSC_RELIABLE in cstate_restore_tsc.
> > The two don't match as tsc writtable-ness has nothing to do with
> > whether it's reliable. As long as Xen can use tsc as the time source
> > and it's writable, it should be OK to continue using deep cstate with
> > tsc save/restore.
> 
> Looks like I just got the assertion the wrong way round, should be
> ASSERT(!boot_cpu_has(X86_FEATURE_TSC_RELIABLE)).

why? so only an unreliable tsc which is not nonstop can enter this path?

> 
> > Also mark tsc as reliable for X86_FEATURE_CONSTANT_TSC.
> 
> Unrelated change? Also, TSC_RELIABLE should only be asserted on
> NONSTOP_TSC platforms. I suspect this change is not correct.
> 

not very related, but I think it does make sense? what's the implication
of reliable TSC in your mind?

Thanks
Kevin

Re: [PATCH] x86, cpuidle: remove assertion on X86_FEATURE_TSC_RELIABLE

[Jan Beulich]

>>> On 13.05.11 at 07:55, Keir Fraser <keir.xen@gmail.com> wrote:
> On 13/05/2011 03:45, "Tian, Kevin" <kevin.tian@intel.com> wrote:
> 
>> x86, cpuidle: remove assertion on X86_FEATURE_TSC_RELIABLE
>> 
>> 23228:1329d99b4f16 disables deep cstate to avoid restoring tsc when
>> tsc msr is not writtable on some old platform, which however also
>> adds an assertion on X86_FEATURE_TSC_RELIABLE in cstate_restore_tsc.
>> The two don't match as tsc writtable-ness has nothing to do with
>> whether it's reliable. As long as Xen can use tsc as the time source
>> and it's writable, it should be OK to continue using deep cstate
>> with tsc save/restore.
> 
> Looks like I just got the assertion the wrong way round, should be
> ASSERT(!boot_cpu_has(X86_FEATURE_TSC_RELIABLE)).

No, the assertion is correct imo (since tsc_check_writability() bails
immediately when boot_cpu_has(X86_FEATURE_TSC_RELIABLE)).

But the problem Kevin reports is exactly what I expected when
we discussed the whole change. Nevertheless, simply removing the
assertion won't be correct - instead your addition of the early
bail out on TSC_RELIABLE is what I'd now put under question (the
comment that goes with it, as we now see, isn't correct).

Jan

RE: [PATCH] x86, cpuidle: remove assertion on X86_FEATURE_TSC_RELIABLE

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@novell.com]
> Sent: Friday, May 13, 2011 3:14 PM
> 
> >>> On 13.05.11 at 07:55, Keir Fraser <keir.xen@gmail.com> wrote:
> > On 13/05/2011 03:45, "Tian, Kevin" <kevin.tian@intel.com> wrote:
> >
> >> x86, cpuidle: remove assertion on X86_FEATURE_TSC_RELIABLE
> >>
> >> 23228:1329d99b4f16 disables deep cstate to avoid restoring tsc when
> >> tsc msr is not writtable on some old platform, which however also
> >> adds an assertion on X86_FEATURE_TSC_RELIABLE in cstate_restore_tsc.
> >> The two don't match as tsc writtable-ness has nothing to do with
> >> whether it's reliable. As long as Xen can use tsc as the time source
> >> and it's writable, it should be OK to continue using deep cstate with
> >> tsc save/restore.
> >
> > Looks like I just got the assertion the wrong way round, should be
> > ASSERT(!boot_cpu_has(X86_FEATURE_TSC_RELIABLE)).
> 
> No, the assertion is correct imo (since tsc_check_writability() bails immediately
> when boot_cpu_has(X86_FEATURE_TSC_RELIABLE)).

here we may need a definition about what a reliable TSC means here. no tsc skew
among cpus, or stably incremented on the bus clock? It looks that we have some
assumption behind various TSC flags, and use them with implicit assumptions.
Here I can see that tsc_check_writability may disable deep cstate when it's not
writable, but it doesn't mean that the assertion on X86_FEATURE_TSC_RELIABLE
is correct since even when this flag is cleared the tsc could still be writable. That
way the assertion absolutely is wrong.

> 
> But the problem Kevin reports is exactly what I expected when we discussed
> the whole change. Nevertheless, simply removing the assertion won't be
> correct - instead your addition of the early bail out on TSC_RELIABLE is what I'd
> now put under question (the comment that goes with it, as we now see, isn't
> correct).
> 

I still don't understand why deep cstate must be disabled when TSC is not reliable.
Also the early bail out doesn't impact my error, since in my case TSC_RELIABLE is
not set but it's simply writable.

Thanks
Kevin

RE: [PATCH] x86, cpuidle: remove assertion on X86_FEATURE_TSC_RELIABLE

[Jan Beulich]

>>> On 13.05.11 at 09:28, "Tian, Kevin" <kevin.tian@intel.com> wrote:
>>  From: Jan Beulich [mailto:JBeulich@novell.com] 
>> Sent: Friday, May 13, 2011 3:14 PM
>> 
>> >>> On 13.05.11 at 07:55, Keir Fraser <keir.xen@gmail.com> wrote:
>> > On 13/05/2011 03:45, "Tian, Kevin" <kevin.tian@intel.com> wrote:
>> >
>> >> x86, cpuidle: remove assertion on X86_FEATURE_TSC_RELIABLE
>> >>
>> >> 23228:1329d99b4f16 disables deep cstate to avoid restoring tsc when
>> >> tsc msr is not writtable on some old platform, which however also
>> >> adds an assertion on X86_FEATURE_TSC_RELIABLE in cstate_restore_tsc.
>> >> The two don't match as tsc writtable-ness has nothing to do with
>> >> whether it's reliable. As long as Xen can use tsc as the time source
>> >> and it's writable, it should be OK to continue using deep cstate with
>> >> tsc save/restore.
>> >
>> > Looks like I just got the assertion the wrong way round, should be
>> > ASSERT(!boot_cpu_has(X86_FEATURE_TSC_RELIABLE)).
>> 
>> No, the assertion is correct imo (since tsc_check_writability() bails 
> immediately
>> when boot_cpu_has(X86_FEATURE_TSC_RELIABLE)).
> 
> here we may need a definition about what a reliable TSC means here. no tsc 
> skew
> among cpus, or stably incremented on the bus clock? It looks that we have 
> some
> assumption behind various TSC flags, and use them with implicit assumptions.
> Here I can see that tsc_check_writability may disable deep cstate when it's 
> not
> writable, but it doesn't mean that the assertion on X86_FEATURE_TSC_RELIABLE
> is correct since even when this flag is cleared the tsc could still be 
> writable. That
> way the assertion absolutely is wrong.
> 
>> 
>> But the problem Kevin reports is exactly what I expected when we discussed
>> the whole change. Nevertheless, simply removing the assertion won't be
>> correct - instead your addition of the early bail out on TSC_RELIABLE is what 
> I'd
>> now put under question (the comment that goes with it, as we now see, isn't
>> correct).
>> 
> 
> I still don't understand why deep cstate must be disabled when TSC is not 
> reliable.
> Also the early bail out doesn't impact my error, since in my case 
> TSC_RELIABLE is
> not set but it's simply writable.

My point is that for the assertion to be removed, the early bail in
tsc_check_writability() must be removed too.

Jan

Re: [PATCH] x86, cpuidle: remove assertion on X86_FEATURE_TSC_RELIABLE

[Keir Fraser]

On 13/05/2011 08:14, "Jan Beulich" <JBeulich@novell.com> wrote:

>> Looks like I just got the assertion the wrong way round, should be
>> ASSERT(!boot_cpu_has(X86_FEATURE_TSC_RELIABLE)).
> 
> No, the assertion is correct imo (since tsc_check_writability() bails
> immediately when boot_cpu_has(X86_FEATURE_TSC_RELIABLE)).

The current idea of TSC_RELIABLE is it means the platform ensures that all
TSCs are in lock step, at constant rate, never stopping even in C3. Hence we
don't need to modify TSCs, hence we don't need to check TSC writability. And
also, hence we shouldn't get to the write_tsc() in cstate_restore_tsc()
(since TSC_RELIABLE should imply NONSTOP_TSC, and hence we should bail early
from cstate_restore_tsc()).

> But the problem Kevin reports is exactly what I expected when
> we discussed the whole change.

Well I don't understand that.

Nevertheless, I feel I'm playing devil's advocate here and batting on DanM's
side for something I don't consider a major issue. If someone wants to clean
this up and come up with (possibly different and new) documented and
consistently applied semantics for these TSC feature flags, please go ahead
and propose it. And we'll see who comes out to care and bat against it.

As it is, I'm still of the opinion that the smallest correct fix would be to
invert the assertion predicate.

 -- Keir

> Nevertheless, simply removing the
> assertion won't be correct - instead your addition of the early
> bail out on TSC_RELIABLE is what I'd now put under question (the
> comment that goes with it, as we now see, isn't correct).

RE: [PATCH] x86, cpuidle: remove assertion on X86_FEATURE_TSC_RELIABLE

[Tian, Kevin]

> From: Keir Fraser [mailto:keir.xen@gmail.com]
> Sent: Friday, May 13, 2011 4:29 PM
> 
> On 13/05/2011 08:14, "Jan Beulich" <JBeulich@novell.com> wrote:
> 
> >> Looks like I just got the assertion the wrong way round, should be
> >> ASSERT(!boot_cpu_has(X86_FEATURE_TSC_RELIABLE)).
> >
> > No, the assertion is correct imo (since tsc_check_writability() bails
> > immediately when boot_cpu_has(X86_FEATURE_TSC_RELIABLE)).
> 
> The current idea of TSC_RELIABLE is it means the platform ensures that all
> TSCs are in lock step, at constant rate, never stopping even in C3. Hence we

How about a system without NONSTOP_TSC, but with deep cstate disabled? This
case we could still deem it as reliable.

> don't need to modify TSCs, hence we don't need to check TSC writability. And
> also, hence we shouldn't get to the write_tsc() in cstate_restore_tsc() (since
> TSC_RELIABLE should imply NONSTOP_TSC, and hence we should bail early
> from cstate_restore_tsc()).

Such implication simply causes confusions. If it's really the point that TSC_RELIABLE
implicates no any write to tsc, then we should make it consistently checked every
where. Say in cstate_restore_tsc, we can just check TSC_RELIABLE to avoid the
assertion.

> 
> > But the problem Kevin reports is exactly what I expected when we
> > discussed the whole change.
> 
> Well I don't understand that.
> 
> Nevertheless, I feel I'm playing devil's advocate here and batting on DanM's
> side for something I don't consider a major issue. If someone wants to clean
> this up and come up with (possibly different and new) documented and
> consistently applied semantics for these TSC feature flags, please go ahead and
> propose it. And we'll see who comes out to care and bat against it.

I'll take a further look to understand it and then may send out a cleanup patch later.

> 
> As it is, I'm still of the opinion that the smallest correct fix would be to invert
> the assertion predicate.
> 

For now, I suggest to remove the assertion before the whole logic is cleaned up.
it's not wise to break a working system by adding assertion on a to-be-discussed 
assumption. :-)

Thanks
Kevin

Re: [PATCH] x86, cpuidle: remove assertion on X86_FEATURE_TSC_RELIABLE

[Keir Fraser]

On 13/05/2011 09:49, "Tian, Kevin" <kevin.tian@intel.com> wrote:

>> From: Keir Fraser [mailto:keir.xen@gmail.com]
>> Sent: Friday, May 13, 2011 4:29 PM
>> 
>> On 13/05/2011 08:14, "Jan Beulich" <JBeulich@novell.com> wrote:
>> 
>>>> Looks like I just got the assertion the wrong way round, should be
>>>> ASSERT(!boot_cpu_has(X86_FEATURE_TSC_RELIABLE)).
>>> 
>>> No, the assertion is correct imo (since tsc_check_writability() bails
>>> immediately when boot_cpu_has(X86_FEATURE_TSC_RELIABLE)).
>> 
>> The current idea of TSC_RELIABLE is it means the platform ensures that all
>> TSCs are in lock step, at constant rate, never stopping even in C3. Hence we
> 
> How about a system without NONSTOP_TSC, but with deep cstate disabled? This
> case we could still deem it as reliable.

Yes, I see TSC_RELIABLE as == NONSTOP_TSC && CONSTANT_TSC. If we have deep
sleep disabled than we have simply TSC_RELIABLE == CONSTANT_TSC.

>> don't need to modify TSCs, hence we don't need to check TSC writability. And
>> also, hence we shouldn't get to the write_tsc() in cstate_restore_tsc()
>> (since
>> TSC_RELIABLE should imply NONSTOP_TSC, and hence we should bail early
>> from cstate_restore_tsc()).
> 
> Such implication simply causes confusions. If it's really the point that
> TSC_RELIABLE
> implicates no any write to tsc, then we should make it consistently checked
> every
> where.

Yes I think actually we can simply put ASSERT(!TSC_RELIABLE) inside
write_tsc().

> Say in cstate_restore_tsc, we can just check TSC_RELIABLE to avoid the
> assertion.
> 
>> 
>>> But the problem Kevin reports is exactly what I expected when we
>>> discussed the whole change.
>> 
>> Well I don't understand that.
>> 
>> Nevertheless, I feel I'm playing devil's advocate here and batting on DanM's
>> side for something I don't consider a major issue. If someone wants to clean
>> this up and come up with (possibly different and new) documented and
>> consistently applied semantics for these TSC feature flags, please go ahead
>> and
>> propose it. And we'll see who comes out to care and bat against it.
> 
> I'll take a further look to understand it and then may send out a cleanup
> patch later.
> 
>> 
>> As it is, I'm still of the opinion that the smallest correct fix would be to
>> invert
>> the assertion predicate.
>> 
> 
> For now, I suggest to remove the assertion before the whole logic is cleaned
> up.
> it's not wise to break a working system by adding assertion on a
> to-be-discussed 
> assumption. :-)

I'll move the fixed assertion into write_tsc() in xen-unstable, and remove
entirely from the stable branches.

 -- Keir

> Thanks
> Kevin

Re: [PATCH] x86, cpuidle: remove assertion on X86_FEATURE_TSC_RELIABLE

[Jan Beulich]

>>> On 13.05.11 at 11:15, Keir Fraser <keir.xen@gmail.com> wrote:
> I'll move the fixed assertion into write_tsc() in xen-unstable, and remove
> entirely from the stable branches.

Could you make this a one-time warning instead at least in the stable
trees, so if there are problems there would at least be a trace in the
log?

Jan

RE: [PATCH] x86, cpuidle: remove assertion on X86_FEATURE_TSC_RELIABLE

[Dan Magenheimer]

> From: Tian, Kevin [mailto:kevin.tian@intel.com]
> Subject: RE: [Xen-devel] [PATCH] x86, cpuidle: remove assertion on
> X86_FEATURE_TSC_RELIABLE
> 
> > From: Keir Fraser [mailto:keir.xen@gmail.com]
> >
> > Nevertheless, I feel I'm playing devil's advocate here and batting on
> DanM's
> > side for something I don't consider a major issue. If someone wants
> to clean
> > this up and come up with (possibly different and new) documented and
> > consistently applied semantics for these TSC feature flags, please go
> ahead and
> > propose it. And we'll see who comes out to care and bat against it.
> 
> I'll take a further look to understand it and then may send out a
> cleanup patch later.

Hi Kevin --

Welcome back to xen-devel (after a two-year hiatus?)

I'm not keeping up with xen-devel as frequently as I was in the past,
so please cc me directly if you propose different semantics.

> How about a system without NONSTOP_TSC, but with deep cstate disabled?
> This case we could still deem it as reliable.

IIRC, this is not true on a multi-socket motherboard.  Even though
each socket has NONSTOP_TSC, they are using different crystals, correct?

Thanks,
Dan

RE: [PATCH] x86, cpuidle: remove assertion on X86_FEATURE_TSC_RELIABLE

[Tian, Kevin]

> From: Dan Magenheimer [mailto:dan.magenheimer@oracle.com]
> Sent: Saturday, May 14, 2011 1:17 AM
> 
> > From: Tian, Kevin [mailto:kevin.tian@intel.com]
> > Subject: RE: [Xen-devel] [PATCH] x86, cpuidle: remove assertion on
> > X86_FEATURE_TSC_RELIABLE
> >
> > > From: Keir Fraser [mailto:keir.xen@gmail.com]
> > >
> > > Nevertheless, I feel I'm playing devil's advocate here and batting
> > > on
> > DanM's
> > > side for something I don't consider a major issue. If someone wants
> > to clean
> > > this up and come up with (possibly different and new) documented and
> > > consistently applied semantics for these TSC feature flags, please
> > > go
> > ahead and
> > > propose it. And we'll see who comes out to care and bat against it.
> >
> > I'll take a further look to understand it and then may send out a
> > cleanup patch later.
> 
> Hi Kevin --
> 
> Welcome back to xen-devel (after a two-year hiatus?)

yes, it's been a long time. :-)

> 
> I'm not keeping up with xen-devel as frequently as I was in the past, so please
> cc me directly if you propose different semantics.

no problem. I knew you guys had multiple rounds of related discussions, and I'll digest
them first.

> 
> > How about a system without NONSTOP_TSC, but with deep cstate disabled?
> > This case we could still deem it as reliable.
> 
> IIRC, this is not true on a multi-socket motherboard.  Even though each socket
> has NONSTOP_TSC, they are using different crystals, correct?
> 

it's true that sockets may use different crystals, and NONSTOP_TSC has nothing to say
synchronization among sockets/cores. So it really depends on how you define a 'reliable': 
is it reliable enough to be a Xen time source, or reliable enough to passthrough to the 
guest? I'll need to check current assumption and your previous discussions first before 
saying anything inappropriate. :-)

Thanks
Kevin

RE: [PATCH] x86, cpuidle: remove assertion on X86_FEATURE_TSC_RELIABLE

[Tian, Kevin]

> From: Keir Fraser [mailto:keir.xen@gmail.com]
> Sent: Friday, May 13, 2011 5:15 PM
> >>
> >> As it is, I'm still of the opinion that the smallest correct fix
> >> would be to invert the assertion predicate.
> >>
> >
> > For now, I suggest to remove the assertion before the whole logic is
> > cleaned up.
> > it's not wise to break a working system by adding assertion on a
> > to-be-discussed assumption. :-)
> 
> I'll move the fixed assertion into write_tsc() in xen-unstable, and remove
> entirely from the stable branches.
> 

Thanks for helping that.
Kevin

Re: [PATCH] x86, cpuidle: remove assertion on X86_FEATURE_TSC_RELIABLE

[Keir Fraser]

On 17/05/2011 01:50, "Tian, Kevin" <kevin.tian@intel.com> wrote:

>> IIRC, this is not true on a multi-socket motherboard.  Even though each
>> socket
>> has NONSTOP_TSC, they are using different crystals, correct?
>> 
> 
> it's true that sockets may use different crystals, and NONSTOP_TSC has nothing
> to say
> synchronization among sockets/cores. So it really depends on how you define a
> 'reliable': 
> is it reliable enough to be a Xen time source, or reliable enough to
> passthrough to the
> guest? I'll need to check current assumption and your previous discussions
> first before 
> saying anything inappropriate. :-)

Yes, Dan is right, RELIABLE_TSC means something more than just NONSTOP_TSC
and CONSTANT_TSC. It means that:
 1. TSCs do not stop in deep sleep (NONSTOP_TSC)
 2. TSCs do not change rate with core frequency (CONSTANT_TSC)
 3. Further, that all TSCs system wide run at the same rate at all times, in
perfect sync (not represented by any other cpu feature flag).

 -- Keir