[dpdk-dev] [PATCH v2 0/5] improve tsc frequency calibration

[dpdk-dev] [PATCH v2 0/5] improve tsc frequency calibration

[Gowrishankar]

From: Gowrishankar Muthukrishnan <gowrishankar.m@linux.vnet.ibm.com>

Some architecture like armv8 provides an architecture specific function
to get the rdtsc frequency. The existing rdtsc calibration scheme uses
OS serivce like sleep(1) to calibrate the frequency which may not
produce the accurate result. Introducing an architecture specific hook
to get the rdtsc frequency if architecture provides it. If not, use the
exiting the calibrate scheme to get the rdtsc frequency.

Jerin Jacob (5):
  eal/x86: define architecture specific rdtsc hz
  eal/ppc64: define architecture specific rdtsc hz
  eal/armv7: define architecture specific rdtsc hz
  eal/armv8: define architecture specific rdtsc hz
  eal/timer: honor architecture specific rdtsc hz function

 lib/librte_eal/common/eal_common_timer.c           |  5 +++-
 .../common/include/arch/arm/rte_cycles_32.h        | 13 ++++++++++
 .../common/include/arch/arm/rte_cycles_64.h        | 30 ++++++++++++++++++++++
 .../common/include/arch/ppc_64/rte_cycles.h        | 24 +++++++++++++++++
 .../common/include/arch/x86/rte_cycles.h           | 13 ++++++++++
 5 files changed, 84 insertions(+), 1 deletion(-)

-- 
1.9.1

[dpdk-dev] [PATCH v2 1/5] eal/x86: define architecture specific rdtsc hz

[Gowrishankar]

From: Jerin Jacob <jerin.jacob@caviumnetworks.com>

CC: Bruce Richardson <bruce.richardson@intel.com>
CC: Konstantin Ananyev <konstantin.ananyev@intel.com>

Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/common/include/arch/x86/rte_cycles.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/lib/librte_eal/common/include/arch/x86/rte_cycles.h b/lib/librte_eal/common/include/arch/x86/rte_cycles.h
index 1bb3e1d..e2661e2 100644
--- a/lib/librte_eal/common/include/arch/x86/rte_cycles.h
+++ b/lib/librte_eal/common/include/arch/x86/rte_cycles.h
@@ -77,6 +77,19 @@
 	return tsc.tsc_64;
 }
 
+/**
+ * Get the number of rdtsc cycles in one second if the architecture supports.
+ *
+ * @return
+ *   The number of rdtsc cycles in one second. Return zero if the architecture
+ *   support is not available.
+ */
+static inline uint64_t
+rte_rdtsc_arch_hz(void)
+{
+	return 0;
+}
+
 static inline uint64_t
 rte_rdtsc_precise(void)
 {
-- 
1.9.1

[dpdk-dev] [PATCH v2 2/5] eal/ppc64: define architecture specific rdtsc hz

[Gowrishankar]

From: Jerin Jacob <jerin.jacob@caviumnetworks.com>

In ppc_64, rte_rdtsc() returns timebase register value which increments
at independent timebase frequency and hence not related to lcore cpu
frequency to derive TSC hz. Hence, we stick with master lcore frequency.

CC: Chao Zhu <chaozhu@linux.vnet.ibm.com>
Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
Signed-off-by: Gowrishankar Muthukrishnan <gowrishankar.m@linux.vnet.ibm.com>
---

v2:
 * add ppc_64 specific implementation

 .../common/include/arch/ppc_64/rte_cycles.h        | 24 ++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/lib/librte_eal/common/include/arch/ppc_64/rte_cycles.h b/lib/librte_eal/common/include/arch/ppc_64/rte_cycles.h
index 8fa6fc6..1b36587 100644
--- a/lib/librte_eal/common/include/arch/ppc_64/rte_cycles.h
+++ b/lib/librte_eal/common/include/arch/ppc_64/rte_cycles.h
@@ -38,9 +38,13 @@
 #endif
 
 #include "generic/rte_cycles.h"
+#include "../../lib/librte_eal/common/eal_filesystem.h"
 
 #include <rte_byteorder.h>
 #include <rte_common.h>
+#include <rte_lcore.h>
+
+static const char sys_cpu_dir[] = "/sys/devices/system/cpu";
 
 /**
  * Read the time base register.
@@ -79,6 +83,26 @@
 	return tsc.tsc_64;
 }
 
+/**
+ * Get the number of rdtsc cycles in one second.
+ *
+ * @return
+ *   The number of rdtsc cycles in one second.
+ */
+static inline uint64_t
+rte_rdtsc_arch_hz(void)
+{
+	unsigned long cpu_hz;
+	char path[PATH_MAX];
+
+	snprintf(path, sizeof(path), "%s/cpu%d/cpufreq/cpuinfo_cur_freq",
+			sys_cpu_dir, rte_get_master_lcore());
+	if (eal_parse_sysfs_value(path, &cpu_hz) < 0)
+		RTE_LOG(WARNING, EAL, "Unable to parse %s\n", path);
+
+	return cpu_hz*1000;
+}
+
 static inline uint64_t
 rte_rdtsc_precise(void)
 {
-- 
1.9.1

[dpdk-dev] [PATCH v2 3/5] eal/armv7: define architecture specific rdtsc hz

[Gowrishankar]

From: Jerin Jacob <jerin.jacob@caviumnetworks.com>

CC: Jan Viktorin <viktorin@rehivetech.com>
CC: Jianbo Liu <jianbo.liu@linaro.org>
Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
---
 lib/librte_eal/common/include/arch/arm/rte_cycles_32.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/lib/librte_eal/common/include/arch/arm/rte_cycles_32.h b/lib/librte_eal/common/include/arch/arm/rte_cycles_32.h
index 9c1be71..68d7462 100644
--- a/lib/librte_eal/common/include/arch/arm/rte_cycles_32.h
+++ b/lib/librte_eal/common/include/arch/arm/rte_cycles_32.h
@@ -104,6 +104,19 @@
 
 #endif /* RTE_ARM_EAL_RDTSC_USE_PMU */
 
+/**
+ * Get the number of rdtsc cycles in one second if the architecture supports.
+ *
+ * @return
+ *   The number of rdtsc cycles in one second. Return zero if the architecture
+ *   support is not available.
+ */
+static inline uint64_t
+rte_rdtsc_arch_hz(void)
+{
+	return 0;
+}
+
 static inline uint64_t
 rte_rdtsc_precise(void)
 {
-- 
1.9.1

[dpdk-dev] [PATCH v2 4/5] eal/armv8: define architecture specific rdtsc hz

[Gowrishankar]

From: Jerin Jacob <jerin.jacob@caviumnetworks.com>

Use cntvct_el0 system register to get the system counter frequency.

If the system is configured with RTE_ARM_EAL_RDTSC_USE_PMU then
return 0(let the common code calibrate the tsc frequency).

CC: Jianbo Liu <jianbo.liu@linaro.org>
Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
Acked-by: Jianbo Liu <jianbo.liu@linaro.org>
---
 .../common/include/arch/arm/rte_cycles_64.h        | 30 ++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/lib/librte_eal/common/include/arch/arm/rte_cycles_64.h b/lib/librte_eal/common/include/arch/arm/rte_cycles_64.h
index 1545769..5b95cb6 100644
--- a/lib/librte_eal/common/include/arch/arm/rte_cycles_64.h
+++ b/lib/librte_eal/common/include/arch/arm/rte_cycles_64.h
@@ -58,6 +58,23 @@
 	asm volatile("mrs %0, cntvct_el0" : "=r" (tsc));
 	return tsc;
 }
+
+/**
+ * Get the number of rdtsc cycles in one second if the architecture supports.
+ *
+ * @return
+ *   The number of rdtsc cycles in one second. Return zero if the architecture
+ *   support is not available.
+ */
+static inline uint64_t
+rte_rdtsc_arch_hz(void)
+{
+	uint64_t freq;
+
+	asm volatile("mrs %0, cntfrq_el0" : "=r" (freq));
+	return freq;
+}
+
 #else
 /**
  * This is an alternative method to enable rte_rdtsc() with high resolution
@@ -85,6 +102,19 @@
 	asm volatile("mrs %0, pmccntr_el0" : "=r"(tsc));
 	return tsc;
 }
+
+/**
+ * Get the number of rdtsc cycles in one second if the architecture supports.
+ *
+ * @return
+ *   The number of rdtsc cycles in one second. Return zero if the architecture
+ *   support is not available.
+ */
+static inline uint64_t
+rte_rdtsc_arch_hz(void)
+{
+	return 0;
+}
 #endif
 
 static inline uint64_t
-- 
1.9.1

[dpdk-dev] [PATCH v2 5/5] eal/timer: honor architecture specific rdtsc hz function

[Gowrishankar]

From: Jerin Jacob <jerin.jacob@caviumnetworks.com>

When calibrating the tsc frequency, first, probe the architecture specific
rdtsc hz function. if not available, use the existing calibrate scheme
to calibrate the tsc frequency.

Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
---
 lib/librte_eal/common/eal_common_timer.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/eal_common_timer.c b/lib/librte_eal/common/eal_common_timer.c
index ed0b16d..978edae 100644
--- a/lib/librte_eal/common/eal_common_timer.c
+++ b/lib/librte_eal/common/eal_common_timer.c
@@ -80,8 +80,11 @@
 void
 set_tsc_freq(void)
 {
-	uint64_t freq = get_tsc_freq();
+	uint64_t freq;
 
+	freq = rte_rdtsc_arch_hz();
+	if (!freq)
+		freq = get_tsc_freq();
 	if (!freq)
 		freq = estimate_tsc_freq();
 
-- 
1.9.1

Re: [dpdk-dev] [PATCH v2 5/5] eal/timer: honor architecture specific rdtsc hz function

[Thomas Monjalon]

22/09/2017 10:25, Gowrishankar:
> From: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> 
> When calibrating the tsc frequency, first, probe the architecture specific
> rdtsc hz function. if not available, use the existing calibrate scheme
> to calibrate the tsc frequency.
> 
> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>

I agree on the idea.

The namespace of cycles related function in DPDK is a real mess.
I think we can choose better names in this series as a first step
to tidy this mess.
I will explain below.

At first, we should avoid TSC and RDTSC which are Intel-only wording.
The generic word could be "cycles" (the word used in arch headers),
or "ticks".
We should also name the timer sources or their function in a generic way.
Examples: CPU cycles? fast counter? precise counter?

Sometimes we use "hz", sometimes "freq".
It would better to keep one of them.

> --- a/lib/librte_eal/common/eal_common_timer.c
> +++ b/lib/librte_eal/common/eal_common_timer.c
> @@ -80,8 +80,11 @@
>  void
>  set_tsc_freq(void)
>  {
> -	uint64_t freq = get_tsc_freq();
> +	uint64_t freq;
>  
> +	freq = rte_rdtsc_arch_hz();

This new function is arch-specific and exported as a new API.

> +	if (!freq)
> +		freq = get_tsc_freq();

The function get_tsc_freq is guessing the freq with OS-specific method.

>  	if (!freq)
>  		freq = estimate_tsc_freq();

The function estimate_tsc_freq is doing an estimation based on sleep().

At the end, the most accurate frequency is saved in eal_tsc_resolution_hz
and can be retrieved with rte_get_tsc_hz().
I don't understand why rte_rdtsc_arch_hz() is also exported to the apps.

TSC and HPET timer sources are wrapped in rte_get_timer_hz() in the
generic code despite HPET is Intel specific.

Similarly we can get the current timer with rte_get_timer_cycles().
In the case of TSC, it calls rte_get_tsc_cycles() which is an alias
of rte_rdtsc().
Some code is still using directly rte_rdtsc().
There is also rte_rdtsc_precise which adds a memory barrier.

The real question is what is the right abstraction for the application?
Do we want the fastest timer? the CPU timer? a precise timer?

I would like to see a real discussion on this topic, in order of building
a new timer API which would alias the old one for some time.

If you don't want to bother with all these questions, I suggest to not
export the new function rte_rdtsc_arch_hz() and rename it to tsc_arch_hz.

Re: [dpdk-dev] [PATCH v2 5/5] eal/timer: honor architecture specific rdtsc hz function

[Jerin Jacob]

-----Original Message-----
> Date: Wed, 11 Oct 2017 19:36:11 +0200
> From: Thomas Monjalon <thomas@monjalon.net>
> To: Gowrishankar <gowrishankar.m@linux.vnet.ibm.com>, Jerin Jacob
>  <jerin.jacob@caviumnetworks.com>
> Cc: dev@dpdk.org, Chao Zhu <chaozhu@linux.vnet.ibm.com>, Bruce Richardson
>  <bruce.richardson@intel.com>, Konstantin Ananyev
>  <konstantin.ananyev@intel.com>, viktorin@rehivetech.com,
>  jianbo.liu@linaro.org
> Subject: Re: [dpdk-dev] [PATCH v2 5/5] eal/timer: honor architecture
>  specific rdtsc hz function
> 
> 22/09/2017 10:25, Gowrishankar:
> > From: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > 
> > When calibrating the tsc frequency, first, probe the architecture specific
> > rdtsc hz function. if not available, use the existing calibrate scheme
> > to calibrate the tsc frequency.
> > 
> > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> 
> I agree on the idea.

OK

> 
> The namespace of cycles related function in DPDK is a real mess.

Absolutely!!

> I think we can choose better names in this series as a first step
> to tidy this mess.
> I will explain below.
> 
> At first, we should avoid TSC and RDTSC which are Intel-only wording.
> The generic word could be "cycles" (the word used in arch headers),
> or "ticks".
> We should also name the timer sources or their function in a generic way.
> Examples: CPU cycles? fast counter? precise counter?
> 
> Sometimes we use "hz", sometimes "freq".
> It would better to keep one of them.
> 
> > --- a/lib/librte_eal/common/eal_common_timer.c
> > +++ b/lib/librte_eal/common/eal_common_timer.c
> > @@ -80,8 +80,11 @@
> >  void
> >  set_tsc_freq(void)
> >  {
> > -	uint64_t freq = get_tsc_freq();
> > +	uint64_t freq;
> >  
> > +	freq = rte_rdtsc_arch_hz();
> 
> This new function is arch-specific and exported as a new API.

I thought of avoid exporting it. But then if the function is in
lib/librte_eal/common/include/arch/../rte_cycles.h it is anyway exposed to
application. i.e whatever files in lib/librte_eal/common/include/arch/../
anyway exposed to application. 

See last comment.

> 
> > +	if (!freq)
> > +		freq = get_tsc_freq();
> 
> The function get_tsc_freq is guessing the freq with OS-specific method.
> 
> >  	if (!freq)
> >  		freq = estimate_tsc_freq();
> 
> The function estimate_tsc_freq is doing an estimation based on sleep().
> 
> At the end, the most accurate frequency is saved in eal_tsc_resolution_hz
> and can be retrieved with rte_get_tsc_hz().
> I don't understand why rte_rdtsc_arch_hz() is also exported to the apps.
> 
> TSC and HPET timer sources are wrapped in rte_get_timer_hz() in the
> Similarly we can get the current timer with rte_get_timer_cycles().
> In the case of TSC, it calls rte_get_tsc_cycles() which is an alias
> of rte_rdtsc().
> Some code is still using directly rte_rdtsc().
> There is also rte_rdtsc_precise which adds a memory barrier.
> 
> The real question is what is the right abstraction for the application?
> Do we want the fastest timer? the CPU timer? a precise timer?
> 
> I would like to see a real discussion on this topic, in order of building
> a new timer API which would alias the old one for some time.

I guess, we may need to see to how abstract vmware TSC support also in
proper way

> 
> If you don't want to bother with all these questions, I suggest to not
> export the new function rte_rdtsc_arch_hz() and rename it to tsc_arch_hz.

If I understand it correctly, You would like to create a header file 
in lib/librte_eal/common/include/arch/../ which should not be exported and change
the name to tsc_arch_hz.

Re: [dpdk-dev] [PATCH v2 5/5] eal/timer: honor architecture specific rdtsc hz function

[Thomas Monjalon]

11/10/2017 20:57, Jerin Jacob:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 22/09/2017 10:25, Gowrishankar:
> > > From: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > 
> > > When calibrating the tsc frequency, first, probe the architecture specific
> > > rdtsc hz function. if not available, use the existing calibrate scheme
> > > to calibrate the tsc frequency.
> > > 
> > > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > 
> > I agree on the idea.
> 
> OK
> 
> > The namespace of cycles related function in DPDK is a real mess.
> 
> Absolutely!!
> 
> > I think we can choose better names in this series as a first step
> > to tidy this mess.
> > I will explain below.
> > 
> > At first, we should avoid TSC and RDTSC which are Intel-only wording.
> > The generic word could be "cycles" (the word used in arch headers),
> > or "ticks".
> > We should also name the timer sources or their function in a generic way.
> > Examples: CPU cycles? fast counter? precise counter?
> > 
> > Sometimes we use "hz", sometimes "freq".
> > It would better to keep one of them.
> > 
> > > --- a/lib/librte_eal/common/eal_common_timer.c
> > > +++ b/lib/librte_eal/common/eal_common_timer.c
> > > @@ -80,8 +80,11 @@
> > >  void
> > >  set_tsc_freq(void)
> > >  {
> > > -	uint64_t freq = get_tsc_freq();
> > > +	uint64_t freq;
> > >  
> > > +	freq = rte_rdtsc_arch_hz();
> > 
> > This new function is arch-specific and exported as a new API.
> 
> I thought of avoid exporting it. But then if the function is in
> lib/librte_eal/common/include/arch/../rte_cycles.h it is anyway exposed to
> application. i.e whatever files in lib/librte_eal/common/include/arch/../
> anyway exposed to application.

Ah yes, you are right!

> See last comment.
> 
> > 
> > > +	if (!freq)
> > > +		freq = get_tsc_freq();
> > 
> > The function get_tsc_freq is guessing the freq with OS-specific method.
> > 
> > >  	if (!freq)
> > >  		freq = estimate_tsc_freq();
> > 
> > The function estimate_tsc_freq is doing an estimation based on sleep().
> > 
> > At the end, the most accurate frequency is saved in eal_tsc_resolution_hz
> > and can be retrieved with rte_get_tsc_hz().
> > I don't understand why rte_rdtsc_arch_hz() is also exported to the apps.
> > 
> > TSC and HPET timer sources are wrapped in rte_get_timer_hz() in the
> > Similarly we can get the current timer with rte_get_timer_cycles().
> > In the case of TSC, it calls rte_get_tsc_cycles() which is an alias
> > of rte_rdtsc().
> > Some code is still using directly rte_rdtsc().
> > There is also rte_rdtsc_precise which adds a memory barrier.
> > 
> > The real question is what is the right abstraction for the application?
> > Do we want the fastest timer? the CPU timer? a precise timer?
> > 
> > I would like to see a real discussion on this topic, in order of building
> > a new timer API which would alias the old one for some time.
> 
> I guess, we may need to see to how abstract vmware TSC support also in
> proper way

Yes

> > If you don't want to bother with all these questions, I suggest to not
> > export the new function rte_rdtsc_arch_hz() and rename it to tsc_arch_hz.
> 
> If I understand it correctly, You would like to create a header file 
> in lib/librte_eal/common/include/arch/../ which should not be exported and change
> the name to tsc_arch_hz.

I had not think about the way to do this.
What about having internal headers in lib/librte_eal/common/arch/ ?

Re: [dpdk-dev] [PATCH v2 5/5] eal/timer: honor architecture specific rdtsc hz function

[Bruce Richardson]

On Wed, Oct 11, 2017 at 09:25:58PM +0200, Thomas Monjalon wrote:
> 11/10/2017 20:57, Jerin Jacob:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 22/09/2017 10:25, Gowrishankar:
> > > > From: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > > 
> > > > When calibrating the tsc frequency, first, probe the architecture specific
> > > > rdtsc hz function. if not available, use the existing calibrate scheme
> > > > to calibrate the tsc frequency.
> > > > 
> > > > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > 
> > > I agree on the idea.
> > 
> > OK
> > 
> > > The namespace of cycles related function in DPDK is a real mess.
> > 
> > Absolutely!!
> > 
> > > I think we can choose better names in this series as a first step
> > > to tidy this mess.
> > > I will explain below.
> > > 
> > > At first, we should avoid TSC and RDTSC which are Intel-only wording.
> > > The generic word could be "cycles" (the word used in arch headers),
> > > or "ticks".
> > > We should also name the timer sources or their function in a generic way.
> > > Examples: CPU cycles? fast counter? precise counter?
> > > 
> > > Sometimes we use "hz", sometimes "freq".
> > > It would better to keep one of them.
> > > 
> > > > --- a/lib/librte_eal/common/eal_common_timer.c
> > > > +++ b/lib/librte_eal/common/eal_common_timer.c
> > > > @@ -80,8 +80,11 @@
> > > >  void
> > > >  set_tsc_freq(void)
> > > >  {
> > > > -	uint64_t freq = get_tsc_freq();
> > > > +	uint64_t freq;
> > > >  
> > > > +	freq = rte_rdtsc_arch_hz();
> > > 
> > > This new function is arch-specific and exported as a new API.
> > 
> > I thought of avoid exporting it. But then if the function is in
> > lib/librte_eal/common/include/arch/../rte_cycles.h it is anyway exposed to
> > application. i.e whatever files in lib/librte_eal/common/include/arch/../
> > anyway exposed to application.
> 
> Ah yes, you are right!
> 
> > See last comment.
> > 
> > > 
> > > > +	if (!freq)
> > > > +		freq = get_tsc_freq();
> > > 
> > > The function get_tsc_freq is guessing the freq with OS-specific method.
> > > 
> > > >  	if (!freq)
> > > >  		freq = estimate_tsc_freq();
> > > 
> > > The function estimate_tsc_freq is doing an estimation based on sleep().
> > > 
> > > At the end, the most accurate frequency is saved in eal_tsc_resolution_hz
> > > and can be retrieved with rte_get_tsc_hz().
> > > I don't understand why rte_rdtsc_arch_hz() is also exported to the apps.
> > > 
> > > TSC and HPET timer sources are wrapped in rte_get_timer_hz() in the
> > > Similarly we can get the current timer with rte_get_timer_cycles().
> > > In the case of TSC, it calls rte_get_tsc_cycles() which is an alias
> > > of rte_rdtsc().
> > > Some code is still using directly rte_rdtsc().
> > > There is also rte_rdtsc_precise which adds a memory barrier.
> > > 
> > > The real question is what is the right abstraction for the application?
> > > Do we want the fastest timer? the CPU timer? a precise timer?
> > > 
> > > I would like to see a real discussion on this topic, in order of building
> > > a new timer API which would alias the old one for some time.
> > 
> > I guess, we may need to see to how abstract vmware TSC support also in
> > proper way
> 
> Yes
> 
> > > If you don't want to bother with all these questions, I suggest to not
> > > export the new function rte_rdtsc_arch_hz() and rename it to tsc_arch_hz.
> > 
> > If I understand it correctly, You would like to create a header file 
> > in lib/librte_eal/common/include/arch/../ which should not be exported and change
> > the name to tsc_arch_hz.
> 
> I had not think about the way to do this.
> What about having internal headers in lib/librte_eal/common/arch/ ?
> 

Yes, this area needs cleanup, but I also think that this patchset (and
follow-on patch for x86-specific implementation) does not make things
significantly worse than they are now, while also giving significant
benefits for users both in terms of improved clock accuracy and reduced
startup time (due to not needing sleep). Therefore I'd like to see this
merged into 17.11 as a definite improvement, even if it does not "fix" the
bigger issues of poor naming etc. I think this is important enough an
improvement that we need to see it in the LTS release.

/Bruce

Re: [dpdk-dev] [PATCH v2 5/5] eal/timer: honor architecture specific rdtsc hz function

[Thomas Monjalon]

12/10/2017 10:48, Bruce Richardson:
> On Wed, Oct 11, 2017 at 09:25:58PM +0200, Thomas Monjalon wrote:
> > 11/10/2017 20:57, Jerin Jacob:
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > 22/09/2017 10:25, Gowrishankar:
> > > > > From: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > > > 
> > > > > When calibrating the tsc frequency, first, probe the architecture specific
> > > > > rdtsc hz function. if not available, use the existing calibrate scheme
> > > > > to calibrate the tsc frequency.
> > > > > 
> > > > > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > > 
> > > > I agree on the idea.
> > > 
> > > OK
> > > 
> > > > The namespace of cycles related function in DPDK is a real mess.
> > > 
> > > Absolutely!!
> > > 
> > > > I think we can choose better names in this series as a first step
> > > > to tidy this mess.
> > > > I will explain below.
> > > > 
> > > > At first, we should avoid TSC and RDTSC which are Intel-only wording.
> > > > The generic word could be "cycles" (the word used in arch headers),
> > > > or "ticks".
> > > > We should also name the timer sources or their function in a generic way.
> > > > Examples: CPU cycles? fast counter? precise counter?
> > > > 
> > > > Sometimes we use "hz", sometimes "freq".
> > > > It would better to keep one of them.
> > > > 
> > > > > --- a/lib/librte_eal/common/eal_common_timer.c
> > > > > +++ b/lib/librte_eal/common/eal_common_timer.c
> > > > > @@ -80,8 +80,11 @@
> > > > >  void
> > > > >  set_tsc_freq(void)
> > > > >  {
> > > > > -	uint64_t freq = get_tsc_freq();
> > > > > +	uint64_t freq;
> > > > >  
> > > > > +	freq = rte_rdtsc_arch_hz();
> > > > 
> > > > This new function is arch-specific and exported as a new API.
> > > 
> > > I thought of avoid exporting it. But then if the function is in
> > > lib/librte_eal/common/include/arch/../rte_cycles.h it is anyway exposed to
> > > application. i.e whatever files in lib/librte_eal/common/include/arch/../
> > > anyway exposed to application.
> > 
> > Ah yes, you are right!
> > 
> > > See last comment.
> > > 
> > > > 
> > > > > +	if (!freq)
> > > > > +		freq = get_tsc_freq();
> > > > 
> > > > The function get_tsc_freq is guessing the freq with OS-specific method.
> > > > 
> > > > >  	if (!freq)
> > > > >  		freq = estimate_tsc_freq();
> > > > 
> > > > The function estimate_tsc_freq is doing an estimation based on sleep().
> > > > 
> > > > At the end, the most accurate frequency is saved in eal_tsc_resolution_hz
> > > > and can be retrieved with rte_get_tsc_hz().
> > > > I don't understand why rte_rdtsc_arch_hz() is also exported to the apps.
> > > > 
> > > > TSC and HPET timer sources are wrapped in rte_get_timer_hz() in the
> > > > Similarly we can get the current timer with rte_get_timer_cycles().
> > > > In the case of TSC, it calls rte_get_tsc_cycles() which is an alias
> > > > of rte_rdtsc().
> > > > Some code is still using directly rte_rdtsc().
> > > > There is also rte_rdtsc_precise which adds a memory barrier.
> > > > 
> > > > The real question is what is the right abstraction for the application?
> > > > Do we want the fastest timer? the CPU timer? a precise timer?
> > > > 
> > > > I would like to see a real discussion on this topic, in order of building
> > > > a new timer API which would alias the old one for some time.
> > > 
> > > I guess, we may need to see to how abstract vmware TSC support also in
> > > proper way
> > 
> > Yes
> > 
> > > > If you don't want to bother with all these questions, I suggest to not
> > > > export the new function rte_rdtsc_arch_hz() and rename it to tsc_arch_hz.
> > > 
> > > If I understand it correctly, You would like to create a header file 
> > > in lib/librte_eal/common/include/arch/../ which should not be exported and change
> > > the name to tsc_arch_hz.
> > 
> > I had not think about the way to do this.
> > What about having internal headers in lib/librte_eal/common/arch/ ?
> > 
> 
> Yes, this area needs cleanup, but I also think that this patchset (and
> follow-on patch for x86-specific implementation) does not make things
> significantly worse than they are now, while also giving significant
> benefits for users both in terms of improved clock accuracy and reduced
> startup time (due to not needing sleep). Therefore I'd like to see this
> merged into 17.11 as a definite improvement, even if it does not "fix" the
> bigger issues of poor naming etc. I think this is important enough an
> improvement that we need to see it in the LTS release.

I agree Bruce.
My initial point was to avoid exporting a new function with a wrong name.
It would be better to find a way to avoid this export.
If it cannot be done in 17.11 timeframe, we can at least add the
experimental tag.

Re: [dpdk-dev] [PATCH v2 5/5] eal/timer: honor architecture specific rdtsc hz function

[Jerin Jacob]

-----Original Message-----
> Date: Wed, 11 Oct 2017 21:25:58 +0200
> From: Thomas Monjalon <thomas@monjalon.net>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> Cc: Gowrishankar <gowrishankar.m@linux.vnet.ibm.com>, dev@dpdk.org, Chao
>  Zhu <chaozhu@linux.vnet.ibm.com>, Bruce Richardson
>  <bruce.richardson@intel.com>, Konstantin Ananyev
>  <konstantin.ananyev@intel.com>, viktorin@rehivetech.com,
>  jianbo.liu@linaro.org
> Subject: Re: [dpdk-dev] [PATCH v2 5/5] eal/timer: honor architecture
>  specific rdtsc hz function
> 
> 11/10/2017 20:57, Jerin Jacob:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 22/09/2017 10:25, Gowrishankar:
> > > > From: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > > 
> > > > When calibrating the tsc frequency, first, probe the architecture specific
> > > > rdtsc hz function. if not available, use the existing calibrate scheme
> > > > to calibrate the tsc frequency.
> > > > 
> > > > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > 
> > > I agree on the idea.
> > 
> > OK
> > 
> > > The namespace of cycles related function in DPDK is a real mess.
> > 
> > Absolutely!!
> > 
> > > I think we can choose better names in this series as a first step
> > > to tidy this mess.
> > > I will explain below.
> > > 
> > > At first, we should avoid TSC and RDTSC which are Intel-only wording.
> > > The generic word could be "cycles" (the word used in arch headers),
> > > or "ticks".
> > > We should also name the timer sources or their function in a generic way.
> > > Examples: CPU cycles? fast counter? precise counter?
> > > 
> > > Sometimes we use "hz", sometimes "freq".
> > > It would better to keep one of them.
> > > 
> > > > --- a/lib/librte_eal/common/eal_common_timer.c
> > > > +++ b/lib/librte_eal/common/eal_common_timer.c
> > > > @@ -80,8 +80,11 @@
> > > >  void
> > > >  set_tsc_freq(void)
> > > >  {
> > > > -	uint64_t freq = get_tsc_freq();
> > > > +	uint64_t freq;
> > > >  
> > > > +	freq = rte_rdtsc_arch_hz();
> > > 
> > > This new function is arch-specific and exported as a new API.
> > 
> > I thought of avoid exporting it. But then if the function is in
> > lib/librte_eal/common/include/arch/../rte_cycles.h it is anyway exposed to
> > application. i.e whatever files in lib/librte_eal/common/include/arch/../
> > anyway exposed to application.
> 
> Ah yes, you are right!
> 
> > See last comment.
> > 
> > > 
> > > > +	if (!freq)
> > > > +		freq = get_tsc_freq();
> > > 
> > > The function get_tsc_freq is guessing the freq with OS-specific method.
> > > 
> > > >  	if (!freq)
> > > >  		freq = estimate_tsc_freq();
> > > 
> > > The function estimate_tsc_freq is doing an estimation based on sleep().
> > > 
> > > At the end, the most accurate frequency is saved in eal_tsc_resolution_hz
> > > and can be retrieved with rte_get_tsc_hz().
> > > I don't understand why rte_rdtsc_arch_hz() is also exported to the apps.
> > > 
> > > TSC and HPET timer sources are wrapped in rte_get_timer_hz() in the
> > > Similarly we can get the current timer with rte_get_timer_cycles().
> > > In the case of TSC, it calls rte_get_tsc_cycles() which is an alias
> > > of rte_rdtsc().
> > > Some code is still using directly rte_rdtsc().
> > > There is also rte_rdtsc_precise which adds a memory barrier.
> > > 
> > > The real question is what is the right abstraction for the application?
> > > Do we want the fastest timer? the CPU timer? a precise timer?
> > > 
> > > I would like to see a real discussion on this topic, in order of building
> > > a new timer API which would alias the old one for some time.
> > 
> > I guess, we may need to see to how abstract vmware TSC support also in
> > proper way
> 
> Yes
> 
> > > If you don't want to bother with all these questions, I suggest to not
> > > export the new function rte_rdtsc_arch_hz() and rename it to tsc_arch_hz.
> > 
> > If I understand it correctly, You would like to create a header file 
> > in lib/librte_eal/common/include/arch/../ which should not be exported and change
> > the name to tsc_arch_hz.
> 
> I had not think about the way to do this.
> What about having internal headers in lib/librte_eal/common/arch/ ?

Looks bit odd when compare with existing scheme of arch function management.
Anyway the whole timer stuff needs to be cleanup.
I think, we can take that when do the cleanup.

On the other side, This API has real ARM64, PPC, x86 implementation now.
I think,it is important to have this feature for v17.11

> 

Re: [dpdk-dev] [PATCH v2 5/5] eal/timer: honor architecture specific rdtsc hz function

[Bruce Richardson]

On Thu, Oct 12, 2017 at 12:12:48PM +0200, Thomas Monjalon wrote:
> 12/10/2017 10:48, Bruce Richardson:
> > On Wed, Oct 11, 2017 at 09:25:58PM +0200, Thomas Monjalon wrote:
> > > 11/10/2017 20:57, Jerin Jacob:
> > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > 22/09/2017 10:25, Gowrishankar:
> > > > > > From: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > > > > 
> > > > > > When calibrating the tsc frequency, first, probe the architecture specific
> > > > > > rdtsc hz function. if not available, use the existing calibrate scheme
> > > > > > to calibrate the tsc frequency.
> > > > > > 
> > > > > > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > > > 
> > > > > I agree on the idea.
> > > > 
> > > > OK
> > > > 
> > > > > The namespace of cycles related function in DPDK is a real mess.
> > > > 
> > > > Absolutely!!
> > > > 
> > > > > I think we can choose better names in this series as a first step
> > > > > to tidy this mess.
> > > > > I will explain below.
> > > > > 
> > > > > At first, we should avoid TSC and RDTSC which are Intel-only wording.
> > > > > The generic word could be "cycles" (the word used in arch headers),
> > > > > or "ticks".
> > > > > We should also name the timer sources or their function in a generic way.
> > > > > Examples: CPU cycles? fast counter? precise counter?
> > > > > 
> > > > > Sometimes we use "hz", sometimes "freq".
> > > > > It would better to keep one of them.
> > > > > 
> > > > > > --- a/lib/librte_eal/common/eal_common_timer.c
> > > > > > +++ b/lib/librte_eal/common/eal_common_timer.c
> > > > > > @@ -80,8 +80,11 @@
> > > > > >  void
> > > > > >  set_tsc_freq(void)
> > > > > >  {
> > > > > > -	uint64_t freq = get_tsc_freq();
> > > > > > +	uint64_t freq;
> > > > > >  
> > > > > > +	freq = rte_rdtsc_arch_hz();
> > > > > 
> > > > > This new function is arch-specific and exported as a new API.
> > > > 
> > > > I thought of avoid exporting it. But then if the function is in
> > > > lib/librte_eal/common/include/arch/../rte_cycles.h it is anyway exposed to
> > > > application. i.e whatever files in lib/librte_eal/common/include/arch/../
> > > > anyway exposed to application.
> > > 
> > > Ah yes, you are right!
> > > 
> > > > See last comment.
> > > > 
> > > > > 
> > > > > > +	if (!freq)
> > > > > > +		freq = get_tsc_freq();
> > > > > 
> > > > > The function get_tsc_freq is guessing the freq with OS-specific method.
> > > > > 
> > > > > >  	if (!freq)
> > > > > >  		freq = estimate_tsc_freq();
> > > > > 
> > > > > The function estimate_tsc_freq is doing an estimation based on sleep().
> > > > > 
> > > > > At the end, the most accurate frequency is saved in eal_tsc_resolution_hz
> > > > > and can be retrieved with rte_get_tsc_hz().
> > > > > I don't understand why rte_rdtsc_arch_hz() is also exported to the apps.
> > > > > 
> > > > > TSC and HPET timer sources are wrapped in rte_get_timer_hz() in the
> > > > > Similarly we can get the current timer with rte_get_timer_cycles().
> > > > > In the case of TSC, it calls rte_get_tsc_cycles() which is an alias
> > > > > of rte_rdtsc().
> > > > > Some code is still using directly rte_rdtsc().
> > > > > There is also rte_rdtsc_precise which adds a memory barrier.
> > > > > 
> > > > > The real question is what is the right abstraction for the application?
> > > > > Do we want the fastest timer? the CPU timer? a precise timer?
> > > > > 
> > > > > I would like to see a real discussion on this topic, in order of building
> > > > > a new timer API which would alias the old one for some time.
> > > > 
> > > > I guess, we may need to see to how abstract vmware TSC support also in
> > > > proper way
> > > 
> > > Yes
> > > 
> > > > > If you don't want to bother with all these questions, I suggest to not
> > > > > export the new function rte_rdtsc_arch_hz() and rename it to tsc_arch_hz.
> > > > 
> > > > If I understand it correctly, You would like to create a header file 
> > > > in lib/librte_eal/common/include/arch/../ which should not be exported and change
> > > > the name to tsc_arch_hz.
> > > 
> > > I had not think about the way to do this.
> > > What about having internal headers in lib/librte_eal/common/arch/ ?
> > > 
> > 
> > Yes, this area needs cleanup, but I also think that this patchset (and
> > follow-on patch for x86-specific implementation) does not make things
> > significantly worse than they are now, while also giving significant
> > benefits for users both in terms of improved clock accuracy and reduced
> > startup time (due to not needing sleep). Therefore I'd like to see this
> > merged into 17.11 as a definite improvement, even if it does not "fix" the
> > bigger issues of poor naming etc. I think this is important enough an
> > improvement that we need to see it in the LTS release.
> 
> I agree Bruce.
> My initial point was to avoid exporting a new function with a wrong name.
> It would be better to find a way to avoid this export.
> If it cannot be done in 17.11 timeframe, we can at least add the
> experimental tag.

Ah, ok. That sounds fine.

/Bruce

Re: [dpdk-dev] [PATCH v2 2/5] eal/ppc64: define architecture specific rdtsc hz

[Thomas Monjalon]

22/09/2017 10:25, Gowrishankar:
> From: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> 
> In ppc_64, rte_rdtsc() returns timebase register value which increments
> at independent timebase frequency and hence not related to lcore cpu
> frequency to derive TSC hz. Hence, we stick with master lcore frequency.
> 
> CC: Chao Zhu <chaozhu@linux.vnet.ibm.com>
> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> Signed-off-by: Gowrishankar Muthukrishnan <gowrishankar.m@linux.vnet.ibm.com>
[...]
> --- a/lib/librte_eal/common/include/arch/ppc_64/rte_cycles.h
> +++ b/lib/librte_eal/common/include/arch/ppc_64/rte_cycles.h
> @@ -38,9 +38,13 @@
>  #endif
>  
>  #include "generic/rte_cycles.h"
> +#include "../../lib/librte_eal/common/eal_filesystem.h"

rte_cycles.h is an installed header.
eal_filesystem.h is not exported.
It cannot work.

[dpdk-dev] [PATCH 0/4] improve tsc frequency calibration

[Thomas Monjalon]

v3 changes:
- implement in .c files instead of exporting an inline arch function
- rename arch function from rte_rdtsc_arch_hz to get_tsc_freq_arch
- integrate x86 implementation in the series
- fix private EAL include in PPC implementation (not tested)


From: Gowrishankar Muthukrishnan <gowrishankar.m@linux.vnet.ibm.com>

Some architecture like armv8 provides an architecture specific function
to get the rdtsc frequency. The existing rdtsc calibration scheme uses
OS serivce like sleep(1) to calibrate the frequency which may not
produce the accurate result. Introducing an architecture specific hook
to get the rdtsc frequency if architecture provides it. If not, use the
exiting the calibrate scheme to get the rdtsc frequency.

Jerin Jacob (3):
  timer: honor arch-specific TSC frequency query
  eal/armv8: implement arch-specific TSC freq query
  eal/ppc64: implement arch-specific TSC freq query

Sergio Gonzalez Monroy (1):
  eal/x86: implement arch-specific TSC freq query

 lib/librte_eal/bsdapp/eal/Makefile                 |   1 +
 lib/librte_eal/common/arch/arm/rte_cycles.c        |  45 +++++++
 .../ppc_64/rte_cycles.c}                           |  78 ++---------
 .../{eal_common_timer.c => arch/x86/rte_cycles.c}  | 143 ++++++++++++++-------
 lib/librte_eal/common/eal_common_timer.c           |   5 +-
 lib/librte_eal/common/eal_private.h                |  11 ++
 lib/librte_eal/linuxapp/eal/Makefile               |   1 +
 7 files changed, 170 insertions(+), 114 deletions(-)
 create mode 100644 lib/librte_eal/common/arch/arm/rte_cycles.c
 copy lib/librte_eal/common/{eal_common_timer.c => arch/ppc_64/rte_cycles.c} (50%)
 copy lib/librte_eal/common/{eal_common_timer.c => arch/x86/rte_cycles.c} (50%)

-- 
2.14.1

[dpdk-dev] [PATCH 1/4] timer: honor arch-specific TSC frequency query

[Thomas Monjalon]

From: Jerin Jacob <jerin.jacob@caviumnetworks.com>

When calibrating the TSC frequency, first, probe the architecture specific
function. If not available, use the existing calibrate scheme.

Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 lib/librte_eal/bsdapp/eal/Makefile             |  1 +
 lib/librte_eal/common/arch/arm/rte_cycles.c    |  7 +++++++
 lib/librte_eal/common/arch/ppc_64/rte_cycles.c |  7 +++++++
 lib/librte_eal/common/arch/x86/rte_cycles.c    |  7 +++++++
 lib/librte_eal/common/eal_common_timer.c       |  5 ++++-
 lib/librte_eal/common/eal_private.h            | 11 +++++++++++
 lib/librte_eal/linuxapp/eal/Makefile           |  1 +
 7 files changed, 38 insertions(+), 1 deletion(-)
 create mode 100644 lib/librte_eal/common/arch/arm/rte_cycles.c
 create mode 100644 lib/librte_eal/common/arch/ppc_64/rte_cycles.c
 create mode 100644 lib/librte_eal/common/arch/x86/rte_cycles.c

diff --git a/lib/librte_eal/bsdapp/eal/Makefile b/lib/librte_eal/bsdapp/eal/Makefile
index 317a75ebc..b42306bc3 100644
--- a/lib/librte_eal/bsdapp/eal/Makefile
+++ b/lib/librte_eal/bsdapp/eal/Makefile
@@ -92,6 +92,7 @@ SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += rte_service.c
 # from arch dir
 SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += rte_cpuflags.c
 SRCS-$(CONFIG_RTE_ARCH_X86) += rte_spinlock.c
+SRCS-y += rte_cycles.c
 
 CFLAGS_eal_common_cpuflags.o := $(CPUFLAGS_LIST)
 
diff --git a/lib/librte_eal/common/arch/arm/rte_cycles.c b/lib/librte_eal/common/arch/arm/rte_cycles.c
new file mode 100644
index 000000000..851fd0255
--- /dev/null
+++ b/lib/librte_eal/common/arch/arm/rte_cycles.c
@@ -0,0 +1,7 @@
+#include "eal_private.h"
+
+uint64_t
+get_tsc_freq_arch(void)
+{
+	return 0;
+}
diff --git a/lib/librte_eal/common/arch/ppc_64/rte_cycles.c b/lib/librte_eal/common/arch/ppc_64/rte_cycles.c
new file mode 100644
index 000000000..851fd0255
--- /dev/null
+++ b/lib/librte_eal/common/arch/ppc_64/rte_cycles.c
@@ -0,0 +1,7 @@
+#include "eal_private.h"
+
+uint64_t
+get_tsc_freq_arch(void)
+{
+	return 0;
+}
diff --git a/lib/librte_eal/common/arch/x86/rte_cycles.c b/lib/librte_eal/common/arch/x86/rte_cycles.c
new file mode 100644
index 000000000..851fd0255
--- /dev/null
+++ b/lib/librte_eal/common/arch/x86/rte_cycles.c
@@ -0,0 +1,7 @@
+#include "eal_private.h"
+
+uint64_t
+get_tsc_freq_arch(void)
+{
+	return 0;
+}
diff --git a/lib/librte_eal/common/eal_common_timer.c b/lib/librte_eal/common/eal_common_timer.c
index ed0b16d05..5980b294e 100644
--- a/lib/librte_eal/common/eal_common_timer.c
+++ b/lib/librte_eal/common/eal_common_timer.c
@@ -80,8 +80,11 @@ estimate_tsc_freq(void)
 void
 set_tsc_freq(void)
 {
-	uint64_t freq = get_tsc_freq();
+	uint64_t freq;
 
+	freq = get_tsc_freq_arch();
+	if (!freq)
+		freq = get_tsc_freq();
 	if (!freq)
 		freq = estimate_tsc_freq();
 
diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
index 597d82e44..6e0f85def 100644
--- a/lib/librte_eal/common/eal_private.h
+++ b/lib/librte_eal/common/eal_private.h
@@ -314,6 +314,17 @@ void set_tsc_freq(void);
  */
 uint64_t get_tsc_freq(void);
 
+/**
+ * Get TSC frequency if the architecture supports.
+ *
+ * This function is private to the EAL.
+ *
+ * @return
+ *   The number of TSC cycles in one second.
+ *   Returns zero if the architecture support is not available.
+ */
+uint64_t get_tsc_freq_arch(void);
+
 /**
  * Prepare physical memory mapping
  * i.e. hugepages on Linux and
diff --git a/lib/librte_eal/linuxapp/eal/Makefile b/lib/librte_eal/linuxapp/eal/Makefile
index 21e0b4aad..140d1acfa 100644
--- a/lib/librte_eal/linuxapp/eal/Makefile
+++ b/lib/librte_eal/linuxapp/eal/Makefile
@@ -101,6 +101,7 @@ SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += rte_service.c
 # from arch dir
 SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += rte_cpuflags.c
 SRCS-$(CONFIG_RTE_ARCH_X86) += rte_spinlock.c
+SRCS-y += rte_cycles.c
 
 CFLAGS_eal_common_cpuflags.o := $(CPUFLAGS_LIST)
 
-- 
2.14.1

[dpdk-dev] [PATCH 2/4] eal/armv8: implement arch-specific TSC freq query

[Thomas Monjalon]

From: Jerin Jacob <jerin.jacob@caviumnetworks.com>

Use cntvct_el0 system register to get the system counter frequency.

If the system is configured with RTE_ARM_EAL_RDTSC_USE_PMU then
return 0(let the common code calibrate the tsc frequency).

Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
Acked-by: Jianbo Liu <jianbo.liu@linaro.org>
---
 lib/librte_eal/common/arch/arm/rte_cycles.c | 38 +++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/lib/librte_eal/common/arch/arm/rte_cycles.c b/lib/librte_eal/common/arch/arm/rte_cycles.c
index 851fd0255..3e31e5be1 100644
--- a/lib/librte_eal/common/arch/arm/rte_cycles.c
+++ b/lib/librte_eal/common/arch/arm/rte_cycles.c
@@ -1,7 +1,45 @@
+/*
+ *   BSD LICENSE
+ *
+ *   Copyright (C) Cavium, Inc. 2015.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Cavium, Inc nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
 #include "eal_private.h"
 
 uint64_t
 get_tsc_freq_arch(void)
 {
+#if defined RTE_ARCH_ARM64 && !defined RTE_ARM_EAL_RDTSC_USE_PMU
+	uint64_t freq;
+	asm volatile("mrs %0, cntfrq_el0" : "=r" (freq));
+	return freq;
+#else
 	return 0;
+#endif
 }
-- 
2.14.1

[dpdk-dev] [PATCH 3/4] eal/ppc64: implement arch-specific TSC freq query

[Thomas Monjalon]

From: Jerin Jacob <jerin.jacob@caviumnetworks.com>

In ppc_64, rte_rdtsc() returns timebase register value which increments
at independent timebase frequency and hence not related to lcore cpu
frequency to derive TSC hz. Hence, we stick with master lcore frequency.

Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
Signed-off-by: Gowrishankar Muthukrishnan <gowrishankar.m@linux.vnet.ibm.com>
---
 lib/librte_eal/common/arch/ppc_64/rte_cycles.c | 47 +++++++++++++++++++++++++-
 1 file changed, 46 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/arch/ppc_64/rte_cycles.c b/lib/librte_eal/common/arch/ppc_64/rte_cycles.c
index 851fd0255..69a9f7475 100644
--- a/lib/librte_eal/common/arch/ppc_64/rte_cycles.c
+++ b/lib/librte_eal/common/arch/ppc_64/rte_cycles.c
@@ -1,7 +1,52 @@
+/*
+ *   BSD LICENSE
+ *
+ *   Copyright (C) IBM Corporation 2014.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of IBM Corporation nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include <rte_lcore.h>
+#include <rte_log.h>
+#include "eal_filesystem.h"
 #include "eal_private.h"
 
+static const char sys_cpu_dir[] = "/sys/devices/system/cpu";
+
 uint64_t
 get_tsc_freq_arch(void)
 {
-	return 0;
+	unsigned long cpu_hz;
+	char path[PATH_MAX];
+
+	snprintf(path, sizeof(path), "%s/cpu%d/cpufreq/cpuinfo_cur_freq",
+			sys_cpu_dir, rte_get_master_lcore());
+	if (eal_parse_sysfs_value(path, &cpu_hz) < 0)
+		RTE_LOG(WARNING, EAL, "Unable to parse %s\n", path);
+
+	return cpu_hz*1000;
 }
-- 
2.14.1

[dpdk-dev] [PATCH 4/4] eal/x86: implement arch-specific TSC freq query

[Thomas Monjalon]

From: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>

First, try to use CPUID Time Stamp Counter and Nominal Core Crystal
Clock Information Leaf to determine the tsc hz on platforms that
supports it (does not require privileged user).

If the CPUID leaf is not available, then try to determine the tsc hz by
reading the MSR 0xCE (requires privileged user).

Default to the tsc hz estimation if both methods fail.

Signed-off-by: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>
Acked-by: Harry van Haaren <harry.van.haaren@intel.com>
Tested-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/librte_eal/common/arch/x86/rte_cycles.c | 142 +++++++++++++++++++++++++++-
 1 file changed, 141 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/arch/x86/rte_cycles.c b/lib/librte_eal/common/arch/x86/rte_cycles.c
index 851fd0255..1049d6567 100644
--- a/lib/librte_eal/common/arch/x86/rte_cycles.c
+++ b/lib/librte_eal/common/arch/x86/rte_cycles.c
@@ -1,7 +1,147 @@
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2017 Intel Corporation. All rights reserved.
+ *   All rights reserved.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Intel Corporation nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include <fcntl.h>
+#include <unistd.h>
+#include <cpuid.h>
+
 #include "eal_private.h"
 
+static unsigned int
+rte_cpu_get_model(uint32_t fam_mod_step)
+{
+	uint32_t family, model, ext_model;
+
+	family = (fam_mod_step >> 8) & 0xf;
+	model = (fam_mod_step >> 4) & 0xf;
+
+	if (family == 6 || family == 15) {
+		ext_model = (fam_mod_step >> 16) & 0xf;
+		model += (ext_model << 4);
+	}
+
+	return model;
+}
+
+static int32_t
+rdmsr(int msr, uint64_t *val)
+{
+#ifdef RTE_EXEC_ENV_LINUXAPP
+	int fd;
+	int ret;
+
+	fd = open("/dev/cpu/0/msr", O_RDONLY);
+	if (fd < 0)
+		return fd;
+
+	ret = pread(fd, val, sizeof(uint64_t), msr);
+
+	close(fd);
+
+	return ret;
+#else
+	return -1;
+#endif
+}
+
+static uint32_t
+check_model_wsm_nhm(uint8_t model)
+{
+	switch (model) {
+	/* Westmere */
+	case 0x25:
+	case 0x2C:
+	case 0x2F:
+	/* Nehalem */
+	case 0x1E:
+	case 0x1F:
+	case 0x1A:
+	case 0x2E:
+		return 1;
+	}
+
+	return 0;
+}
+
+static uint32_t
+check_model_gdm_dnv(uint8_t model)
+{
+	switch (model) {
+	/* Goldmont */
+	case 0x5C:
+	/* Denverton */
+	case 0x5F:
+		return 1;
+	}
+
+	return 0;
+}
+
 uint64_t
 get_tsc_freq_arch(void)
 {
-	return 0;
+	uint64_t tsc_hz = 0;
+	uint32_t a, b, c, d, maxleaf;
+	uint8_t mult, model;
+	int32_t ret;
+
+	/*
+	 * Time Stamp Counter and Nominal Core Crystal Clock
+	 * Information Leaf
+	 */
+	maxleaf = __get_cpuid_max(0, NULL);
+
+	if (maxleaf >= 0x15) {
+		__cpuid(0x15, a, b, c, d);
+
+		/* EBX : TSC/Crystal ratio, ECX : Crystal Hz */
+		if (b && c)
+			return c * (b / a);
+	}
+
+	__cpuid(0x1, a, b, c, d);
+	model = rte_cpu_get_model(a);
+
+	if (check_model_wsm_nhm(model))
+		mult = 133;
+	else if ((c & bit_AVX) || check_model_gdm_dnv(model))
+		mult = 100;
+	else
+		return 0;
+
+	ret = rdmsr(0xCE, &tsc_hz);
+	if (ret < 0)
+		return 0;
+
+	return ((tsc_hz >> 8) & 0xff) * mult * 1E6;
 }
-- 
2.14.1

Re: [dpdk-dev] [PATCH 0/4] improve tsc frequency calibration

[Jerin Jacob]

-----Original Message-----
> Date: Fri, 13 Oct 2017 02:02:43 +0200
> From: Thomas Monjalon <thomas@monjalon.net>
> To: gowrishankar.m@linux.vnet.ibm.com, jerin.jacob@caviumnetworks.com,
>  jianbo.liu@linaro.org, sergio.gonzalez.monroy@intel.com,
>  bruce.richardson@intel.com
> Cc: dev@dpdk.org
> Subject: [PATCH 0/4] improve tsc frequency calibration
> X-Mailer: git-send-email 2.14.1
> 
> v3 changes:
> - implement in .c files instead of exporting an inline arch function
> - rename arch function from rte_rdtsc_arch_hz to get_tsc_freq_arch
> - integrate x86 implementation in the series
> - fix private EAL include in PPC implementation (not tested)

Thanks Thomas.

Tested on a arm64 machine.

Tested-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>

> 
> 
> From: Gowrishankar Muthukrishnan <gowrishankar.m@linux.vnet.ibm.com>
> 
> Some architecture like armv8 provides an architecture specific function
> to get the rdtsc frequency. The existing rdtsc calibration scheme uses
> OS serivce like sleep(1) to calibrate the frequency which may not
> produce the accurate result. Introducing an architecture specific hook
> to get the rdtsc frequency if architecture provides it. If not, use the
> exiting the calibrate scheme to get the rdtsc frequency.
> 
> Jerin Jacob (3):
>   timer: honor arch-specific TSC frequency query
>   eal/armv8: implement arch-specific TSC freq query
>   eal/ppc64: implement arch-specific TSC freq query
> 
> Sergio Gonzalez Monroy (1):
>   eal/x86: implement arch-specific TSC freq query
> 
>  lib/librte_eal/bsdapp/eal/Makefile                 |   1 +
>  lib/librte_eal/common/arch/arm/rte_cycles.c        |  45 +++++++
>  .../ppc_64/rte_cycles.c}                           |  78 ++---------
>  .../{eal_common_timer.c => arch/x86/rte_cycles.c}  | 143 ++++++++++++++-------
>  lib/librte_eal/common/eal_common_timer.c           |   5 +-
>  lib/librte_eal/common/eal_private.h                |  11 ++
>  lib/librte_eal/linuxapp/eal/Makefile               |   1 +
>  7 files changed, 170 insertions(+), 114 deletions(-)
>  create mode 100644 lib/librte_eal/common/arch/arm/rte_cycles.c
>  copy lib/librte_eal/common/{eal_common_timer.c => arch/ppc_64/rte_cycles.c} (50%)
>  copy lib/librte_eal/common/{eal_common_timer.c => arch/x86/rte_cycles.c} (50%)
> 
> -- 
> 2.14.1
> 

Re: [dpdk-dev] [PATCH 4/4] eal/x86: implement arch-specific TSC freq query

[Stephen Hemminger]

On Fri, 13 Oct 2017 02:02:47 +0200
Thomas Monjalon <thomas@monjalon.net> wrote:

> +static uint32_t
> +check_model_wsm_nhm(uint8_t model)
> +{
> +	switch (model) {
> +	/* Westmere */
> +	case 0x25:
> +	case 0x2C:
> +	case 0x2F:
> +	/* Nehalem */
> +	case 0x1E:
> +	case 0x1F:
> +	case 0x1A:
> +	case 0x2E:
> +		return 1;
> +	}
> +
> +	return 0;
> +}

How about a table rather than switch?
Also bool rather than int?

Re: [dpdk-dev] [PATCH 4/4] eal/x86: implement arch-specific TSC freq query

[Thomas Monjalon]

13/10/2017 05:47, Stephen Hemminger:
> On Fri, 13 Oct 2017 02:02:47 +0200
> Thomas Monjalon <thomas@monjalon.net> wrote:
> 
> > +static uint32_t
> > +check_model_wsm_nhm(uint8_t model)
> > +{
> > +	switch (model) {
> > +	/* Westmere */
> > +	case 0x25:
> > +	case 0x2C:
> > +	case 0x2F:
> > +	/* Nehalem */
> > +	case 0x1E:
> > +	case 0x1F:
> > +	case 0x1A:
> > +	case 0x2E:
> > +		return 1;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> How about a table rather than switch?
> Also bool rather than int?

It is a matter of taste :)
I plan to push it as is. It can be changed later if desired.

Re: [dpdk-dev] [PATCH 0/4] improve tsc frequency calibration

[Thomas Monjalon]

> > v3 changes:
> > - implement in .c files instead of exporting an inline arch function
> > - rename arch function from rte_rdtsc_arch_hz to get_tsc_freq_arch
> > - integrate x86 implementation in the series
> > - fix private EAL include in PPC implementation (not tested)
> 
> Thanks Thomas.
> 
> Tested on a arm64 machine.
> 
> Tested-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>

Applied

Re: [dpdk-dev] [PATCH 4/4] eal/x86: implement arch-specific TSC freq query

[Stephen Hemminger]

On Fri, 13 Oct 2017 09:30:43 +0200
Thomas Monjalon <thomas@monjalon.net> wrote:

> 13/10/2017 05:47, Stephen Hemminger:
> > On Fri, 13 Oct 2017 02:02:47 +0200
> > Thomas Monjalon <thomas@monjalon.net> wrote:
> >   
> > > +static uint32_t
> > > +check_model_wsm_nhm(uint8_t model)
> > > +{
> > > +	switch (model) {
> > > +	/* Westmere */
> > > +	case 0x25:
> > > +	case 0x2C:
> > > +	case 0x2F:
> > > +	/* Nehalem */
> > > +	case 0x1E:
> > > +	case 0x1F:
> > > +	case 0x1A:
> > > +	case 0x2E:
> > > +		return 1;
> > > +	}
> > > +
> > > +	return 0;
> > > +}  
> > 
> > How about a table rather than switch?
> > Also bool rather than int?  
> 
> It is a matter of taste :)
> I plan to push it as is. It can be changed later if desired.

Also using #define's rather than magic constants would help.

Re: [dpdk-dev] [PATCH 4/4] eal/x86: implement arch-specific TSC freq query

[Bruce Richardson]

On Fri, Oct 13, 2017 at 08:13:06AM -0700, Stephen Hemminger wrote:
> On Fri, 13 Oct 2017 09:30:43 +0200
> Thomas Monjalon <thomas@monjalon.net> wrote:
> 
> > 13/10/2017 05:47, Stephen Hemminger:
> > > On Fri, 13 Oct 2017 02:02:47 +0200
> > > Thomas Monjalon <thomas@monjalon.net> wrote:
> > >   
> > > > +static uint32_t
> > > > +check_model_wsm_nhm(uint8_t model)
> > > > +{
> > > > +	switch (model) {
> > > > +	/* Westmere */
> > > > +	case 0x25:
> > > > +	case 0x2C:
> > > > +	case 0x2F:
> > > > +	/* Nehalem */
> > > > +	case 0x1E:
> > > > +	case 0x1F:
> > > > +	case 0x1A:
> > > > +	case 0x2E:
> > > > +		return 1;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}  
> > > 
> > > How about a table rather than switch?
> > > Also bool rather than int?  
> > 
> > It is a matter of taste :)
> > I plan to push it as is. It can be changed later if desired.
> 
> Also using #define's rather than magic constants would help.

Assuming that we could just come up with more meaningful names that help
more than just have a comment indicating what uarch's each set of
numbers is for. :-)
For me, I don't think #defines are needed to enhance readability here.