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

["Jan Beulich" <JBeulich@novell.com>]

>>> On 13.05.11 at 09:28, "Tian, Kevin" <kevin.tian@intel.com> wrote:
>>  From: Jan Beulich [mailto:JBeulich@novell.com] 
>> Sent: Friday, May 13, 2011 3:14 PM
>> 
>> >>> On 13.05.11 at 07:55, Keir Fraser <keir.xen@gmail.com> wrote:
>> > On 13/05/2011 03:45, "Tian, Kevin" <kevin.tian@intel.com> wrote:
>> >
>> >> x86, cpuidle: remove assertion on X86_FEATURE_TSC_RELIABLE
>> >>
>> >> 23228:1329d99b4f16 disables deep cstate to avoid restoring tsc when
>> >> tsc msr is not writtable on some old platform, which however also
>> >> adds an assertion on X86_FEATURE_TSC_RELIABLE in cstate_restore_tsc.
>> >> The two don't match as tsc writtable-ness has nothing to do with
>> >> whether it's reliable. As long as Xen can use tsc as the time source
>> >> and it's writable, it should be OK to continue using deep cstate with
>> >> tsc save/restore.
>> >
>> > 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)).
> 
> here we may need a definition about what a reliable TSC means here. no tsc 
> skew
> among cpus, or stably incremented on the bus clock? It looks that we have 
> some
> assumption behind various TSC flags, and use them with implicit assumptions.
> Here I can see that tsc_check_writability may disable deep cstate when it's 
> not
> writable, but it doesn't mean that the assertion on X86_FEATURE_TSC_RELIABLE
> is correct since even when this flag is cleared the tsc could still be 
> writable. That
> way the assertion absolutely is wrong.
> 
>> 
>> But the problem Kevin reports is exactly what I expected when we discussed
>> the whole change. 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).
>> 
> 
> I still don't understand why deep cstate must be disabled when TSC is not 
> reliable.
> Also the early bail out doesn't impact my error, since in my case 
> TSC_RELIABLE is
> not set but it's simply writable.

My point is that for the assertion to be removed, the early bail in
tsc_check_writability() must be removed too.

Jan