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

[Bruce Richardson <bruce.richardson@intel.com>]

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