* [dpdk-dev] [PATCH] timer: add lfence before TSC read @ 2014-01-24 11:17 Didier Pallard 2014-01-24 11:42 ` François-Frédéric Ozog 0 siblings, 1 reply; 6+ messages in thread From: Didier Pallard @ 2014-01-24 11:17 UTC (permalink / raw) To: dev According to Intel Developer's Manual: "The RDTSC instruction is not a serializing instruction. It does not necessarily wait until all previous instructions have been executed before reading the counter. Simi- larly, subsequent instructions may begin execution before the read operation is performed. If software requires RDTSC to be executed only after all previous instruc- tions have completed locally, it can either use RDTSCP (if the processor supports that instruction) or execute the sequence LFENCE;RDTSC." So add a lfence instruction before rdtsc to synchronize read operations and ensure that the read is done at the expected instant. Signed-off-by: Didier Pallard <didier.pallard@6wind.com> --- lib/librte_eal/common/include/rte_cycles.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/librte_eal/common/include/rte_cycles.h b/lib/librte_eal/common/include/rte_cycles.h index cc6fe71..487dba6 100644 --- a/lib/librte_eal/common/include/rte_cycles.h +++ b/lib/librte_eal/common/include/rte_cycles.h @@ -110,6 +110,9 @@ rte_rdtsc(void) }; } tsc; + /* serialize previous load instructions in pipe */ + asm volatile("lfence"); + #ifdef RTE_LIBRTE_EAL_VMWARE_TSC_MAP_SUPPORT if (unlikely(rte_cycles_vmware_tsc_map)) { /* ecx = 0x10000 corresponds to the physical TSC for VMware */ -- 1.7.10.4 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH] timer: add lfence before TSC read 2014-01-24 11:17 [dpdk-dev] [PATCH] timer: add lfence before TSC read Didier Pallard @ 2014-01-24 11:42 ` François-Frédéric Ozog 2014-01-24 12:48 ` Ananyev, Konstantin 2014-01-27 9:57 ` Thomas Monjalon 0 siblings, 2 replies; 6+ messages in thread From: François-Frédéric Ozog @ 2014-01-24 11:42 UTC (permalink / raw) To: 'Didier Pallard', dev Hi, Most of the time rdtsc is used for timestamping and a few cycles incorrect are most of the time not an issue (a precision of 0.1us for session start is usually enough). Sometimes you need to serialize because the time you want to measure is very short, in the order of few nanoseconds. If the code is running in a VM, which usually virtualize rdtsc instruction, then it even make no sense to have more "precision". IMHO, adding the lfence for all cases is introducing an un-necessary performance penalty. What about adding rte_rdtsc_sync() or rte_rdtsc_serial() with the comment about the rdtsc instruction behavior so that developers can choose which form they want? François-Frédéric > -----Message d'origine----- > De : dev [mailto:dev-bounces@dpdk.org] De la part de Didier Pallard > Envoyé : vendredi 24 janvier 2014 12:18 > À : dev@dpdk.org > Objet : [dpdk-dev] [PATCH] timer: add lfence before TSC read > > According to Intel Developer's Manual: > > "The RDTSC instruction is not a serializing instruction. It does not > necessarily wait until all previous instructions have been executed before > reading the counter. Simi- larly, subsequent instructions may begin > execution before the read operation is performed. If software requires > RDTSC to be executed only after all previous instruc- tions have completed > locally, it can either use RDTSCP (if the processor supports that > instruction) or execute the sequence LFENCE;RDTSC." > > So add a lfence instruction before rdtsc to synchronize read operations and > ensure that the read is done at the expected instant. > > Signed-off-by: Didier Pallard <didier.pallard@6wind.com> > --- > lib/librte_eal/common/include/rte_cycles.h | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/lib/librte_eal/common/include/rte_cycles.h > b/lib/librte_eal/common/include/rte_cycles.h > index cc6fe71..487dba6 100644 > --- a/lib/librte_eal/common/include/rte_cycles.h > +++ b/lib/librte_eal/common/include/rte_cycles.h > @@ -110,6 +110,9 @@ rte_rdtsc(void) > }; > } tsc; > > + /* serialize previous load instructions in pipe */ > + asm volatile("lfence"); > + > #ifdef RTE_LIBRTE_EAL_VMWARE_TSC_MAP_SUPPORT > if (unlikely(rte_cycles_vmware_tsc_map)) { > /* ecx = 0x10000 corresponds to the physical TSC for VMware */ > -- > 1.7.10.4 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH] timer: add lfence before TSC read 2014-01-24 11:42 ` François-Frédéric Ozog @ 2014-01-24 12:48 ` Ananyev, Konstantin 2014-01-27 9:57 ` Thomas Monjalon 1 sibling, 0 replies; 6+ messages in thread From: Ananyev, Konstantin @ 2014-01-24 12:48 UTC (permalink / raw) To: François-Frédéric Ozog, 'Didier Pallard', dev Hi, Totally agree with François-Frédéric. Actually was going to suggest exactly the same thing. BTW, there is rte_rmb() defined inside rte_atomic.h, that should produce an lfence instruction. Probably better to use it to keep code consistent. Konstantin -----Original Message----- From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of François-Frédéric Ozog Sent: Friday, January 24, 2014 11:43 AM To: 'Didier Pallard'; dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH] timer: add lfence before TSC read Hi, Most of the time rdtsc is used for timestamping and a few cycles incorrect are most of the time not an issue (a precision of 0.1us for session start is usually enough). Sometimes you need to serialize because the time you want to measure is very short, in the order of few nanoseconds. If the code is running in a VM, which usually virtualize rdtsc instruction, then it even make no sense to have more "precision". IMHO, adding the lfence for all cases is introducing an un-necessary performance penalty. What about adding rte_rdtsc_sync() or rte_rdtsc_serial() with the comment about the rdtsc instruction behavior so that developers can choose which form they want? François-Frédéric > -----Message d'origine----- > De : dev [mailto:dev-bounces@dpdk.org] De la part de Didier Pallard > Envoyé : vendredi 24 janvier 2014 12:18 À : dev@dpdk.org Objet : > [dpdk-dev] [PATCH] timer: add lfence before TSC read > > According to Intel Developer's Manual: > > "The RDTSC instruction is not a serializing instruction. It does not > necessarily wait until all previous instructions have been executed before > reading the counter. Simi- larly, subsequent instructions may begin > execution before the read operation is performed. If software > requires RDTSC to be executed only after all previous instruc- tions > have completed > locally, it can either use RDTSCP (if the processor supports that > instruction) or execute the sequence LFENCE;RDTSC." > > So add a lfence instruction before rdtsc to synchronize read > operations and > ensure that the read is done at the expected instant. > > Signed-off-by: Didier Pallard <didier.pallard@6wind.com> > --- > lib/librte_eal/common/include/rte_cycles.h | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/lib/librte_eal/common/include/rte_cycles.h > b/lib/librte_eal/common/include/rte_cycles.h > index cc6fe71..487dba6 100644 > --- a/lib/librte_eal/common/include/rte_cycles.h > +++ b/lib/librte_eal/common/include/rte_cycles.h > @@ -110,6 +110,9 @@ rte_rdtsc(void) > }; > } tsc; > > + /* serialize previous load instructions in pipe */ > + asm volatile("lfence"); > + > #ifdef RTE_LIBRTE_EAL_VMWARE_TSC_MAP_SUPPORT > if (unlikely(rte_cycles_vmware_tsc_map)) { > /* ecx = 0x10000 corresponds to the physical TSC for VMware */ > -- > 1.7.10.4 -------------------------------------------------------------- Intel Shannon Limited Registered in Ireland Registered Office: Collinstown Industrial Park, Leixlip, County Kildare Registered Number: 308263 Business address: Dromore House, East Park, Shannon, Co. Clare This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH] timer: add lfence before TSC read 2014-01-24 11:42 ` François-Frédéric Ozog 2014-01-24 12:48 ` Ananyev, Konstantin @ 2014-01-27 9:57 ` Thomas Monjalon 2014-01-27 11:58 ` didier.pallard 1 sibling, 1 reply; 6+ messages in thread From: Thomas Monjalon @ 2014-01-27 9:57 UTC (permalink / raw) To: François-Frédéric Ozog, 'Didier Pallard'; +Cc: dev 24/01/2014 12:42, François-Frédéric Ozog: > IMHO, adding the lfence for all cases is introducing an un-necessary > performance penalty. > > What about adding rte_rdtsc_sync() or rte_rdtsc_serial() with the comment > about the rdtsc instruction behavior so that developers can choose which > form they want? Yes it could be a good idea in some cases. Didier, could you try to add such function ? But in some debugging cases we need to have high precision for almost all timestamps. Here I don't know what is the smartest solution. Thank you for commenting. Hope we'll find a good fix. -- Thomas ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH] timer: add lfence before TSC read 2014-01-27 9:57 ` Thomas Monjalon @ 2014-01-27 11:58 ` didier.pallard 2014-01-28 1:28 ` Sangjin Han 0 siblings, 1 reply; 6+ messages in thread From: didier.pallard @ 2014-01-27 11:58 UTC (permalink / raw) To: Thomas Monjalon; +Cc: dev Yes, i will add a new function that includes the lfence; for the performance penalty, we did not see noticable performance impact on our full software, so we did not see any reason to use 2 functions, but it's certainly because we make a very limited number of calls to rdtsc and it's true that it is highly application dependant, so 2 functions are probably better. But if using the unaccurate function, you may have some hard time the first time you want to debug or do some precise measures, since the measure is not always done when expected. And generally, especially when debugging, you're not focusing at first on the function you're using to debug... i don't know how to do to be sure that people will be aware of the problem and do not lose time on the same problem, i will try to add some kind of warning in rte_rdtsc function itself. But perhaps should it be better to use the precise version as default one and let the optimized version with another name to be use on purpose when accuracy is not important; By default, i think we generaly suppose a time reading function to be accurate... thanks didier On 01/27/2014 10:57 AM, Thomas Monjalon wrote: > 24/01/2014 12:42, François-Frédéric Ozog: >> IMHO, adding the lfence for all cases is introducing an un-necessary >> performance penalty. >> >> What about adding rte_rdtsc_sync() or rte_rdtsc_serial() with the comment >> about the rdtsc instruction behavior so that developers can choose which >> form they want? > Yes it could be a good idea in some cases. Didier, could you try to add such > function ? > > But in some debugging cases we need to have high precision for almost all > timestamps. Here I don't know what is the smartest solution. > > Thank you for commenting. Hope we'll find a good fix. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH] timer: add lfence before TSC read 2014-01-27 11:58 ` didier.pallard @ 2014-01-28 1:28 ` Sangjin Han 0 siblings, 0 replies; 6+ messages in thread From: Sangjin Han @ 2014-01-28 1:28 UTC (permalink / raw) To: didier.pallard; +Cc: dev Why LFENCE, rather than CPUID? I guess LFENCE does not prevent out-of-order execution for non-load instructions across it. This link has detailed information on RDTSC, RDTSCP, and CPUID: http://www.intel.com/content/dam/www/public/us/en/documents/white-papers/ia-32-ia-64-benchmark-code-execution-paper.pdf Sangjin On Mon, Jan 27, 2014 at 3:58 AM, didier.pallard <didier.pallard@6wind.com> wrote: > Yes, i will add a new function that includes the lfence; > > for the performance penalty, we did not see noticable performance impact on > our full software, so we did not see any reason to use 2 functions, but it's > certainly because we make a very limited number of calls to rdtsc and it's > true that it is highly application dependant, so 2 functions are probably > better. But if using the unaccurate function, you may have some hard time > the first time you want to debug or do some precise measures, since the > measure is not always done when expected. And generally, especially when > debugging, you're not focusing at first on the function you're using to > debug... > i don't know how to do to be sure that people will be aware of the problem > and do not lose time on the same problem, i will try to add some kind of > warning in rte_rdtsc function itself. > But perhaps should it be better to use the precise version as default one > and let the optimized version with another name to be use on purpose when > accuracy is not important; By default, i think we generaly suppose a time > reading function to be accurate... > > thanks > didier > > > On 01/27/2014 10:57 AM, Thomas Monjalon wrote: >> >> 24/01/2014 12:42, François-Frédéric Ozog: >>> >>> IMHO, adding the lfence for all cases is introducing an un-necessary >>> performance penalty. >>> >>> What about adding rte_rdtsc_sync() or rte_rdtsc_serial() with the comment >>> about the rdtsc instruction behavior so that developers can choose which >>> form they want? >> >> Yes it could be a good idea in some cases. Didier, could you try to add >> such >> function ? >> >> But in some debugging cases we need to have high precision for almost all >> timestamps. Here I don't know what is the smartest solution. >> >> Thank you for commenting. Hope we'll find a good fix. > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-01-28 1:26 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-01-24 11:17 [dpdk-dev] [PATCH] timer: add lfence before TSC read Didier Pallard 2014-01-24 11:42 ` François-Frédéric Ozog 2014-01-24 12:48 ` Ananyev, Konstantin 2014-01-27 9:57 ` Thomas Monjalon 2014-01-27 11:58 ` didier.pallard 2014-01-28 1:28 ` Sangjin Han
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).