diff mbox

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

Message ID 05e205ba2a617012e7dbea4b384a2a2280b36b63.1506058385.git.gowrishankar.m@linux.vnet.ibm.com (mailing list archive)
State Superseded, archived
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Gowrishankar Sept. 22, 2017, 8:25 a.m. UTC 
  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(-)

Comments

Thomas Monjalon Oct. 11, 2017, 5:36 p.m. UTC | #1 
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.
Jerin Jacob Oct. 11, 2017, 6:57 p.m. UTC | #2 
-----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.
Thomas Monjalon Oct. 11, 2017, 7:25 p.m. UTC | #3 
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/ ?
Bruce Richardson Oct. 12, 2017, 8:48 a.m. UTC | #4 
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
Thomas Monjalon Oct. 12, 2017, 10:12 a.m. UTC | #5 
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.
Jerin Jacob Oct. 12, 2017, 10:16 a.m. UTC | #6 
-----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

>
Bruce Richardson Oct. 12, 2017, 10:25 a.m. UTC | #7 
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
diff mbox

Patch

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();