From: David Marchand <david.marchand@redhat.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> Subject: Re: [dpdk-dev] [PATCH] common/mlx5: fix CPU detection for PCI relaxed ordering Date: Fri, 24 Jul 2020 16:53:47 +0200 Message-ID: <CAJFAV8xsF-OOZOop14-y3OqdGxn3xrwLkW2aVuTV5qy_3DOqqQ@mail.gmail.com> (raw) In-Reply-To: <20200719100735.2473014-1-thomas@monjalon.net> 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
prev parent reply other threads:[~2020-07-24 14:54 UTC|newest] Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-07-19 10:07 Thomas Monjalon 2020-07-19 10:11 ` Slava Ovsiienko 2020-07-19 10:56 ` Matan Azrad 2020-07-19 11:13 ` Thomas Monjalon 2020-07-19 11:41 ` Matan Azrad 2020-07-19 13:33 ` Thomas Monjalon 2020-07-24 15:43 ` Matan Azrad 2020-07-28 10:21 ` Thomas Monjalon 2020-07-28 10:38 ` Matan Azrad 2020-07-24 14:53 ` David Marchand [this message]
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=CAJFAV8xsF-OOZOop14-y3OqdGxn3xrwLkW2aVuTV5qy_3DOqqQ@mail.gmail.com \ --to=david.marchand@redhat.com \ --cc=dev@dpdk.org \ --cc=matan@mellanox.com \ --cc=mdr@ashroe.eu \ --cc=nhorman@tuxdriver.com \ --cc=rasland@mellanox.com \ --cc=shahafs@mellanox.com \ --cc=shirik@mellanox.com \ --cc=stable@dpdk.org \ --cc=thomas@monjalon.net \ --cc=viacheslavo@mellanox.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
DPDK patches and discussions This inbox may be cloned and mirrored by anyone: git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \ dev@dpdk.org public-inbox-index dev Example config snippet for mirrors. Newsgroup available over NNTP: nntp://inbox.dpdk.org/inbox.dpdk.dev AGPL code for this site: git clone https://public-inbox.org/public-inbox.git