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

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

> From: Keir Fraser [mailto:keir.xen@xxxxxxxxx]
> Sent: Friday, May 13, 2011 4:29 PM
> 
> On 13/05/2011 08:14, "Jan Beulich" <JBeulich@xxxxxxxxxx> 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

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