From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 52A0A1B1AB for ; Thu, 12 Oct 2017 10:48:55 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 12 Oct 2017 01:48:53 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.43,365,1503385200"; d="scan'208";a="1204997083" Received: from bricha3-mobl3.ger.corp.intel.com ([10.237.221.37]) by fmsmga001.fm.intel.com with SMTP; 12 Oct 2017 01:48:50 -0700 Received: by (sSMTP sendmail emulation); Thu, 12 Oct 2017 09:48:49 +0100 Date: Thu, 12 Oct 2017 09:48:48 +0100 From: Bruce Richardson To: Thomas Monjalon Cc: Jerin Jacob , Gowrishankar , dev@dpdk.org, Chao Zhu , Konstantin Ananyev , viktorin@rehivetech.com, jianbo.liu@linaro.org Message-ID: <20171012084847.GA46424@bricha3-MOBL3.ger.corp.intel.com> References: <3939224.L4DEiYxvN2@xps> <20171011185719.GA19065@jerin> <1897496.fbWrZq1zZe@xps> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1897496.fbWrZq1zZe@xps> Organization: Intel Research and Development Ireland Ltd. User-Agent: Mutt/1.8.3 (2017-05-23) Subject: Re: [dpdk-dev] [PATCH v2 5/5] eal/timer: honor architecture specific rdtsc hz function X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 12 Oct 2017 08:48:55 -0000 On Wed, Oct 11, 2017 at 09:25:58PM +0200, Thomas Monjalon wrote: > 11/10/2017 20:57, Jerin Jacob: > > From: Thomas Monjalon > > > 22/09/2017 10:25, Gowrishankar: > > > > From: Jerin Jacob > > > > > > > > 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 > > > > > > 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