Skip site navigation (1)Skip section navigation (2)
Date:      Wed, 25 Jul 2012 11:00:41 -0700
From:      Jim Harris <jim.harris@gmail.com>
To:        Konstantin Belousov <kostikbel@gmail.com>
Cc:        svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Andriy Gapon <avg@freebsd.org>, Bruce Evans <brde@optusnet.com.au>
Subject:   Re: svn commit: r238755 - head/sys/x86/x86
Message-ID:  <CAJP=Hc9Ys6uou2wfcO=r9D%2BAaC34mVeHOK1bgFWtBz8kfvWo6A@mail.gmail.com>
In-Reply-To: <20120725173212.GN2676@deviant.kiev.zoral.com.ua>
References:  <201207242210.q6OMACqV079603@svn.freebsd.org> <500F9E22.4080608@FreeBSD.org> <20120725102130.GH2676@deviant.kiev.zoral.com.ua> <20120725233033.N5406@besplex.bde.org> <20120725173212.GN2676@deviant.kiev.zoral.com.ua>

next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Jul 25, 2012 at 10:32 AM, Konstantin Belousov
<kostikbel@gmail.com> wrote:
> On Thu, Jul 26, 2012 at 12:15:54AM +1000, Bruce Evans wrote:
>> On Wed, 25 Jul 2012, Konstantin Belousov wrote:
>>
>> >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 ?
>> >
>> >>Or is this change universally good for the real uses of TSC?
>>
>> It's too slow for real uses.  But synchronization code, and some uses
>> that requires serialization may need it for, er, synchronization and
>> serialization.
>>
>> It's hard to think of many uses that need serialization.  I often use
>> it for timing instructions.  For timng a large number of instructions,
>> serialization doesn't matter since errors of a few tens in a few billion
>> done matter.  For timing a small number of instructions, I don't want
>> serialization, since the serialization invalidates the timing.
>>
>> Most uses in FreeBSD are for timecounters.  Timecounters deliver the
>> current time.  This is unrelated to whatever instructions haven't
>> completed when the TSC is read.  Except possibly when the time needs
>> to be synchronized across CPUs, and when the uncompleted instruction
>> is a TSC read.
>>
>> >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.
>>
>> Gak.  Even when they are in the same instruction sequence?  Even though
>> the TSC reads fixed registers and some other instructions in the sequence
>> between the TSC use these registers?  The CPU would have to do significant
>> register renaming to break this.
> As I could only speculate, I believe that any modern CPU executes RDTSC
> as at least two separate steps, one is read from internal counter, and
> second is the registers update. It seems that the first kind of action
> is not serialized. I have no other explanation for the Jim findings.
>
> I also asked Jim to test whether the cause the TSC sync test failure
> is the lack of synchronization between gathering data and tasting it,
> but ut appeared that the reason is genuine timecounter value going
> backward.

I wonder if instead of timecounter going backward, that TSC test
fails because CPU speculatively performs rdtsc instruction in relation
to waiter checks in smp_rendezvous_action.  Or maybe we are saying
the same thing.

>
> Sp the bug seems real, and I cannot imagine we will live with the known
> defect in timecounters which can step back.
>>
>> >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.
>>
>> This is not a fix if it is full serialization.  It just gives slowness
>> using a single instruction instead of a couple.
>>
>> >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());
>> >}
>>
>> Please don't pessimize this further.  The time for rdtsc went from 6.5
>> cycles on AthlonXP to 65 cycles on core2 (mainly for for
>> P-state-invariance hardware synchronization I think).  Pretty soon it
>> will be as slow as an HPET and heading towards an i8254.  Adding rmb()
>> only makes it 12 cycles slower on core2, but 16 cycles (almost 3 times)
>> slower on AthlonXP.
> AthlonXP does not look as interesting target for optimizations. Fom what I
> can find this is PIII-era CPU.
>
>>
>> >@@ -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);
>> >}
>>
>> The previous TSC-low/shrd pessimization adds only 2 cycles on AthlonXP
>> and core2.  I think it only "works" by backing the TSC's resolution
>> so low that it usually can't see its own, or at least other TSC's lack of
>> serialness.  The shift count is usually 7 or 8, so the resolution is
>> reduced from 1 cycle to 128 or 256.  Out of order times that fall in
>> the same block of 128 or 256 cycles would appear to be the same, but
>> out of order times like 129 and 127 would apear to be even more out
>> of order after a shift of 7 turns them into 128 and 0.
>>
>> Bruce



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJP=Hc9Ys6uou2wfcO=r9D%2BAaC34mVeHOK1bgFWtBz8kfvWo6A>