This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
Home Products Support Community News


Re: [Xen-devel] [PATCH] x86, cpuidle: remove assertion on X86_FEATURE_TS

To: "Tian, Kevin" <kevin.tian@xxxxxxxxx>, Jan Beulich <JBeulich@xxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH] x86, cpuidle: remove assertion on X86_FEATURE_TSC_RELIABLE
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
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:user-agent:date:subject:from:to:cc:message-id :thread-topic:thread-index:in-reply-to:mime-version:content-type :content-transfer-encoding; bh=pIKweDIifnnkp0pW94kRuDFe2/qkkWwhs34hTltI8ps=; b=t0+9vMz8bSu0I19R0yKTPATKL+8dnQ9kPe8H39yeH4QLqc+C51ZoZOMO0ipauReftq UzVbU3PzyaLePwgniOZcEMNfWHBa/etJMpiM8IKyy7Rs3s3yqCIuUxUDmVCvlM90FyC5 C275p3hYsjTIpjFCbTPjWKH5byGAQoYwzEkCs=
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=
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <625BA99ED14B2D499DC4E29D8138F1505C8F0A4C34@xxxxxxxxxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
Thread-index: AcwRR98VCeyRBP9eDEy6fQ1rA4OPkQAAZLGwAAE0y24=
Thread-topic: [Xen-devel] [PATCH] x86, cpuidle: remove assertion on X86_FEATURE_TSC_RELIABLE
User-agent: Microsoft-Entourage/
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