From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from dpdk.org (dpdk.org [92.243.14.124])
by inbox.dpdk.org (Postfix) with ESMTP id DF172A0527;
Fri, 24 Jul 2020 16:54:05 +0200 (CEST)
Received: from [92.243.14.124] (localhost [127.0.0.1])
by dpdk.org (Postfix) with ESMTP id D2B2B1C032;
Fri, 24 Jul 2020 16:54:03 +0200 (CEST)
Received: from us-smtp-delivery-1.mimecast.com (us-smtp-1.mimecast.com
[207.211.31.81]) by dpdk.org (Postfix) with ESMTP id 150F41C025
for <dev@dpdk.org>; Fri, 24 Jul 2020 16:54:01 +0200 (CEST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;
s=mimecast20190719; t=1595602441;
h=from:from:reply-to:subject:subject:date:date:message-id:message-id:
to:to:cc:cc:mime-version:mime-version:content-type:content-type:
in-reply-to:in-reply-to:references:references;
bh=yhaC+XX0m68DeIkFB/O40J+FU9NOECWN2VPUVU+H35Q=;
b=HWDWYOyH78alA3ybJKG23vtpHallmUG5DJeeDwzvJpHzQHNpZe7GZgaYHo91r26ROrEUL/
nivXStTSS9s8O4IoGxMc8bDOfR/58FuK4tpIyBqfnyfJa7J8aF+Mv7i4pHsKq372FyOgZs
gU0ZIHF+f2L+a5u5Hclt8kKkRv0l1KM=
Received: from mail-ua1-f69.google.com (mail-ua1-f69.google.com
[209.85.222.69]) (Using TLS) by relay.mimecast.com with ESMTP id
us-mta-321-jWRs2O0TPvKHtU8WUQhgcQ-1; Fri, 24 Jul 2020 10:53:59 -0400
X-MC-Unique: jWRs2O0TPvKHtU8WUQhgcQ-1
Received: by mail-ua1-f69.google.com with SMTP id h94so2749893uad.14
for <dev@dpdk.org>; Fri, 24 Jul 2020 07:53:59 -0700 (PDT)
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=1e100.net; s=20161025;
h=x-gm-message-state:mime-version:references:in-reply-to:from:date
:message-id:subject:to:cc;
bh=yhaC+XX0m68DeIkFB/O40J+FU9NOECWN2VPUVU+H35Q=;
b=iI4K8fnzY8OOQtXgTkkfaGPQcEmHdv77u0QSlwpX0X5+NDTWu8QsF3iNRf0jvlrx5h
T2i1uXNFNxQh40Z7/op+bT/tpDrJJ00JQv1W43fIHyj2Tl201PucXQTpYM4terhLSe2N
rR+KW64SkzcHTol0NjcnrKGxMLKarUzoilk/OO7DQvrFyyR7UfMUpcA+a1qw5mlBYT2I
ENKjVxx5BGYl/buHBIgcFQd/oTeLob4Z9+1l2OkAALoRGRmosn9qPvfH/n5GjEkDGcWp
JYcwj1dRByBrI9f9AUgSVv9t7YmDW8ibrWO8m56kBym2hx0qhFBFpBLWa8wgSuIoLFnX
Axqw==
X-Gm-Message-State: AOAM530IYXgwkBL2NbIM0kyyQgsRmoj2jVsk1t1Mf523M0FjceSm4Usp
CyyG3j3sIF3Ii5Qrq4XooF89IOWMjjnUSTU0XvXOWWRHWqLM3WBZk1b9T+ZumzG2kPzsHVuv/EM
AfBbFoRwDU6kd3ZmKa3Y=
X-Received: by 2002:ab0:5a72:: with SMTP id m47mr8169853uad.86.1595602439136;
Fri, 24 Jul 2020 07:53:59 -0700 (PDT)
X-Google-Smtp-Source: ABdhPJwkH5zny7NVJkrA+ROmWsfVzVYv5Mq1xeDDtUxnbideL+2Pz8ywgCE3wr3VdVkSp8dCTb/sWTSWNZuFBcOygAs=
X-Received: by 2002:ab0:5a72:: with SMTP id m47mr8169824uad.86.1595602438790;
Fri, 24 Jul 2020 07:53:58 -0700 (PDT)
MIME-Version: 1.0
References: <20200719100735.2473014-1-thomas@monjalon.net>
In-Reply-To: <20200719100735.2473014-1-thomas@monjalon.net>
From: David Marchand <david.marchand@redhat.com>
Date: Fri, 24 Jul 2020 16:53:47 +0200
Message-ID: <CAJFAV8xsF-OOZOop14-y3OqdGxn3xrwLkW2aVuTV5qy_3DOqqQ@mail.gmail.com>
To: Thomas Monjalon <thomas@monjalon.net>
Cc: dev <dev@dpdk.org>, Raslan <rasland@mellanox.com>, shirik@mellanox.com,
dpdk stable <stable@dpdk.org>, Matan Azrad <matan@mellanox.com>,
Shahaf Shuler <shahafs@mellanox.com>,
Viacheslav Ovsiienko <viacheslavo@mellanox.com>,
Ray Kinsella <mdr@ashroe.eu>, Neil Horman <nhorman@tuxdriver.com>
X-Mimecast-Spam-Score: 0
X-Mimecast-Originator: redhat.com
Content-Type: text/plain; charset="UTF-8"
Subject: Re: [dpdk-dev] [PATCH] common/mlx5: fix CPU detection for PCI
relaxed ordering
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>
On Sun, Jul 19, 2020 at 12:09 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> The detection of the CPU was done in a constructor and shared
> in a global variable.
>
> This variable may not be visible in the net PMD
> because it was not exported as part of the .map file.
> It is fixed by exporting a function, which is cleaner than a variable.
>
> By checking the CPU only at the first call of the function,
> doing the check in a constructor becomes useless.
> Note: the priority of the constructor was probably irrelevant.
>
> At the same time, the comments are reworded or dropped if useless.
>
> Fixes: 4c204fe5e5d2 ("common/mlx5: disable relaxed ordering in unsuitable CPUs")
> Cc: shirik@mellanox.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
> drivers/common/mlx5/linux/mlx5_common_verbs.c | 2 +-
> drivers/common/mlx5/mlx5_common.c | 53 ++++++++-----------
> drivers/common/mlx5/mlx5_common.h | 4 +-
> .../common/mlx5/rte_common_mlx5_version.map | 2 +
> drivers/net/mlx5/mlx5_flow_dv.c | 2 +-
> 5 files changed, 28 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/common/mlx5/linux/mlx5_common_verbs.c b/drivers/common/mlx5/linux/mlx5_common_verbs.c
> index a2fc7a36bd..31ac20fe09 100644
> --- a/drivers/common/mlx5/linux/mlx5_common_verbs.c
> +++ b/drivers/common/mlx5/linux/mlx5_common_verbs.c
> @@ -55,7 +55,7 @@ mlx5_common_verbs_reg_mr(void *pd, void *addr, size_t length,
> memset(pmd_mr, 0, sizeof(*pmd_mr));
> ibv_mr = mlx5_glue->reg_mr(pd, addr, length,
> IBV_ACCESS_LOCAL_WRITE |
> - (haswell_broadwell_cpu ? 0 :
> + (mlx5_cpu_is_haswell_broadwell() ? 0 :
> IBV_ACCESS_RELAXED_ORDERING));
> if (!ibv_mr)
> return -1;
> diff --git a/drivers/common/mlx5/mlx5_common.c b/drivers/common/mlx5/mlx5_common.c
> index 693e2c68c8..7232d5131d 100644
> --- a/drivers/common/mlx5/mlx5_common.c
> +++ b/drivers/common/mlx5/mlx5_common.c
> @@ -20,8 +20,6 @@ int mlx5_common_logtype;
> const struct mlx5_glue *mlx5_glue;
> #endif
>
> -uint8_t haswell_broadwell_cpu;
> -
> static int
> mlx5_class_check_handler(__rte_unused const char *key, const char *value,
> void *opaque)
> @@ -59,19 +57,8 @@ mlx5_class_get(struct rte_devargs *devargs)
> }
>
>
> -/* In case this is an x86_64 intel processor to check if
> - * we should use relaxed ordering.
> - */
> #ifdef RTE_ARCH_X86_64
> -/**
> - * This function returns processor identification and feature information
> - * into the registers.
> - *
> - * @param eax, ebx, ecx, edx
> - * Pointers to the registers that will hold cpu information.
> - * @param level
> - * The main category of information returned.
> - */
> +/* Processor identification and feature information filled in registers. */
Nit, no need for inline.
> static inline void mlx5_cpu_id(unsigned int level,
> unsigned int *eax, unsigned int *ebx,
> unsigned int *ecx, unsigned int *edx)
> @@ -97,17 +84,7 @@ RTE_INIT_PRIO(mlx5_glue_init, CLASS)
> mlx5_glue_constructor();
> }
>
> -/**
> - * This function is responsible of initializing the variable
> - * haswell_broadwell_cpu by checking if the cpu is intel
> - * and reading the data returned from mlx5_cpu_id().
> - * since haswell and broadwell cpus don't have improved performance
> - * when using relaxed ordering we want to check the cpu type before
> - * before deciding whether to enable RO or not.
> - * if the cpu is haswell or broadwell the variable will be set to 1
> - * otherwise it will be 0.
> - */
> -RTE_INIT_PRIO(mlx5_is_haswell_broadwell_cpu, LOG)
> +static bool mlx5_x86_is_haswell_broadwell(void)
> {
> #ifdef RTE_ARCH_X86_64
> unsigned int broadwell_models[4] = {0x3d, 0x47, 0x4F, 0x56};
> @@ -125,8 +102,7 @@ RTE_INIT_PRIO(mlx5_is_haswell_broadwell_cpu, LOG)
> vendor = ebx;
> max_level = eax;
> if (max_level < 1) {
> - haswell_broadwell_cpu = 0;
> - return;
> + return false;
> }
> mlx5_cpu_id(1, &eax, &ebx, &ecx, &edx);
> model = (eax >> 4) & 0x0f;
> @@ -140,18 +116,31 @@ RTE_INIT_PRIO(mlx5_is_haswell_broadwell_cpu, LOG)
> if (brand_id == 0 && family == 0x6) {
> for (i = 0; i < RTE_DIM(broadwell_models); i++)
> if (model == broadwell_models[i]) {
> - haswell_broadwell_cpu = 1;
> - return;
> + return true;
> }
> for (i = 0; i < RTE_DIM(haswell_models); i++)
> if (model == haswell_models[i]) {
> - haswell_broadwell_cpu = 1;
> - return;
> + return true;
> }
> }
> }
> #endif
> - haswell_broadwell_cpu = 0;
> + return false;
> +}
> +
> +/*
> + * Check if the CPU is Intel Haswell or Broadwell,
> + * because PCI relaxed ordering has no performance benefit with these CPUs.
> + */
> +bool mlx5_cpu_is_haswell_broadwell(void)
> +{
> + static bool haswell_broadwell_cpu;
> + static bool once = false;
> +
> + if (once)
> + return haswell_broadwell_cpu;
> + once = true;
> + return haswell_broadwell_cpu = mlx5_x86_is_haswell_broadwell();
> }
>
> /**
> diff --git a/drivers/common/mlx5/mlx5_common.h b/drivers/common/mlx5/mlx5_common.h
> index 2851507058..d453e0b3d8 100644
> --- a/drivers/common/mlx5/mlx5_common.h
> +++ b/drivers/common/mlx5/mlx5_common.h
> @@ -243,6 +243,9 @@ struct mlx5_klm {
>
> LIST_HEAD(mlx5_dbr_page_list, mlx5_devx_dbr_page);
>
> +__rte_internal
> +bool mlx5_cpu_is_haswell_broadwell(void);
> +
> __rte_internal
> enum mlx5_class mlx5_class_get(struct rte_devargs *devargs);
> __rte_internal
> @@ -255,6 +258,5 @@ int64_t mlx5_get_dbr(void *ctx, struct mlx5_dbr_page_list *head,
> __rte_internal
> int32_t mlx5_release_dbr(struct mlx5_dbr_page_list *head, uint32_t umem_id,
> uint64_t offset);
> -extern uint8_t haswell_broadwell_cpu;
>
> #endif /* RTE_PMD_MLX5_COMMON_H_ */
> diff --git a/drivers/common/mlx5/rte_common_mlx5_version.map b/drivers/common/mlx5/rte_common_mlx5_version.map
> index ae57ebdba5..501b9fff3b 100644
> --- a/drivers/common/mlx5/rte_common_mlx5_version.map
> +++ b/drivers/common/mlx5/rte_common_mlx5_version.map
> @@ -6,6 +6,8 @@ INTERNAL {
> mlx5_common_verbs_reg_mr;
> mlx5_common_verbs_dereg_mr;
>
> + mlx5_cpu_is_haswell_broadwell;
> +
> mlx5_create_mr_ext;
>
> mlx5_dev_to_pci_addr;
> diff --git a/drivers/net/mlx5/mlx5_flow_dv.c b/drivers/net/mlx5/mlx5_flow_dv.c
> index 8b5b6838fa..f1109ae095 100644
> --- a/drivers/net/mlx5/mlx5_flow_dv.c
> +++ b/drivers/net/mlx5/mlx5_flow_dv.c
> @@ -4201,7 +4201,7 @@ flow_dv_create_counter_stat_mem_mng(struct rte_eth_dev *dev, int raws_n)
> mkey_attr.klm_num = 0;
> if (priv->config.hca_attr.relaxed_ordering_write &&
> priv->config.hca_attr.relaxed_ordering_read &&
> - !haswell_broadwell_cpu)
> + !mlx5_cpu_is_haswell_broadwell())
> mkey_attr.relaxed_ordering = 1;
> mem_mng->dm = mlx5_devx_cmd_mkey_create(sh->ctx, &mkey_attr);
> if (!mem_mng->dm) {
> --
> 2.27.0
>
I ended up on this constructor hack while looking at the common code.
This patch makes sense.
Reviewed-by: David Marchand <david.marchand@redhat.com>
--
David Marchand