From: "Van Haaren, Harry" <harry.van.haaren@intel.com>
To: "Gonzalez Monroy, Sergio" <sergio.gonzalez.monroy@intel.com>,
"dev@dpdk.org" <dev@dpdk.org>
Cc: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
"Richardson, Bruce" <bruce.richardson@intel.com>
Subject: Re: [dpdk-dev] [PATCH] eal/x86: implement x86 specific tsc hz
Date: Mon, 4 Sep 2017 09:38:08 +0000 [thread overview]
Message-ID: <E923DB57A917B54B9182A2E928D00FA640C73D49@IRSMSX102.ger.corp.intel.com> (raw)
In-Reply-To: <20170823150027.70565-1-sergio.gonzalez.monroy@intel.com>
> 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 <konstantin.ananyev@intel.com>; Richardson, Bruce
> <bruce.richardson@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@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.
> +
> + 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@intel.com>
next prev parent reply other threads:[~2017-09-04 9:38 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-23 15:00 Sergio Gonzalez Monroy
2017-08-23 15:00 ` [dpdk-dev] [PATCH] eal/x86: use cpuid builtin Sergio Gonzalez Monroy
2017-10-04 23:39 ` Ferruh Yigit
2017-10-11 20:00 ` Thomas Monjalon
2017-09-04 9:38 ` Van Haaren, Harry [this message]
2017-09-04 10:24 ` [dpdk-dev] [PATCH] eal/x86: implement x86 specific tsc hz Bruce Richardson
2017-09-04 10:32 ` Bruce Richardson
2017-10-02 10:09 ` [dpdk-dev] [PATCH v2] " Sergio Gonzalez Monroy
2017-10-02 11:17 ` [dpdk-dev] [PATCH v3] " Sergio Gonzalez Monroy
2017-10-02 11:24 ` Jerin Jacob
2017-10-02 11:27 ` Sergio Gonzalez Monroy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=E923DB57A917B54B9182A2E928D00FA640C73D49@IRSMSX102.ger.corp.intel.com \
--to=harry.van.haaren@intel.com \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=konstantin.ananyev@intel.com \
--cc=sergio.gonzalez.monroy@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).