Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     "H. Peter Anvin" <hpa@zytor.com>,
        Dave Hansen <dave.hansen@intel.com>,
        Rong Tao <rtoax@foxmail.com>
Cc:     Rong Tao <rongtao@cestc.cn>, Ingo Molnar <mingo@redhat.com>,
        Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@kernel.org>,
        "open list:X86 ARCHITECTURE (32-BIT AND 64-BIT)" 
        <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] x86/vdso: Use non-serializing instruction rdtsc
In-Reply-To: <4959DDEE-58DC-45E7-BE00-019A45A97D2B@zytor.com>
References: <tencent_4DC4468312A1CB2CA34B0215FAD797D11F07@qq.com>
 <56ea846e-bce8-2508-e485-1dada8c39643@intel.com>
 <4959DDEE-58DC-45E7-BE00-019A45A97D2B@zytor.com>
Date:   Tue, 16 May 2023 22:39:49 +0200
Message-ID: <878rdo6mei.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On Tue, May 16 2023 at 10:57, H. Peter Anvin wrote:
> On May 16, 2023 7:12:34 AM PDT, Dave Hansen <dave.hansen@intel.com> wrote:
>>On 5/15/23 23:52, Rong Tao wrote:
>>> Replacing rdtscp or 'lfence;rdtsc' with the non-serializable instruction
>>> rdtsc can achieve a 40% performance improvement with only a small loss of
>>> precision.
>>
>>I think the minimum that can be done in a changelog like this is to
>>figure out _why_ a RDTSCP was in use.  There are a ton of things that
>>can make the kernel go faster, but not all of them are a good idea.
>>
>>I assume that the folks that wrote this had good reason for not using
>>plain RSTSC.  What were those reasons?
>
> I believe the motivation is that it is atomic with reading the CPU number.

Believe belongs in the realm of religion and does not help much to
explain technical issues. :)

rdtsc_ordered() has actually useful comments and also see:
  https://lore.kernel.org/lkml/87ttwc73za.ffs@tglx

The Intel SDM and the AMD APM are both blury about RDTSC speculation and
we've observed (quite some time ago) situations where the RDTSC value
was clearly from the past solely due to speculation. So we had to bite
the bullet to add the fencing. Preferrably RDTSCP or if not available
LFENCE; RDTSC. IIRC the original variant was even CPUID; RDTSC, which is
daft.

The time readout does (simplified):

    do {
           // Wait for the sequence count to become even
           while ((seq = READ_ONCE(vd->seq)) & 1);

           tsc = rdtsc_ordered();
           now = convert(vd, tsc);
    } while (seq != READ_ONCE(vd->seq));

It's obviously more complex than that, but you get the idea.

Now replace RDTSCP with RDTSC and explain what guarantees that
the TSC read isn't speculated ahead of the sequence check.

If it's architecturally guaranteed that this can't happen, I'm more than
happy to use plain RDTSC.

But as I've observed that myself in the past, I'm pretty sure that it is
not guaranteed, at least not on older microarchitectures. If newer ones
make that guarantee then they should have exposed that as a feature bit
in CPUID and clearly documented it in the SDM.

As long as that does not happen, I'm sticking to the correctness first
principle.

Thanks,

        tglx