From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id DBC4A37B0 for ; Mon, 4 Sep 2017 12:24:12 +0200 (CEST) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 04 Sep 2017 03:24:11 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,474,1498546800"; d="scan'208";a="1214448238" Received: from bricha3-mobl3.ger.corp.intel.com ([10.237.221.24]) by fmsmga002.fm.intel.com with SMTP; 04 Sep 2017 03:24:08 -0700 Received: by (sSMTP sendmail emulation); Mon, 04 Sep 2017 11:24:08 +0100 Date: Mon, 4 Sep 2017 11:24:07 +0100 From: Bruce Richardson To: "Van Haaren, Harry" Cc: "Gonzalez Monroy, Sergio" , "dev@dpdk.org" , "Ananyev, Konstantin" Message-ID: <20170904102407.GA16984@bricha3-MOBL3.ger.corp.intel.com> References: <20170823150027.70565-1-sergio.gonzalez.monroy@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Organization: Intel Research and =?iso-8859-1?Q?De=ACvel?= =?iso-8859-1?Q?opment?= Ireland Ltd. User-Agent: Mutt/1.8.3 (2017-05-23) Subject: Re: [dpdk-dev] [PATCH] eal/x86: implement x86 specific tsc hz 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: Mon, 04 Sep 2017 10:24:13 -0000 On Mon, Sep 04, 2017 at 10:38:08AM +0100, Van Haaren, Harry wrote: > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Sergio Gonzalez Monroy > > Sent: Wednesday, August 23, 2017 4:00 PM > > To: dev@dpdk.org > > Cc: Ananyev, Konstantin ; Richardson, Bruce > > > > Subject: [dpdk-dev] [PATCH] eal/x86: implement x86 specific tsc hz > > > > 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 priviledge user). > > Checkpatch notifies me that "privilege" is spelled wrong (once above, once below). > > > If the CPUID leaf is not available, then try to determine the tsc hz by > > reading the MSR 0xCE (requires priviledge user). > > > > Default to the tsc hz estimation if both methods fail. > > > > Signed-off-by: Sergio Gonzalez Monroy > > --- > > > > DEPENDS on Jerin's patch: > > http://dpdk.org/dev/patchwork/patch/27526/ > > > > lib/librte_eal/common/arch/x86/rte_cycles.c | 142 +++++++++++++++++++++ > > .../common/include/arch/x86/rte_cycles.h | 7 +- > > lib/librte_eal/linuxapp/eal/Makefile | 1 + > > 3 files changed, 145 insertions(+), 5 deletions(-) > > create mode 100644 lib/librte_eal/common/arch/x86/rte_cycles.c > > > > 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 0000000..9336947 > > --- /dev/null > > +++ b/lib/librte_eal/common/arch/x86/rte_cycles.c > > @@ -0,0 +1,142 @@ > > +/*- > > + * 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 > > +#include > > +#include > > +#include > > + > > +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) > > +{ > > + int fd; > > + int ret = 0; > > Initialization of ret will always be overwritten by pread() below, so no need to initialize. > > > + > > + 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; > > +} > > + > > +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; > > + } > > DPDK coding standards say /* fallthrough */ comments required when falling through cases. > In this case I feel it would reduce readability, more than it improves it, but I recall > some recent gcc/clang prints warnings if no /* fallthrough */ comments exist.. opinions? > > Same for switch() below. > I see no warnings in this case with gcc 7.x. I don't think it counts as a fallthrough unless there is code after the label - i.e. multiple labels though technically fallthrough are treated as such by compiler. > > + > > + 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 > > +rte_rdtsc_arch_hz(void) > > +{ > > + 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 ((tsc_hz >> 8) & 0xff) * mult * 1E6; > > + > > + return 0; > > > if (!(ret < 0)) feels a little awkward, end of the function could be reworked to, which reads a little cleaner to me? > > + if (ret < 0) > + return 0; > + return ((tsc_hz >> 8) & 0xff) * mult * 1E6; > > > > +} > > 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 e2661e2..0db89dc 100644 > > --- a/lib/librte_eal/common/include/arch/x86/rte_cycles.h > > +++ b/lib/librte_eal/common/include/arch/x86/rte_cycles.h > > @@ -84,11 +84,8 @@ rte_rdtsc(void) > > * 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; > > -} > > +uint64_t > > +rte_rdtsc_arch_hz(void); > > > > static inline uint64_t > > rte_rdtsc_precise(void) > > diff --git a/lib/librte_eal/linuxapp/eal/Makefile b/lib/librte_eal/linuxapp/eal/Makefile > > index 90bca4d..9d44828 100644 > > --- a/lib/librte_eal/linuxapp/eal/Makefile > > +++ b/lib/librte_eal/linuxapp/eal/Makefile > > @@ -104,6 +104,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-$(CONFIG_RTE_ARCH_X86) += rte_cycles.c > > > > CFLAGS_eal_common_cpuflags.o := $(CPUFLAGS_LIST) > > > > -- > > 2.9.5 > > With above changes, and optionally /* fallthrough */ comments; > > Acked-by: Harry van Haaren > No need for fallthrough comments. On my system with "Intel(R) Xeon(R) Gold 6154 CPU @ 3.00GHz" the TSC frequency is now reported as 3,000,000 KHz, rather than 2,999,998 KHz as before. Tested-by: Bruce Richardson