[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

  • To: Keir Fraser <keir.xen@xxxxxxxxx>, Jan Beulich <JBeulich@xxxxxxxxxx>
  • From: "Tian, Kevin" <kevin.tian@xxxxxxxxx>
  • Date: Fri, 13 May 2011 16:49:12 +0800
  • Accept-language: en-US
  • Acceptlanguage: en-US
  • Cc: xen devel <xen-devel@xxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Fri, 13 May 2011 01:51:28 -0700
  • List-id: Xen developer discussion <xen-devel.lists.xensource.com>
  • Thread-index: AcwRR98VCeyRBP9eDEy6fQ1rA4OPkQAAZLGw
  • Thread-topic: [Xen-devel] [PATCH] x86, cpuidle: remove assertion on X86_FEATURE_TSC_RELIABLE

> 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 
implicates no any write to tsc, then we should make it consistently checked 
where. Say in cstate_restore_tsc, we can just check TSC_RELIABLE to avoid the

> > 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 

> 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 
assumption. :-)


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.