[dpdk-dev] [PATCH] eal/x86: implement x86 specific tsc hz
Bruce Richardson
bruce.richardson at intel.com
Mon Sep 4 12:24:07 CEST 2017
On Mon, Sep 04, 2017 at 10:38:08AM +0100, Van Haaren, Harry wrote:
> > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Sergio Gonzalez Monroy
> > Sent: Wednesday, August 23, 2017 4:00 PM
> > To: dev at dpdk.org
> > Cc: Ananyev, Konstantin <konstantin.ananyev at intel.com>; Richardson, Bruce
> > <bruce.richardson at intel.com>
> > 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 <sergio.gonzalez.monroy at intel.com>
> > ---
> >
> > 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 <fcntl.h>
> > +#include <unistd.h>
> > +#include <cpuid.h>
> > +#include <rte_cycles.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)
> > +{
> > + 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 <harry.van.haaren at intel.com>
>
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 <bruce.richardson at intel.com>
More information about the dev
mailing list