[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: "Tian, Kevin" <kevin.tian@xxxxxxxxx>, Jan Beulich <JBeulich@xxxxxxxxxx>
  • From: Keir Fraser <keir.xen@xxxxxxxxx>
  • Date: Fri, 13 May 2011 10:15:15 +0100
  • Cc: xen devel <xen-devel@xxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Fri, 13 May 2011 02:16:01 -0700
  • Domainkey-signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=user-agent:date:subject:from:to:cc:message-id:thread-topic :thread-index:in-reply-to:mime-version:content-type :content-transfer-encoding; b=CQEYLbyzfNPfv8HU7Vo0mwr/XzdSuWtKwhNcao8o9qQK8ZPpb0qh5zH1DlFC6Wmii0 7CVGvuei9aZGuetqReB9fD/TaYLsFYUbSlw8+x88CKTjUMdGgxOlHukrSho6tb4VNh00 /CPEwZpW2VXTqrATXYBX6f/5Oew/Fqvsw20Qk=
  • List-id: Xen developer discussion <xen-devel.lists.xensource.com>
  • Thread-index: AcwRR98VCeyRBP9eDEy6fQ1rA4OPkQAAZLGwAAE0y24=
  • Thread-topic: [Xen-devel] [PATCH] x86, cpuidle: remove assertion on X86_FEATURE_TSC_RELIABLE

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

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

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

> 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

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.