On Wed, Jun 29, 2022 at 9:32 AM David Marchand <[email protected]>
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: 352b6c7116cd ("dpif-lookup: add avx512 gather implementation.")
> Fixes: abb807e27dd4 ("dpif-netdev: Add command to switch dpif
> implementation.")
> Fixes: 250ceddcc2d0 ("dpif-netdev/mfex: Add AVX512 based optimized
> miniflow extract")
> Fixes: b366fa2f4947 ("dpif-netdev: Call cpuid for x86 isa availability.")
> Reported-at: https://bugzilla.redhat.com/2100393
> Reported-by: Ales Musil <[email protected]>
> Co-authored-by: Ales Musil <[email protected]>
> Signed-off-by: Ales Musil <[email protected]>
> Signed-off-by: David Marchand <[email protected]>
> Acked-by: Sunil Pai G <[email protected]>
> ---
> Changes since v1:
> - restored Ales as co-author,
> - added more Fixes: lines: backports go back to 2.14 and are not that
> difficult
> to do, but I can help if needed,
> - fixed one indent issue,
>
> ---
> 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(-)
>
> 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 f1919befd1..5029730292 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"
> @@ -690,47 +689,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..7d3d81151f 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..ac6d973173 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 ffd6ff907a..9027129617 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"
> @@ -35,6 +36,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 a70ab6a4d6..c0c14221b7 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 b2cd6779d7..2a3a91744e 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
> \
> --
> 2.36.1
>
>
Thank you!
Acked-by: Ales Musil <[email protected] <[email protected]>>
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA <https://www.redhat.com>
[email protected] IM: amusil
<https://red.ht/sig>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
