Re: [PATCH] x86, cpuidle: remove assertion on X86_FEATURE_TSC_RELIABLE

[Keir Fraser <keir.xen@gmail.com>]

On 13/05/2011 08:14, "Jan Beulich" <JBeulich@novell.com> 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
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()).

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

As it is, I'm still of the opinion that the smallest correct fix would be to
invert the assertion predicate.

 -- Keir

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