Re: [PATCH] x86: prefer RDTSCP in rdtsc_ordered()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.10.2024 11:45, Andrew Cooper wrote:
> On 01/10/2024 9:12 am, Jan Beulich wrote:
>> On 30.09.2024 18:40, Andrew Cooper wrote:
>>> On 30/09/2024 4:08 pm, Jan Beulich wrote:
>>>> --- a/xen/arch/x86/include/asm/msr.h
>>>> +++ b/xen/arch/x86/include/asm/msr.h
>>>> @@ -108,18 +108,30 @@ static inline uint64_t rdtsc(void)
>>>>  
>>>>  static inline uint64_t rdtsc_ordered(void)
>>>>  {
>>>> -  /*
>>>> -   * The RDTSC instruction is not ordered relative to memory access.
>>>> -   * The Intel SDM and the AMD APM are both vague on this point, but
>>>> -   * empirically an RDTSC instruction can be speculatively executed
>>>> -   * before prior loads.  An RDTSC immediately after an appropriate
>>>> -   * barrier appears to be ordered as a normal load, that is, it
>>>> -   * provides the same ordering guarantees as reading from a global
>>>> -   * memory location that some other imaginary CPU is updating
>>>> -   * continuously with a time stamp.
>>>> -   */
>>>> -  alternative("lfence", "mfence", X86_FEATURE_MFENCE_RDTSC);
>>>> -  return rdtsc();
>>>> +    uint64_t low, high, aux;
>>>> +
>>>> +    /*
>>>> +     * The RDTSC instruction is not ordered relative to memory access.
>>>> +     * The Intel SDM and the AMD APM are both vague on this point, but
>>>> +     * empirically an RDTSC instruction can be speculatively executed
>>>> +     * before prior loads.
>>> This part of the comment is stale now.  For RDTSC, AMD state:
>>>
>>> "This instruction is not serializing. Therefore, there is no guarantee
>>> that all instructions have completed at the time the time-stamp counter
>>> is read."
>>>
>>> and for RDTSCP:
>>>
>>> "Unlike the RDTSC instruction, RDTSCP forces all older instructions to
>>> retire before reading the time-stamp counter."
>>>
>>> i.e. it's dispatch serialising, given our new post-Spectre terminology.
>> I don't read that as truly "dispatch serializing";
> 
> That is precisely what dispatch serialising is and means.
> 
> Both LFENCE and RDTSCP wait at dispatch until they're the only
> instruction in the pipeline.  That is how they get the property of
> waiting for all older instructions to retire before executing.
> 
>> both Intel and AMD
>> leave open whether subsequent insns would also be affected, or whether
>> those could pass the RDTSCP.
> 
> Superscalar pipelines which can dispatch more than one uop per cycle can
> issue LFENCE/RDTSCP concurrently with younger instructions.
> 
> This is why LFENCE; JMP * was retracted as safe alternative to
> retpoline, and why the Intel docs call out explicitly that you need
> LFENCE following the RDTSC(P) if you want it to complete before
> subsequent instructions start.

Yet what you describe still only puts in place a relationship between
RDTSCP and what follows. What I was saying is that there's no guarantee
that insns following RDTSCP can't actually execute not only in parallel
with RDTSCP, but also in parallel with / ahead of earlier insns. Aiui
LFENCE makes this guarantee. IOW in

        ADD ...; LFENCE; SUB ...

the SUB is guaranteed to dispatch only after the ADD, whereas in

        ADD ...; RDTSCP; SUB ...

there doesn't appear to be such a guarantee; the only guarantee here is
for RDTSCP to dispatch after the ADD.

Jan