| Message ID | 20220624072959.240183-1-david.marchand@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] dpif-netdev: Refactor AVX512 runtime checks. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On Fri, Jun 24, 2022 at 9:30 AM David Marchand <david.marchand@redhat.com> wrote: > > As described in the bugzilla below, cpu_has_isa code may be compiled > with some AVX512 instructions in it, because cpu.c is built as part of > the libopenvswitchavx512. > This is a problem when this function (supposed to probe for AVX512 > instructions availability) is invoked from generic OVS code, on older > CPUs that don't support them. > > For the same reason, dpcls_subtable_avx512_gather_probe, > dp_netdev_input_outer_avx512_probe, mfex_avx512_probe and > mfex_avx512_vbmi_probe are potential runtime bombs and can't either be > built as part of libopenvswitchavx512. > > Move cpu.c to be part of the "normal" libopenvswitch. > And move other helpers in generic OVS code. > > Note: > - dpcls_subtable_avx512_gather_probe is split in two, because it also > needs to do its own magic, > - while moving those helpers, prefer direct calls to cpu_has_isa and > avoid cast to intermediate integer variables when a simple boolean > is enough, > > Fixes: b366fa2f4947 ("dpif-netdev: Call cpuid for x86 isa availability.") Thinking again, there is probably more Fixes: lines to add. I'll update for v2 if this current patch passes the CI.
On 6/24/22 09:29, David Marchand wrote: > As described in the bugzilla below, cpu_has_isa code may be compiled > with some AVX512 instructions in it, because cpu.c is built as part of > the libopenvswitchavx512. > This is a problem when this function (supposed to probe for AVX512 > instructions availability) is invoked from generic OVS code, on older > CPUs that don't support them. > > For the same reason, dpcls_subtable_avx512_gather_probe, > dp_netdev_input_outer_avx512_probe, mfex_avx512_probe and > mfex_avx512_vbmi_probe are potential runtime bombs and can't either be > built as part of libopenvswitchavx512. > > Move cpu.c to be part of the "normal" libopenvswitch. > And move other helpers in generic OVS code. > > Note: > - dpcls_subtable_avx512_gather_probe is split in two, because it also > needs to do its own magic, > - while moving those helpers, prefer direct calls to cpu_has_isa and > avoid cast to intermediate integer variables when a simple boolean > is enough, > > Fixes: b366fa2f4947 ("dpif-netdev: Call cpuid for x86 isa availability.") > Reported-at: https://bugzilla.redhat.com/2100393 > Reported-by: Ales Musil <amusil@redhat.com> > Signed-off-by: David Marchand <david.marchand@redhat.com> > --- Thanks, David and Ales. The change looks good to me in general. Cian, Sunil, could you, please, take a look? The issue is causing failures in upstream OpenStack CI. We'll also need to backport this change to stable branches. Best regards, Ilya Maximets.
> -----Original Message----- > From: Ilya Maximets <i.maximets@ovn.org> > Sent: Tuesday, June 28, 2022 1:07 AM > To: David Marchand <david.marchand@redhat.com>; dev@openvswitch.org > Cc: i.maximets@ovn.org; amusil@redhat.com; Van Haaren, Harry > <harry.van.haaren@intel.com>; Ferriter, Cian <cian.ferriter@intel.com>; > Pai G, Sunil <sunil.pai.g@intel.com> > Subject: Re: [PATCH] dpif-netdev: Refactor AVX512 runtime checks. > > On 6/24/22 09:29, David Marchand wrote: > > As described in the bugzilla below, cpu_has_isa code may be compiled > > with some AVX512 instructions in it, because cpu.c is built as part of > > the libopenvswitchavx512. > > This is a problem when this function (supposed to probe for AVX512 > > instructions availability) is invoked from generic OVS code, on older > > CPUs that don't support them. > > > > For the same reason, dpcls_subtable_avx512_gather_probe, > > dp_netdev_input_outer_avx512_probe, mfex_avx512_probe and > > mfex_avx512_vbmi_probe are potential runtime bombs and can't either be > > built as part of libopenvswitchavx512. > > > > Move cpu.c to be part of the "normal" libopenvswitch. > > And move other helpers in generic OVS code. > > > > Note: > > - dpcls_subtable_avx512_gather_probe is split in two, because it also > > needs to do its own magic, > > - while moving those helpers, prefer direct calls to cpu_has_isa and > > avoid cast to intermediate integer variables when a simple boolean > > is enough, > > > > Fixes: b366fa2f4947 ("dpif-netdev: Call cpuid for x86 isa > > availability.") > > Reported-at: https://bugzilla.redhat.com/2100393 > > Reported-by: Ales Musil <amusil@redhat.com> > > Signed-off-by: David Marchand <david.marchand@redhat.com> > > --- > > Thanks, David and Ales. The change looks good to me in general. > > Cian, Sunil, could you, please, take a look? > > The issue is causing failures in upstream OpenStack CI. > We'll also need to backport this change to stable branches. > > Best regards, Ilya Maximets. The patch looks good to me. I'm happy to ack. Acked-by: Sunil Pai G <sunil.pai.g@intel.com>
diff --git a/lib/automake.mk b/lib/automake.mk index cb50578eb7..3b9e775d4e 100644 --- a/lib/automake.mk +++ b/lib/automake.mk @@ -36,8 +36,6 @@ lib_libopenvswitchavx512_la_CFLAGS = \ -fPIC \ $(AM_CFLAGS) lib_libopenvswitchavx512_la_SOURCES = \ - lib/cpu.c \ - lib/cpu.h \ lib/dpif-netdev-avx512.c if HAVE_AVX512BW lib_libopenvswitchavx512_la_CFLAGS += \ @@ -92,6 +90,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/dpif-netdev-avx512.c b/lib/dpif-netdev-avx512.c index 11d9a00052..82a4138184 100644 --- a/lib/dpif-netdev-avx512.c +++ b/lib/dpif-netdev-avx512.c @@ -20,7 +20,6 @@ #include <config.h> -#include "cpu.h" #include "dpif-netdev.h" #include "dpif-netdev-perf.h" #include "dpif-netdev-private.h" @@ -59,19 +58,6 @@ struct dpif_userdata { struct pkt_flow_meta pkt_meta[NETDEV_MAX_BURST]; }; -int32_t -dp_netdev_input_outer_avx512_probe(void) -{ - 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; - } - - return 0; -} - int32_t dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread *pmd, struct dp_packet_batch *packets, diff --git a/lib/dpif-netdev-extract-avx512.c b/lib/dpif-netdev-extract-avx512.c index 12271be17c..e7fac4131f 100644 --- a/lib/dpif-netdev-extract-avx512.c +++ b/lib/dpif-netdev-extract-avx512.c @@ -42,7 +42,6 @@ #include <stdint.h> #include <string.h> -#include "cpu.h" #include "flow.h" #include "dpif-netdev-private-dpcls.h" @@ -686,47 +685,5 @@ DECLARE_MFEX_FUNC(ip_udp, PROFILE_ETH_IPV4_UDP) DECLARE_MFEX_FUNC(ip_tcp, PROFILE_ETH_IPV4_TCP) DECLARE_MFEX_FUNC(dot1q_ip_udp, PROFILE_ETH_VLAN_IPV4_UDP) DECLARE_MFEX_FUNC(dot1q_ip_tcp, PROFILE_ETH_VLAN_IPV4_TCP) - - -static int32_t -avx512_isa_probe(uint32_t needs_vbmi) -{ - 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 (!cpu_has_isa(isa_required[i])) { - ret = -ENOTSUP; - } - } - - if (needs_vbmi) { - if (!cpu_has_isa(OVS_CPU_ISA_X86_AVX512VBMI)) { - ret = -ENOTSUP; - } - } - - return ret; -} - -/* Probe functions to check ISA requirements. */ -int32_t -mfex_avx512_probe(void) -{ - const uint32_t needs_vbmi = 0; - return avx512_isa_probe(needs_vbmi); -} - -int32_t -mfex_avx512_vbmi_probe(void) -{ - const uint32_t needs_vbmi = 1; - return avx512_isa_probe(needs_vbmi); -} - #endif /* __CHECKER__ */ #endif /* __x86_64__ */ diff --git a/lib/dpif-netdev-lookup-avx512-gather.c b/lib/dpif-netdev-lookup-avx512-gather.c index 1e86be2072..6e8bdc092b 100644 --- a/lib/dpif-netdev-lookup-avx512-gather.c +++ b/lib/dpif-netdev-lookup-avx512-gather.c @@ -23,7 +23,6 @@ #include "dpif-netdev-lookup.h" #include "cmap.h" -#include "cpu.h" #include "flow.h" #include "pvector.h" #include "openvswitch/vlog.h" @@ -413,18 +412,11 @@ dpcls_avx512_gather_mf_any(struct dpcls_subtable *subtable, uint32_t keys_map, } dpcls_subtable_lookup_func -dpcls_subtable_avx512_gather_probe(uint32_t u0_bits, uint32_t u1_bits) +dpcls_subtable_avx512_gather_probe__(uint32_t u0_bits, uint32_t u1_bits, + bool use_vpop) { dpcls_subtable_lookup_func f = NULL; - 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 = cpu_has_isa(OVS_CPU_ISA_X86_VPOPCNTDQ); - CHECK_LOOKUP_FUNCTION(9, 4, use_vpop); CHECK_LOOKUP_FUNCTION(9, 1, use_vpop); CHECK_LOOKUP_FUNCTION(5, 3, use_vpop); diff --git a/lib/dpif-netdev-lookup.c b/lib/dpif-netdev-lookup.c index 34ce12695f..6bcfb8ba81 100644 --- a/lib/dpif-netdev-lookup.c +++ b/lib/dpif-netdev-lookup.c @@ -18,10 +18,26 @@ #include <errno.h> #include "dpif-netdev-lookup.h" +#include "cpu.h" #include "openvswitch/vlog.h" VLOG_DEFINE_THIS_MODULE(dpif_netdev_lookup); +#if (__x86_64__ && HAVE_AVX512F && HAVE_LD_AVX512_GOOD && HAVE_AVX512BW \ + && __SSE4_2__) +static dpcls_subtable_lookup_func +dpcls_subtable_avx512_gather_probe(uint32_t u0_bits, uint32_t u1_bits) +{ + if (!cpu_has_isa(OVS_CPU_ISA_X86_AVX512F) + || !cpu_has_isa(OVS_CPU_ISA_X86_BMI2)) { + return NULL; + } + + return dpcls_subtable_avx512_gather_probe__(u0_bits, u1_bits, + cpu_has_isa(OVS_CPU_ISA_X86_VPOPCNTDQ)); +} +#endif + /* Actual list of implementations goes here */ static struct dpcls_subtable_lookup_info_t subtable_lookups[] = { /* The autovalidator implementation will not be used by default, it must diff --git a/lib/dpif-netdev-lookup.h b/lib/dpif-netdev-lookup.h index 7f124a46e9..91c419a632 100644 --- a/lib/dpif-netdev-lookup.h +++ b/lib/dpif-netdev-lookup.h @@ -45,7 +45,8 @@ dpcls_subtable_generic_probe(uint32_t u0_bit_count, uint32_t u1_bit_count); /* Probe function for AVX-512 gather implementation */ dpcls_subtable_lookup_func -dpcls_subtable_avx512_gather_probe(uint32_t u0_bit_cnt, uint32_t u1_bit_cnt); +dpcls_subtable_avx512_gather_probe__(uint32_t u0_bit_cnt, uint32_t u1_bit_cnt, + bool use_vpop); /* Subtable registration and iteration helpers */ diff --git a/lib/dpif-netdev-private-dpif.c b/lib/dpif-netdev-private-dpif.c index 84d4ec156e..5ae119a308 100644 --- a/lib/dpif-netdev-private-dpif.c +++ b/lib/dpif-netdev-private-dpif.c @@ -22,6 +22,7 @@ #include <errno.h> #include <string.h> +#include "cpu.h" #include "openvswitch/dynamic-string.h" #include "openvswitch/vlog.h" #include "util.h" @@ -33,6 +34,19 @@ enum dpif_netdev_impl_info_idx { DPIF_NETDEV_IMPL_AVX512 }; +#if (__x86_64__ && HAVE_AVX512F && HAVE_LD_AVX512_GOOD && __SSE4_2__) +static int32_t +dp_netdev_input_outer_avx512_probe(void) +{ + if (!cpu_has_isa(OVS_CPU_ISA_X86_AVX512F) + || !cpu_has_isa(OVS_CPU_ISA_X86_BMI2)) { + return -ENOTSUP; + } + + return 0; +} +#endif + /* Actual list of implementations goes here. */ static struct dpif_netdev_impl_info_t dpif_impls[] = { /* The default scalar C code implementation. */ diff --git a/lib/dpif-netdev-private-dpif.h b/lib/dpif-netdev-private-dpif.h index 0da639c55a..3e38630f53 100644 --- a/lib/dpif-netdev-private-dpif.h +++ b/lib/dpif-netdev-private-dpif.h @@ -67,10 +67,7 @@ dp_netdev_input(struct dp_netdev_pmd_thread *pmd, struct dp_packet_batch *packets, odp_port_t in_port); -/* AVX512 enabled DPIF implementation and probe functions. */ -int32_t -dp_netdev_input_outer_avx512_probe(void); - +/* AVX512 enabled DPIF implementation function. */ int32_t dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread *pmd, struct dp_packet_batch *packets, diff --git a/lib/dpif-netdev-private-extract.c b/lib/dpif-netdev-private-extract.c index 9ce4e09095..bd6bb8d94d 100644 --- a/lib/dpif-netdev-private-extract.c +++ b/lib/dpif-netdev-private-extract.c @@ -19,6 +19,7 @@ #include <stdint.h> #include <string.h> +#include "cpu.h" #include "dp-packet.h" #include "dpif-netdev-private-dpcls.h" #include "dpif-netdev-private-extract.h" @@ -33,6 +34,46 @@ VLOG_DEFINE_THIS_MODULE(dpif_netdev_extract); /* Variable to hold the default MFEX implementation. */ static ATOMIC(miniflow_extract_func) default_mfex_func; +#if (__x86_64__ && HAVE_AVX512F && HAVE_LD_AVX512_GOOD && HAVE_AVX512BW \ + && __SSE4_2__) +static int32_t +avx512_isa_probe(bool needs_vbmi) +{ + static enum ovs_cpu_isa isa_required[] = { + OVS_CPU_ISA_X86_AVX512F, + OVS_CPU_ISA_X86_AVX512BW, + OVS_CPU_ISA_X86_BMI2, + }; + + for (uint32_t i = 0; i < ARRAY_SIZE(isa_required); i++) { + if (!cpu_has_isa(isa_required[i])) { + return -ENOTSUP; + } + } + + if (needs_vbmi && !cpu_has_isa(OVS_CPU_ISA_X86_AVX512VBMI)) { + return -ENOTSUP; + } + + return 0; +} + +/* Probe functions to check ISA requirements. */ +static int32_t +mfex_avx512_probe(void) +{ + return avx512_isa_probe(false); +} + +#if HAVE_AVX512VBMI +static int32_t +mfex_avx512_vbmi_probe(void) +{ + return avx512_isa_probe(true); +} +#endif +#endif + /* Implementations of available extract options and * the implementations are always in order of preference. */ diff --git a/lib/dpif-netdev-private-extract.h b/lib/dpif-netdev-private-extract.h index 0921261064..0b526e1998 100644 --- a/lib/dpif-netdev-private-extract.h +++ b/lib/dpif-netdev-private-extract.h @@ -191,10 +191,8 @@ mfex_study_traffic(struct dp_packet_batch *packets, int mfex_set_study_pkt_cnt(uint32_t pkt_cmp_count, const char *name); -/* AVX512 MFEX Probe and Implementations functions. */ +/* AVX512 MFEX Implementation functions. */ #ifdef __x86_64__ -int32_t mfex_avx512_probe(void); -int32_t mfex_avx512_vbmi_probe(void); #define DECLARE_AVX512_MFEX_PROTOTYPE(name) \ uint32_t \
As described in the bugzilla below, cpu_has_isa code may be compiled with some AVX512 instructions in it, because cpu.c is built as part of the libopenvswitchavx512. This is a problem when this function (supposed to probe for AVX512 instructions availability) is invoked from generic OVS code, on older CPUs that don't support them. For the same reason, dpcls_subtable_avx512_gather_probe, dp_netdev_input_outer_avx512_probe, mfex_avx512_probe and mfex_avx512_vbmi_probe are potential runtime bombs and can't either be built as part of libopenvswitchavx512. Move cpu.c to be part of the "normal" libopenvswitch. And move other helpers in generic OVS code. Note: - dpcls_subtable_avx512_gather_probe is split in two, because it also needs to do its own magic, - while moving those helpers, prefer direct calls to cpu_has_isa and avoid cast to intermediate integer variables when a simple boolean is enough, Fixes: b366fa2f4947 ("dpif-netdev: Call cpuid for x86 isa availability.") Reported-at: https://bugzilla.redhat.com/2100393 Reported-by: Ales Musil <amusil@redhat.com> Signed-off-by: David Marchand <david.marchand@redhat.com> --- lib/automake.mk | 4 +-- lib/dpif-netdev-avx512.c | 14 --------- lib/dpif-netdev-extract-avx512.c | 43 -------------------------- lib/dpif-netdev-lookup-avx512-gather.c | 12 ++----- lib/dpif-netdev-lookup.c | 16 ++++++++++ lib/dpif-netdev-lookup.h | 3 +- lib/dpif-netdev-private-dpif.c | 14 +++++++++ lib/dpif-netdev-private-dpif.h | 5 +-- lib/dpif-netdev-private-extract.c | 41 ++++++++++++++++++++++++ lib/dpif-netdev-private-extract.h | 4 +-- 10 files changed, 79 insertions(+), 77 deletions(-)