This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
Home Products Support Community News


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

To: Keir Fraser <keir.xen@xxxxxxxxx>, Jan Beulich <JBeulich@xxxxxxxxxx>
Subject: RE: [Xen-devel] [PATCH] x86, cpuidle: remove assertion on X86_FEATURE_TSC_RELIABLE
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
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <C9F2AA77.1A3B2%keir.xen@xxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <4DCCF65802000078000412A4@xxxxxxxxxxxxxxxxxx> <C9F2AA77.1A3B2%keir.xen@xxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
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