[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
> 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
> 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
Xen-devel mailing list