[ovs-dev] [PATCH] dpif-netdev: Call cpuid for x86 isa availability.

Ilya Maximets i.maximets at ovn.org
Wed Nov 3 15:32:20 UTC 2021


On 10/12/21 21:49, David Marchand wrote:
> DPIF AVX512 optimisations currently rely on DPDK availability while
> they can be used without DPDK.
> Besides, checking for availability of some isa only has to be done once
> and won't change while a OVS process runs.
> 
> Resolve isa availability in constructors by using a simplified query
> based on cpuid API that comes from the compiler.
> 
> Note: this also fixes the check on BMI2 availability: DPDK had a bug
> for this isa, see https://git.dpdk.org/dpdk/commit/?id=aae3037ab1e0.
> 
> Suggested-by: Ilya Maximets <i.maximets at ovn.org>
> Signed-off-by: David Marchand <david.marchand at redhat.com>
> ---
>  lib/automake.mk                        |  2 +
>  lib/cpu.c                              | 68 ++++++++++++++++++++++++++
>  lib/cpu.h                              | 34 +++++++++++++
>  lib/dpdk-stub.c                        |  9 ----
>  lib/dpdk.c                             | 52 --------------------
>  lib/dpdk.h                             |  1 -
>  lib/dpif-netdev-avx512.c               |  5 +-
>  lib/dpif-netdev-extract-avx512.c       | 14 +++---
>  lib/dpif-netdev-lookup-avx512-gather.c |  7 +--
>  9 files changed, 118 insertions(+), 74 deletions(-)
>  create mode 100644 lib/cpu.c
>  create mode 100644 lib/cpu.h
> 

Hi, David.  Thanks ofr the patch!

I didn't check the actual values of cpuid bits, but the overall design
and implementation looks great!  This way non-dpdk setups (e.g. afxdp)
will be able to use optimized lookup functions.  This will also make
unit-testing easier.  This patch also eliminated string comparisons, which
is good to see.

Next step, I suppose, will be to move AVX512 tests out of the DPDK testsuite
to one of generic ones?

Harry, Cian, Amber, could you, please, review/test this out on your setup?

Best regards, Ilya Maximets.

> diff --git a/lib/automake.mk b/lib/automake.mk
> index 46f869a336..041ed96259 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -87,6 +87,8 @@ lib_libopenvswitch_la_SOURCES = \
>  	lib/conntrack.h \
>  	lib/coverage.c \
>  	lib/coverage.h \
> +	lib/cpu.c \
> +	lib/cpu.h \
>  	lib/crc32c.c \
>  	lib/crc32c.h \
>  	lib/csum.c \
> diff --git a/lib/cpu.c b/lib/cpu.c
> new file mode 100644
> index 0000000000..5794dd9430
> --- /dev/null
> +++ b/lib/cpu.c
> @@ -0,0 +1,68 @@
> +/*
> + * Copyright (c) 2021, Red Hat, Inc.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + *     http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include <config.h>
> +
> +#include "cpu.h"
> +#include "openvswitch/compiler.h"
> +
> +#ifdef __x86_64__
> +#include <cpuid.h>
> +#include <inttypes.h>
> +
> +enum x86_reg {
> +    EAX,
> +    EBX,
> +    ECX,
> +    EDX,
> +};
> +#define X86_LEAF_MASK 0x80000000
> +#define X86_EXT_FEATURES_LEAF 0x00000007
> +static bool x86_has_isa(uint32_t leaf, enum x86_reg reg, uint32_t bit)
> +{
> +    uint32_t maxleaf = __get_cpuid_max(leaf & X86_LEAF_MASK, NULL);
> +    uint32_t regs[4];
> +
> +    if (maxleaf < leaf) {
> +        return false;
> +    }
> +    __cpuid_count(leaf, 0, regs[EAX], regs[EBX], regs[ECX], regs[EDX]);
> +    return (regs[reg] & ((uint32_t)1 << bit)) != 0;
> +}
> +
> +static bool x86_isa[OVS_CPU_ISA_X86_LAST - OVS_CPU_ISA_X86_FIRST + 1];
> +#define X86_ISA(leaf, reg, bit, name) \
> +OVS_CONSTRUCTOR(cpu_isa_ ## name) { \
> +    x86_isa[name - OVS_CPU_ISA_X86_FIRST] = x86_has_isa(leaf, reg, bit); \
> +}
> +X86_ISA(X86_EXT_FEATURES_LEAF, EBX,  8, OVS_CPU_ISA_X86_BMI2)
> +X86_ISA(X86_EXT_FEATURES_LEAF, EBX, 16, OVS_CPU_ISA_X86_AVX512F)
> +X86_ISA(X86_EXT_FEATURES_LEAF, EBX, 30, OVS_CPU_ISA_X86_AVX512BW)
> +X86_ISA(X86_EXT_FEATURES_LEAF, ECX,  1, OVS_CPU_ISA_X86_AVX512VBMI)
> +X86_ISA(X86_EXT_FEATURES_LEAF, ECX, 14, OVS_CPU_ISA_X86_VPOPCNTDQ)
> +#endif
> +
> +bool
> +cpu_has_isa(enum ovs_cpu_isa isa OVS_UNUSED)
> +{
> +#ifdef __x86_64__
> +    if (isa >= OVS_CPU_ISA_X86_FIRST &&
> +        isa <= OVS_CPU_ISA_X86_LAST) {
> +        return x86_isa[isa - OVS_CPU_ISA_X86_FIRST];
> +    }
> +#endif
> +    return false;
> +}
> diff --git a/lib/cpu.h b/lib/cpu.h
> new file mode 100644
> index 0000000000..92897bb71c
> --- /dev/null
> +++ b/lib/cpu.h
> @@ -0,0 +1,34 @@
> +/*
> + * Copyright (c) 2021, Red Hat, Inc.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + *     http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#ifndef CPU_H
> +#define CPU_H 1
> +
> +#include <stdbool.h>
> +
> +enum ovs_cpu_isa {
> +    OVS_CPU_ISA_X86_FIRST,
> +    OVS_CPU_ISA_X86_BMI2 = OVS_CPU_ISA_X86_FIRST,
> +    OVS_CPU_ISA_X86_AVX512F,
> +    OVS_CPU_ISA_X86_AVX512BW,
> +    OVS_CPU_ISA_X86_AVX512VBMI,
> +    OVS_CPU_ISA_X86_VPOPCNTDQ,
> +    OVS_CPU_ISA_X86_LAST = OVS_CPU_ISA_X86_VPOPCNTDQ,
> +};
> +
> +bool cpu_has_isa(enum ovs_cpu_isa);
> +
> +#endif /* CPU_H */
> diff --git a/lib/dpdk-stub.c b/lib/dpdk-stub.c
> index fe24f9abdf..c332c217cb 100644
> --- a/lib/dpdk-stub.c
> +++ b/lib/dpdk-stub.c
> @@ -79,15 +79,6 @@ print_dpdk_version(void)
>  {
>  }
>  
> -bool
> -dpdk_get_cpu_has_isa(const char *arch OVS_UNUSED,
> -                     const char *feature OVS_UNUSED)
> -{
> -    VLOG_DBG_ONCE("DPDK not supported in this version of Open vSwitch, "
> -                  "cannot use CPU flag based optimizations");
> -    return false;
> -}
> -
>  void
>  dpdk_status(const struct ovsrec_open_vswitch *cfg)
>  {
> diff --git a/lib/dpdk.c b/lib/dpdk.c
> index b2ef31cd20..fe201bf29c 100644
> --- a/lib/dpdk.c
> +++ b/lib/dpdk.c
> @@ -585,58 +585,6 @@ print_dpdk_version(void)
>      puts(rte_version());
>  }
>  
> -/* Avoid calling rte_cpu_get_flag_enabled() excessively, by caching the
> - * result of the call for each CPU flag in a static variable. To avoid
> - * allocating large numbers of static variables, use a uint8 as a bitfield.
> - * Note the macro must only return if the ISA check is done and available.
> - */
> -#define ISA_CHECK_DONE_BIT (1 << 0)
> -#define ISA_AVAILABLE_BIT  (1 << 1)
> -
> -#define CHECK_CPU_FEATURE(feature, name_str, RTE_CPUFLAG)               \
> -    do {                                                                \
> -        if (strncmp(feature, name_str, strlen(name_str)) == 0) {        \
> -            static uint8_t isa_check_##RTE_CPUFLAG;                     \
> -            int check = isa_check_##RTE_CPUFLAG & ISA_CHECK_DONE_BIT;   \
> -            if (OVS_UNLIKELY(!check)) {                                 \
> -                int has_isa = rte_cpu_get_flag_enabled(RTE_CPUFLAG);    \
> -                VLOG_DBG("CPU flag %s, available %s\n",                 \
> -                         name_str, has_isa ? "yes" : "no");             \
> -                isa_check_##RTE_CPUFLAG = ISA_CHECK_DONE_BIT;           \
> -                if (has_isa) {                                          \
> -                    isa_check_##RTE_CPUFLAG |= ISA_AVAILABLE_BIT;       \
> -                }                                                       \
> -            }                                                           \
> -            if (isa_check_##RTE_CPUFLAG & ISA_AVAILABLE_BIT) {          \
> -                return true;                                            \
> -            } else {                                                    \
> -                return false;                                           \
> -            }                                                           \
> -        }                                                               \
> -    } while (0)
> -
> -bool
> -dpdk_get_cpu_has_isa(const char *arch, const char *feature)
> -{
> -    /* Ensure Arch is x86_64. */
> -    if (strncmp(arch, "x86_64", 6) != 0) {
> -        return false;
> -    }
> -
> -#if __x86_64__
> -    /* CPU flags only defined for the architecture that support it. */
> -    CHECK_CPU_FEATURE(feature, "avx512f", RTE_CPUFLAG_AVX512F);
> -    CHECK_CPU_FEATURE(feature, "avx512bw", RTE_CPUFLAG_AVX512BW);
> -    CHECK_CPU_FEATURE(feature, "avx512vbmi", RTE_CPUFLAG_AVX512VBMI);
> -    CHECK_CPU_FEATURE(feature, "avx512vpopcntdq", RTE_CPUFLAG_AVX512VPOPCNTDQ);
> -    CHECK_CPU_FEATURE(feature, "bmi2", RTE_CPUFLAG_BMI2);
> -#endif
> -
> -    VLOG_WARN("Unknown CPU arch,feature: %s,%s. Returning not supported.\n",
> -              arch, feature);
> -    return false;
> -}
> -
>  void
>  dpdk_status(const struct ovsrec_open_vswitch *cfg)
>  {
> diff --git a/lib/dpdk.h b/lib/dpdk.h
> index 445a51d065..2eb1aedbb0 100644
> --- a/lib/dpdk.h
> +++ b/lib/dpdk.h
> @@ -44,6 +44,5 @@ bool dpdk_per_port_memory(void);
>  bool dpdk_available(void);
>  void print_dpdk_version(void);
>  void dpdk_status(const struct ovsrec_open_vswitch *);
> -bool dpdk_get_cpu_has_isa(const char *arch, const char *feature);
>  
>  #endif /* dpdk.h */
> diff --git a/lib/dpif-netdev-avx512.c b/lib/dpif-netdev-avx512.c
> index 544d36903e..3f1e623f36 100644
> --- a/lib/dpif-netdev-avx512.c
> +++ b/lib/dpif-netdev-avx512.c
> @@ -20,6 +20,7 @@
>  
>  #include <config.h>
>  
> +#include "cpu.h"
>  #include "dpif-netdev.h"
>  #include "dpif-netdev-perf.h"
>  #include "dpif-netdev-private.h"
> @@ -61,8 +62,8 @@ struct dpif_userdata {
>  int32_t
>  dp_netdev_input_outer_avx512_probe(void)
>  {
> -    bool avx512f_available = dpdk_get_cpu_has_isa("x86_64", "avx512f");
> -    bool bmi2_available = dpdk_get_cpu_has_isa("x86_64", "bmi2");
> +    bool avx512f_available = cpu_has_isa(OVS_CPU_ISA_X86_AVX512F);
> +    bool bmi2_available = cpu_has_isa(OVS_CPU_ISA_X86_BMI2);
>  
>      if (!avx512f_available || !bmi2_available) {
>          return -ENOTSUP;
> diff --git a/lib/dpif-netdev-extract-avx512.c b/lib/dpif-netdev-extract-avx512.c
> index ec64419e38..feafba0c32 100644
> --- a/lib/dpif-netdev-extract-avx512.c
> +++ b/lib/dpif-netdev-extract-avx512.c
> @@ -42,8 +42,8 @@
>  #include <stdint.h>
>  #include <string.h>
>  
> +#include "cpu.h"
>  #include "flow.h"
> -#include "dpdk.h"
>  
>  #include "dpif-netdev-private-dpcls.h"
>  #include "dpif-netdev-private-extract.h"
> @@ -589,21 +589,21 @@ DECLARE_MFEX_FUNC(dot1q_ip_tcp, PROFILE_ETH_VLAN_IPV4_TCP)
>  static int32_t
>  avx512_isa_probe(uint32_t needs_vbmi)
>  {
> -    static const char *isa_required[] = {
> -        "avx512f",
> -        "avx512bw",
> -        "bmi2",
> +    static enum ovs_cpu_isa isa_required[] = {
> +        OVS_CPU_ISA_X86_AVX512F,
> +        OVS_CPU_ISA_X86_AVX512BW,
> +        OVS_CPU_ISA_X86_BMI2,
>      };
>  
>      int32_t ret = 0;
>      for (uint32_t i = 0; i < ARRAY_SIZE(isa_required); i++) {
> -        if (!dpdk_get_cpu_has_isa("x86_64", isa_required[i])) {
> +        if (!cpu_has_isa(isa_required[i])) {
>              ret = -ENOTSUP;
>          }
>      }
>  
>      if (needs_vbmi) {
> -        if (!dpdk_get_cpu_has_isa("x86_64", "avx512vbmi")) {
> +        if (!cpu_has_isa(OVS_CPU_ISA_X86_AVX512VBMI)) {
>              ret = -ENOTSUP;
>          }
>      }
> diff --git a/lib/dpif-netdev-lookup-avx512-gather.c b/lib/dpif-netdev-lookup-avx512-gather.c
> index 072831e96a..914c9e2ede 100644
> --- a/lib/dpif-netdev-lookup-avx512-gather.c
> +++ b/lib/dpif-netdev-lookup-avx512-gather.c
> @@ -19,6 +19,7 @@
>  
>  #include <config.h>
>  
> +#include "cpu.h"
>  #include "dpif-netdev.h"
>  #include "dpif-netdev-lookup.h"
>  #include "cmap.h"
> @@ -398,13 +399,13 @@ dpcls_subtable_avx512_gather_probe(uint32_t u0_bits, uint32_t u1_bits)
>  {
>      dpcls_subtable_lookup_func f = NULL;
>  
> -    int avx512f_available = dpdk_get_cpu_has_isa("x86_64", "avx512f");
> -    int bmi2_available = dpdk_get_cpu_has_isa("x86_64", "bmi2");
> +    int avx512f_available = cpu_has_isa(OVS_CPU_ISA_X86_AVX512F);
> +    int bmi2_available = cpu_has_isa(OVS_CPU_ISA_X86_BMI2);
>      if (!avx512f_available || !bmi2_available) {
>          return NULL;
>      }
>  
> -    int use_vpop = dpdk_get_cpu_has_isa("x86_64", "avx512vpopcntdq");
> +    int use_vpop = cpu_has_isa(OVS_CPU_ISA_X86_VPOPCNTDQ);
>  
>      CHECK_LOOKUP_FUNCTION(9, 4, use_vpop);
>      CHECK_LOOKUP_FUNCTION(9, 1, use_vpop);
> 



More information about the dev mailing list