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

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Jan Beulich [mailto:JBeulich@xxxxxxxxxx]
> Sent: Friday, May 13, 2011 3:14 PM
> 
> >>> On 13.05.11 at 07:55, Keir Fraser <keir.xen@xxxxxxxxx> wrote:
> > On 13/05/2011 03:45, "Tian, Kevin" <kevin.tian@xxxxxxxxx> 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

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