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 <[email protected]> > Signed-off-by: David Marchand <[email protected]> > --- > 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); > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
