[PATCH] timer: add lfence before TSC read

[PATCH] timer: add lfence before TSC read

[Didier Pallard]

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-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
---
 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

Re: [PATCH] timer: add lfence before TSC read

[François-Frédéric Ozog]

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-VfR2kkLFssw@public.gmane.org] De la part de Didier Pallard
> Envoyé : vendredi 24 janvier 2014 12:18
> À : dev-VfR2kkLFssw@public.gmane.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-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
> ---
>  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

Re: [PATCH] timer: add lfence before TSC read

[Ananyev, Konstantin]

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-VfR2kkLFssw@public.gmane.org] On Behalf Of François-Frédéric Ozog
Sent: Friday, January 24, 2014 11:43 AM
To: 'Didier Pallard'; dev-VfR2kkLFssw@public.gmane.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-VfR2kkLFssw@public.gmane.org] De la part de Didier Pallard 
> Envoyé : vendredi 24 janvier 2014 12:18 À : dev-VfR2kkLFssw@public.gmane.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-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
> ---
>  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.

Re: [PATCH] timer: add lfence before TSC read

[Thomas Monjalon]

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

Re: [PATCH] timer: add lfence before TSC read

[didier.pallard]

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.

Re: [PATCH] timer: add lfence before TSC read

[Sangjin Han]

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-pdR9zngts4EAvxtiuMwx3w@public.gmane.org> 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.
>
>
>