| Message ID | 20220704122700.54825-1-sunil.pai.g@intel.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v2] dpif-netdev: Simplify AVX512 build time checks to enhance readability. | 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 |
> -----Original Message----- > From: Pai G, Sunil <sunil.pai.g@intel.com> > Sent: Monday 4 July 2022 13:27 > To: dev@openvswitch.org > Cc: echaudro@redhat.com; Finn, Emma <emma.finn@intel.com>; Van Haaren, Harry > <harry.van.haaren@intel.com>; Ferriter, Cian <cian.ferriter@intel.com> > Subject: [PATCH v2] dpif-netdev: Simplify AVX512 build time checks to enhance readability. > > The preprocessor comparison string to check AVX512 capabilities are > lengthy and effecting user readability. Simpify this by aliasing the checks. > > Suggested-by: Eelco Chaudron <echaudro@redhat.com> > Signed-off-by: Sunil Pai G <sunil.pai.g@intel.com> > > --- > v2: rebase on master, added alias for DPCLS. > Remove the acks since there are changes introduced because of > rebase. > --- V2 changes make sense. There's are extra places where the build check is simplified because of "dpif-netdev: Refactor AVX512 runtime checks" change. It makes sense to add an alias for the DPCLS build time checks because it's used in more than one place now, where that wasn't the case for the v1. I ran the same set of tests that I usually run on these build time checks (build with GCC 4.8, 4.9, 5 and 9) to hit cases where the compiler supports the different subsets of AVX512 ISA. All LGTM. Acked-by: Cian Ferriter <cian.ferriter@intel.com>
> > -----Original Message----- > > From: Pai G, Sunil <sunil.pai.g@intel.com> > > Sent: Monday 4 July 2022 13:27 > > To: dev@openvswitch.org > > Cc: echaudro@redhat.com; Finn, Emma <emma.finn@intel.com>; Van Haaren, > > Harry <harry.van.haaren@intel.com>; Ferriter, Cian > > <cian.ferriter@intel.com> > > Subject: [PATCH v2] dpif-netdev: Simplify AVX512 build time checks to > enhance readability. > > > > The preprocessor comparison string to check AVX512 capabilities are > > lengthy and effecting user readability. Simpify this by aliasing the > checks. > > > > Suggested-by: Eelco Chaudron <echaudro@redhat.com> > > Signed-off-by: Sunil Pai G <sunil.pai.g@intel.com> > > > > --- > > v2: rebase on master, added alias for DPCLS. > > Remove the acks since there are changes introduced because of > > rebase. > > --- > > V2 changes make sense. There's are extra places where the build check is > simplified because of "dpif-netdev: Refactor AVX512 runtime checks" > change. > > It makes sense to add an alias for the DPCLS build time checks because > it's used in more than one place now, where that wasn't the case for the > v1. > > I ran the same set of tests that I usually run on these build time checks > (build with GCC 4.8, 4.9, 5 and 9) to hit cases where the compiler > supports the different subsets of AVX512 ISA. All LGTM. > > Acked-by: Cian Ferriter <cian.ferriter@intel.com> Cian, thanks for the review! Sending a friendly ping here to check for any comments/objections on this patch from maintainers. Thanks and regards Sunil
On 8/2/22 09:47, Pai G, Sunil wrote: >>> -----Original Message----- >>> From: Pai G, Sunil <sunil.pai.g@intel.com> >>> Sent: Monday 4 July 2022 13:27 >>> To: dev@openvswitch.org >>> Cc: echaudro@redhat.com; Finn, Emma <emma.finn@intel.com>; Van Haaren, >>> Harry <harry.van.haaren@intel.com>; Ferriter, Cian >>> <cian.ferriter@intel.com> >>> Subject: [PATCH v2] dpif-netdev: Simplify AVX512 build time checks to >> enhance readability. >>> >>> The preprocessor comparison string to check AVX512 capabilities are >>> lengthy and effecting user readability. Simpify this by aliasing the >> checks. >>> >>> Suggested-by: Eelco Chaudron <echaudro@redhat.com> >>> Signed-off-by: Sunil Pai G <sunil.pai.g@intel.com> >>> >>> --- >>> v2: rebase on master, added alias for DPCLS. >>> Remove the acks since there are changes introduced because of >>> rebase. >>> --- >> >> V2 changes make sense. There's are extra places where the build check is >> simplified because of "dpif-netdev: Refactor AVX512 runtime checks" >> change. >> >> It makes sense to add an alias for the DPCLS build time checks because >> it's used in more than one place now, where that wasn't the case for the >> v1. >> >> I ran the same set of tests that I usually run on these build time checks >> (build with GCC 4.8, 4.9, 5 and 9) to hit cases where the compiler >> supports the different subsets of AVX512 ISA. All LGTM. >> >> Acked-by: Cian Ferriter <cian.ferriter@intel.com> > > Cian, thanks for the review! > > Sending a friendly ping here to check for any comments/objections on this patch from maintainers. Hi, Sunil. I think, Ian wanted to apply the patch, we discussed that on a meeting last week. Ian, is it still on your radar? Best regards, Ilya Maximets.
On 8/2/22 22:32, Ilya Maximets wrote: > On 8/2/22 09:47, Pai G, Sunil wrote: >>>> -----Original Message----- >>>> From: Pai G, Sunil <sunil.pai.g@intel.com> >>>> Sent: Monday 4 July 2022 13:27 >>>> To: dev@openvswitch.org >>>> Cc: echaudro@redhat.com; Finn, Emma <emma.finn@intel.com>; Van Haaren, >>>> Harry <harry.van.haaren@intel.com>; Ferriter, Cian >>>> <cian.ferriter@intel.com> >>>> Subject: [PATCH v2] dpif-netdev: Simplify AVX512 build time checks to >>> enhance readability. >>>> >>>> The preprocessor comparison string to check AVX512 capabilities are >>>> lengthy and effecting user readability. Simpify this by aliasing the >>> checks. >>>> >>>> Suggested-by: Eelco Chaudron <echaudro@redhat.com> >>>> Signed-off-by: Sunil Pai G <sunil.pai.g@intel.com> >>>> >>>> --- >>>> v2: rebase on master, added alias for DPCLS. >>>> Remove the acks since there are changes introduced because of >>>> rebase. >>>> --- >>> >>> V2 changes make sense. There's are extra places where the build check is >>> simplified because of "dpif-netdev: Refactor AVX512 runtime checks" >>> change. >>> >>> It makes sense to add an alias for the DPCLS build time checks because >>> it's used in more than one place now, where that wasn't the case for the >>> v1. >>> >>> I ran the same set of tests that I usually run on these build time checks >>> (build with GCC 4.8, 4.9, 5 and 9) to hit cases where the compiler >>> supports the different subsets of AVX512 ISA. All LGTM. >>> >>> Acked-by: Cian Ferriter <cian.ferriter@intel.com> >> >> Cian, thanks for the review! >> >> Sending a friendly ping here to check for any comments/objections on this patch from maintainers. > > Hi, Sunil. I think, Ian wanted to apply the patch, we discussed > that on a meeting last week. It's probably just a PTO season. :) (Got a few OOO replies here) > > Ian, is it still on your radar? > > Best regards, Ilya Maximets.
<snipped> > >> Sending a friendly ping here to check for any comments/objections on > this patch from maintainers. > > > > Hi, Sunil. I think, Ian wanted to apply the patch, we discussed that > > on a meeting last week. > > It's probably just a PTO season. :) > (Got a few OOO replies here) Thanks Ilya 😊 I am informed that this patch is being considered for a backport to 2.17 to keep the codebase consistent. Just had a question on the same: do we also want to backport Cian's patches ("Build some AVX512 code on older compilers.") to 2.17 ? https://patchwork.ozlabs.org/project/openvswitch/list/?series=300610&archive=both&state=* Thanks and regards Sunil
On 8/3/22 13:50, Pai G, Sunil wrote: > > <snipped> > >>>> Sending a friendly ping here to check for any comments/objections on >> this patch from maintainers. >>> >>> Hi, Sunil. I think, Ian wanted to apply the patch, we discussed that >>> on a meeting last week. >> >> It's probably just a PTO season. :) >> (Got a few OOO replies here) > > Thanks Ilya 😊 > I am informed that this patch is being considered for a backport to 2.17 to keep the codebase consistent. > Just had a question on the same: do we also want to backport Cian's patches ("Build some AVX512 code on older compilers.") to 2.17 ? > https://patchwork.ozlabs.org/project/openvswitch/list/?series=300610&archive=both&state=* Current patch is purely cosmetic, AFAICT. But the patch set above contains functional changes that might affect code generation, so I'd not consider it for backporting unless it's necessary. Best regards, Ilya Maximets.
> > <snipped> > > > >>>> Sending a friendly ping here to check for any comments/objections > >>>> on > >> this patch from maintainers. > >>> > >>> Hi, Sunil. I think, Ian wanted to apply the patch, we discussed > >>> that on a meeting last week. > >> > >> It's probably just a PTO season. :) > >> (Got a few OOO replies here) > > > > Thanks Ilya 😊 > > I am informed that this patch is being considered for a backport to 2.17 > to keep the codebase consistent. > > Just had a question on the same: do we also want to backport Cian's > patches ("Build some AVX512 code on older compilers.") to 2.17 ? > > https://patchwork.ozlabs.org/project/openvswitch/list/?series=300610&a > > rchive=both&state=* > > Current patch is purely cosmetic, AFAICT. Agreed. > But the patch set above contains functional changes that might affect code generation, so I'd not > consider it for backporting unless it's necessary. > > Best regards, Ilya Maximets. Sure Ilya, that makes sense to me 😊 Thanks and regards Sunil
> > > The preprocessor comparison string to check AVX512 capabilities are > > > lengthy and effecting user readability. Simpify this by aliasing the > > checks. > > > > > > Suggested-by: Eelco Chaudron <echaudro@redhat.com> > > > Signed-off-by: Sunil Pai G <sunil.pai.g@intel.com> > > > > > > --- > > > v2: rebase on master, added alias for DPCLS. > > > Remove the acks since there are changes introduced because of > > > rebase. > > > --- > > > > V2 changes make sense. There's are extra places where the build check is > > simplified because of "dpif-netdev: Refactor AVX512 runtime checks" > > change. > > > > It makes sense to add an alias for the DPCLS build time checks because > > it's used in more than one place now, where that wasn't the case for the > > v1. > > > > I ran the same set of tests that I usually run on these build time checks > > (build with GCC 4.8, 4.9, 5 and 9) to hit cases where the compiler > > supports the different subsets of AVX512 ISA. All LGTM. > > > > Acked-by: Cian Ferriter <cian.ferriter@intel.com> > > Cian, thanks for the review! > > Sending a friendly ping here to check for any comments/objections on this > patch from maintainers. Thanks for the patch Sunil, applied to master, branch 3.0 and applied the backport to 2.17. Thanks Ian
diff --git a/lib/dpif-netdev-lookup.c b/lib/dpif-netdev-lookup.c index 6bcfb8ba8..4c1379aa5 100644 --- a/lib/dpif-netdev-lookup.c +++ b/lib/dpif-netdev-lookup.c @@ -22,9 +22,10 @@ #include "openvswitch/vlog.h" VLOG_DEFINE_THIS_MODULE(dpif_netdev_lookup); +#define DPCLS_IMPL_AVX512_CHECK (__x86_64__ && HAVE_AVX512F \ + && HAVE_LD_AVX512_GOOD && HAVE_AVX512BW && __SSE4_2__) -#if (__x86_64__ && HAVE_AVX512F && HAVE_LD_AVX512_GOOD && HAVE_AVX512BW \ - && __SSE4_2__) +#if DPCLS_IMPL_AVX512_CHECK static dpcls_subtable_lookup_func dpcls_subtable_avx512_gather_probe(uint32_t u0_bits, uint32_t u1_bits) { @@ -61,8 +62,7 @@ static struct dpcls_subtable_lookup_info_t subtable_lookups[] = { .name = "generic", .usage_cnt = ATOMIC_COUNT_INIT(0), }, -#if (__x86_64__ && HAVE_AVX512F && HAVE_LD_AVX512_GOOD && HAVE_AVX512BW \ - && __SSE4_2__) +#if DPCLS_IMPL_AVX512_CHECK /* Only available on x86_64 bit builds with SSE 4.2 used for OVS core. */ { .prio = 0, .probe = dpcls_subtable_avx512_gather_probe, diff --git a/lib/dpif-netdev-private-dpif.c b/lib/dpif-netdev-private-dpif.c index 5ae119a30..ef4cee2ba 100644 --- a/lib/dpif-netdev-private-dpif.c +++ b/lib/dpif-netdev-private-dpif.c @@ -28,13 +28,15 @@ #include "util.h" VLOG_DEFINE_THIS_MODULE(dpif_netdev_impl); +#define DPIF_NETDEV_IMPL_AVX512_CHECK (__x86_64__ && HAVE_AVX512F \ + && HAVE_LD_AVX512_GOOD && __SSE4_2__) enum dpif_netdev_impl_info_idx { DPIF_NETDEV_IMPL_SCALAR, DPIF_NETDEV_IMPL_AVX512 }; -#if (__x86_64__ && HAVE_AVX512F && HAVE_LD_AVX512_GOOD && __SSE4_2__) +#if DPIF_NETDEV_IMPL_AVX512_CHECK static int32_t dp_netdev_input_outer_avx512_probe(void) { @@ -54,7 +56,7 @@ static struct dpif_netdev_impl_info_t dpif_impls[] = { .probe = NULL, .name = "dpif_scalar", }, -#if (__x86_64__ && HAVE_AVX512F && HAVE_LD_AVX512_GOOD && __SSE4_2__) +#if DPIF_NETDEV_IMPL_AVX512_CHECK /* Only available on x86_64 bit builds with SSE 4.2 used for OVS core. */ [DPIF_NETDEV_IMPL_AVX512] = { .input_func = dp_netdev_input_outer_avx512, .probe = dp_netdev_input_outer_avx512_probe, @@ -73,7 +75,7 @@ dp_netdev_impl_get_default(void) int dpif_idx = DPIF_NETDEV_IMPL_SCALAR; /* Configure-time overriding to run test suite on all implementations. */ -#if (__x86_64__ && HAVE_AVX512F && HAVE_LD_AVX512_GOOD && __SSE4_2__) +#if DPIF_NETDEV_IMPL_AVX512_CHECK #ifdef DPIF_AVX512_DEFAULT dp_netdev_input_func_probe probe; diff --git a/lib/dpif-netdev-private-extract.c b/lib/dpif-netdev-private-extract.c index bd6bb8d94..61258a7ac 100644 --- a/lib/dpif-netdev-private-extract.c +++ b/lib/dpif-netdev-private-extract.c @@ -34,8 +34,7 @@ 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__) +#if MFEX_IMPL_AVX512_CHECK static int32_t avx512_isa_probe(bool needs_vbmi) { @@ -95,8 +94,7 @@ static struct dpif_miniflow_extract_impl mfex_impls[] = { .name = "study", }, /* Compile in implementations only if the compiler ISA checks pass. */ -#if (__x86_64__ && HAVE_AVX512F && HAVE_LD_AVX512_GOOD && HAVE_AVX512BW \ - && __SSE4_2__) +#if MFEX_IMPL_AVX512_CHECK #if HAVE_AVX512VBMI [MFEX_IMPL_VBMI_IPv4_UDP] = { .probe = mfex_avx512_vbmi_probe, diff --git a/lib/dpif-netdev-private-extract.h b/lib/dpif-netdev-private-extract.h index 0b526e199..2a3a91744 100644 --- a/lib/dpif-netdev-private-extract.h +++ b/lib/dpif-netdev-private-extract.h @@ -19,6 +19,9 @@ #include <sys/types.h> +#define MFEX_IMPL_AVX512_CHECK (__x86_64__ && HAVE_AVX512F \ + && HAVE_LD_AVX512_GOOD && HAVE_AVX512BW && __SSE4_2__) + /* Forward declarations. */ struct dp_packet; struct miniflow; @@ -81,8 +84,7 @@ enum dpif_miniflow_extract_impl_idx { MFEX_IMPL_AUTOVALIDATOR, MFEX_IMPL_SCALAR, MFEX_IMPL_STUDY, -#if (__x86_64__ && HAVE_AVX512F && HAVE_LD_AVX512_GOOD && HAVE_AVX512BW \ - && __SSE4_2__) +#if MFEX_IMPL_AVX512_CHECK #if HAVE_AVX512VBMI MFEX_IMPL_VBMI_IPv4_UDP, #endif @@ -108,9 +110,7 @@ extern struct ovs_mutex dp_netdev_mutex; /* Define a index which points to the first traffic optimized MFEX * option from the enum list else holds max value. */ -#if (__x86_64__ && HAVE_AVX512F && HAVE_LD_AVX512_GOOD && HAVE_AVX512BW \ - && __SSE4_2__) - +#if MFEX_IMPL_AVX512_CHECK #if HAVE_AVX512VBMI #define MFEX_IMPL_START_IDX MFEX_IMPL_VBMI_IPv4_UDP #else
The preprocessor comparison string to check AVX512 capabilities are lengthy and effecting user readability. Simpify this by aliasing the checks. Suggested-by: Eelco Chaudron <echaudro@redhat.com> Signed-off-by: Sunil Pai G <sunil.pai.g@intel.com> --- v2: rebase on master, added alias for DPCLS. Remove the acks since there are changes introduced because of rebase. --- lib/dpif-netdev-lookup.c | 8 ++++---- lib/dpif-netdev-private-dpif.c | 8 +++++--- lib/dpif-netdev-private-extract.c | 6 ++---- lib/dpif-netdev-private-extract.h | 10 +++++----- 4 files changed, 16 insertions(+), 16 deletions(-)