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

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

On 13/05/2011 09:49, "Tian, Kevin" <kevin.tian@intel.com> wrote:

>> From: Keir Fraser [mailto:keir.xen@gmail.com]
>> Sent: Friday, May 13, 2011 4:29 PM
>> 
>> 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
> 
> How about a system without NONSTOP_TSC, but with deep cstate disabled? This
> case we could still deem it as reliable.

Yes, I see TSC_RELIABLE as == NONSTOP_TSC && CONSTANT_TSC. If we have deep
sleep disabled than we have simply TSC_RELIABLE == CONSTANT_TSC.

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

Yes I think actually we can simply put ASSERT(!TSC_RELIABLE) inside
write_tsc().

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

I'll move the fixed assertion into write_tsc() in xen-unstable, and remove
entirely from the stable branches.

 -- Keir

> Thanks
> Kevin