svn commit: r238755 - head/sys/x86/x86

[Andriy Gapon]

on 25/07/2012 13:21 Konstantin Belousov said the following:
> On Wed, Jul 25, 2012 at 10:20:02AM +0300, Andriy Gapon wrote:
>> on 25/07/2012 01:10 Jim Harris said the following:
>>> Author: jimharris
>>> Date: Tue Jul 24 22:10:11 2012
>>> New Revision: 238755
>>> URL: http://svn.freebsd.org/changeset/base/238755
>>>
>>> Log:
>>>   Add rmb() to tsc_read_##x to enforce serialization of rdtsc captures.
>>>   
>>>   Intel Architecture Manual specifies that rdtsc instruction is not serialized,
>>>   so without this change, TSC synchronization test would periodically fail,
>>>   resulting in use of HPET timecounter instead of TSC-low.  This caused
>>>   severe performance degradation (40-50%) when running high IO/s workloads due to
>>>   HPET MMIO reads and GEOM stat collection.
>>>   
>>>   Tests on Xeon E5-2600 (Sandy Bridge) 8C systems were seeing TSC synchronization
>>>   fail approximately 20% of the time.
>>
>> Should rather the synchronization test be fixed if it's the culprit?
> Synchronization test for what ?

The synchronization test mentioned above.
So, oops, very sorry - I missed the fact that the change was precisely in the
test.  I confused it for another place where tsc is used.  Thank you for pointing
this out.

>> Or is this change universally good for the real uses of TSC?
> 
> What I understood from the Intel SDM, and also from additional experiments
> which Jim kindly made despite me being annoying as usual, is that 'read
> memory barrier' AKA LFENCE there is used for its secondary implementation
> effects, not for load/load barrier as you might assume.
> 
> According to SDM, LFENCE fully drains execution pipeline (but comparing
> with MFENCE, does not drain write buffers). The result is that RDTSC is
> not started before previous instructions are finished.

Yes, I am fully aware of this.

> For tsc test, this means that after the change RDTSC executions are not
> reordered on the single core among themself. As I understand, CPU has
> no dependency noted between two reads of tsc by RDTSC, which allows
> later read to give lower value of counter. This is fixed by Intel by
> introduction of RDTSCP instruction, which is defined to be serialization
> point, and use of which (instead of LFENCE; RDTSC sequence) also fixes
> test, as confirmed by Jim.

Yes.  I think that previously Intel recommended to precede rdtsc with cpuid for
all the same reasons.  Not sure if there is any difference performance-wise
comparing to lfence.
Unfortunately, rdtscp is not available on all CPUs, so using it would require
extra work.

> In fact, I now think that we should also apply the following patch.
> Otherwise, consequtive calls to e.g. binuptime(9) could return decreased
> time stamps. Note that libc __vdso_gettc.c already has LFENCE nearby the
> tsc reads, which was done not for this reason, but apparently needed for
> the reason too.
> 
> diff --git a/sys/x86/x86/tsc.c b/sys/x86/x86/tsc.c
> index 085c339..229b351 100644
> --- a/sys/x86/x86/tsc.c
> +++ b/sys/x86/x86/tsc.c
> @@ -594,6 +594,7 @@ static u_int
>  tsc_get_timecount(struct timecounter *tc __unused)
>  {
>  
> +	rmb();
>  	return (rdtsc32());
>  }
>  

This makes sense to me.  We probably want correctness over performance here.
[BTW, I originally thought that the change was here; brain malfunction]

> @@ -602,8 +603,9 @@ tsc_get_timecount_low(struct timecounter *tc)
>  {
>  	uint32_t rv;
>  
> +	rmb();
>  	__asm __volatile("rdtsc; shrd %%cl, %%edx, %0"
> -	: "=a" (rv) : "c" ((int)(intptr_t)tc->tc_priv) : "edx");
> +	    : "=a" (rv) : "c" ((int)(intptr_t)tc->tc_priv) : "edx");
>  	return (rv);
>  }
>  

It would correct here too, but not sure if it would make any difference given that
some lower bits are discarded anyway.  Probably depends on exact CPU.


And, oh hmm, I read AMD Software Optimization Guide for AMD Family 10h Processors
and they suggest using cpuid (with a note that it may be intercepted in
virtualized environments) or _mfence_ in the discussed role (Appendix F of the
document).
Googling for 'rdtsc mfence lfence' yields some interesting results.

-- 
Andriy Gapon