diff mbox series

[ovs-dev,v4,6/7] configure/m4: binutils avx512 configure time check.

Message ID 20200618165354.87787-7-harry.van.haaren@intel.com
State Superseded
Headers show
Series DPCLS Subtable ISA Optimization | expand

Commit Message

Harry van Haaren June 18, 2020, 4:53 p.m. UTC
This commit checks at configure time if the assembling in use
has a known bug in assembling AVX512 code. If this bug is present,
all AVX512 code is disabled.

Checking the version string of the binutils or assembler is not
a good method to detect the issue, as backported fixes would not
be reflected.

The method used here is also proposed for DPDK:
http://patches.dpdk.org/patch/71723/

Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>

---

v4:
- This patch is left seperate to ease reviews, as this is a logically
  seperate check. In v5 this code change will be merged with the commit
  that introduces the AVX512 code itself to ensure build consistency.
---
 configure.ac             |  1 +
 lib/dpif-netdev-lookup.c |  2 ++
 m4/openvswitch.m4        | 24 ++++++++++++++++++++++++
 3 files changed, 27 insertions(+)

Comments

William Tu June 27, 2020, 6:27 p.m. UTC | #1
On Thu, Jun 18, 2020 at 9:53 AM Harry van Haaren
<harry.van.haaren@intel.com> wrote:
>
> This commit checks at configure time if the assembling in use
> has a known bug in assembling AVX512 code. If this bug is present,
> all AVX512 code is disabled.
>
> Checking the version string of the binutils or assembler is not
> a good method to detect the issue, as backported fixes would not
> be reflected.
>
> The method used here is also proposed for DPDK:
> http://patches.dpdk.org/patch/71723/
>
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
>
> ---
>
> v4:
> - This patch is left seperate to ease reviews, as this is a logically
>   seperate check. In v5 this code change will be merged with the commit
>   that introduces the AVX512 code itself to ensure build consistency.
> ---
>  configure.ac             |  1 +
>  lib/dpif-netdev-lookup.c |  2 ++
>  m4/openvswitch.m4        | 24 ++++++++++++++++++++++++
>  3 files changed, 27 insertions(+)
>
> diff --git a/configure.ac b/configure.ac
> index 1367c868b..76d6de4e8 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -184,6 +184,7 @@ OVS_ENABLE_WERROR
>  OVS_ENABLE_SPARSE
>  OVS_CTAGS_IDENTIFIERS
>  OVS_CHECK_DPCLS_AUTOVALIDATOR
> +OVS_CHECK_BINUTILS_AVX512
>
>  AC_ARG_VAR(KARCH, [Kernel Architecture String])
>  AC_SUBST(KARCH)
> diff --git a/lib/dpif-netdev-lookup.c b/lib/dpif-netdev-lookup.c
> index 2c740399b..2a77814f2 100644
> --- a/lib/dpif-netdev-lookup.c
> +++ b/lib/dpif-netdev-lookup.c
> @@ -45,6 +45,7 @@ static struct dpcls_subtable_lookup_info_t subtable_lookups[] = {
>
>  #ifdef __x86_64__
>  #if HAVE_AVX512F
> +#if HAVE_LD_AVX512_GOOD
>  #ifdef __SSE4_2__
nit:
maybe combine these three conditions into one #if?

>      /* Only available on x86_64 bit builds with SSE 4.2 used for OVS core. */
>      { .prio = 0,
> @@ -59,6 +60,7 @@ static struct dpcls_subtable_lookup_info_t subtable_lookups[] = {
>  #endif
>  #endif
>  #endif
> +#endif
>  };

The rest looks good to me, thanks

William
Harry van Haaren June 30, 2020, 10:01 a.m. UTC | #2
> -----Original Message-----
> From: William Tu <u9012063@gmail.com>
> Sent: Saturday, June 27, 2020 7:27 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>
> Cc: ovs-dev <ovs-dev@openvswitch.org>; Stokes, Ian <ian.stokes@intel.com>;
> Ilya Maximets <i.maximets@ovn.org>; Federico Iezzi <fiezzi@redhat.com>
> Subject: Re: [PATCH v4 6/7] configure/m4: binutils avx512 configure time check.
> 
> On Thu, Jun 18, 2020 at 9:53 AM Harry van Haaren
> <harry.van.haaren@intel.com> wrote:
> >
> > This commit checks at configure time if the assembling in use
> > has a known bug in assembling AVX512 code. If this bug is present,
> > all AVX512 code is disabled.
> >
> > Checking the version string of the binutils or assembler is not
> > a good method to detect the issue, as backported fixes would not
> > be reflected.
> >
> > The method used here is also proposed for DPDK:
> > http://patches.dpdk.org/patch/71723/
> >
> > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> >
> > ---
> >
> > v4:
> > - This patch is left seperate to ease reviews, as this is a logically
> >   seperate check. In v5 this code change will be merged with the commit
> >   that introduces the AVX512 code itself to ensure build consistency.
> > ---
> >  configure.ac             |  1 +
> >  lib/dpif-netdev-lookup.c |  2 ++
> >  m4/openvswitch.m4        | 24 ++++++++++++++++++++++++
> >  3 files changed, 27 insertions(+)
> >
> > diff --git a/configure.ac b/configure.ac
> > index 1367c868b..76d6de4e8 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -184,6 +184,7 @@ OVS_ENABLE_WERROR
> >  OVS_ENABLE_SPARSE
> >  OVS_CTAGS_IDENTIFIERS
> >  OVS_CHECK_DPCLS_AUTOVALIDATOR
> > +OVS_CHECK_BINUTILS_AVX512
> >
> >  AC_ARG_VAR(KARCH, [Kernel Architecture String])
> >  AC_SUBST(KARCH)
> > diff --git a/lib/dpif-netdev-lookup.c b/lib/dpif-netdev-lookup.c
> > index 2c740399b..2a77814f2 100644
> > --- a/lib/dpif-netdev-lookup.c
> > +++ b/lib/dpif-netdev-lookup.c
> > @@ -45,6 +45,7 @@ static struct dpcls_subtable_lookup_info_t
> subtable_lookups[] = {
> >
> >  #ifdef __x86_64__
> >  #if HAVE_AVX512F
> > +#if HAVE_LD_AVX512_GOOD
> >  #ifdef __SSE4_2__
> nit:
> maybe combine these three conditions into one #if?

Will investigate.

> >      /* Only available on x86_64 bit builds with SSE 4.2 used for OVS core. */
> >      { .prio = 0,
> > @@ -59,6 +60,7 @@ static struct dpcls_subtable_lookup_info_t
> subtable_lookups[] = {
> >  #endif
> >  #endif
> >  #endif
> > +#endif
> >  };
> 
> The rest looks good to me, thanks
> 
> William
diff mbox series

Patch

diff --git a/configure.ac b/configure.ac
index 1367c868b..76d6de4e8 100644
--- a/configure.ac
+++ b/configure.ac
@@ -184,6 +184,7 @@  OVS_ENABLE_WERROR
 OVS_ENABLE_SPARSE
 OVS_CTAGS_IDENTIFIERS
 OVS_CHECK_DPCLS_AUTOVALIDATOR
+OVS_CHECK_BINUTILS_AVX512
 
 AC_ARG_VAR(KARCH, [Kernel Architecture String])
 AC_SUBST(KARCH)
diff --git a/lib/dpif-netdev-lookup.c b/lib/dpif-netdev-lookup.c
index 2c740399b..2a77814f2 100644
--- a/lib/dpif-netdev-lookup.c
+++ b/lib/dpif-netdev-lookup.c
@@ -45,6 +45,7 @@  static struct dpcls_subtable_lookup_info_t subtable_lookups[] = {
 
 #ifdef __x86_64__
 #if HAVE_AVX512F
+#if HAVE_LD_AVX512_GOOD
 #ifdef __SSE4_2__
     /* Only available on x86_64 bit builds with SSE 4.2 used for OVS core. */
     { .prio = 0,
@@ -59,6 +60,7 @@  static struct dpcls_subtable_lookup_info_t subtable_lookups[] = {
 #endif
 #endif
 #endif
+#endif
 };
 
 int32_t
diff --git a/m4/openvswitch.m4 b/m4/openvswitch.m4
index add3aabcc..442cf4cb0 100644
--- a/m4/openvswitch.m4
+++ b/m4/openvswitch.m4
@@ -404,6 +404,30 @@  AC_DEFUN([OVS_CHECK_SPHINX],
    AC_ARG_VAR([SPHINXBUILD])
    AM_CONDITIONAL([HAVE_SPHINX], [test "$SPHINXBUILD" != none])])
 
+dnl Checks for binutils/assembler known issue with AVX512.
+dnl Due to backports, we probe assembling a reproducer instaed of checking
+dnl binutils version string. More details, including ASM dumps and debug here:
+dnl   GCC: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90028
+dnl The checking of binutils funcationality instead of LD version is similar
+dnl to as how DPDK proposes to solve this issue:
+dnl   http://patches.dpdk.org/patch/71723/
+AC_DEFUN([OVS_CHECK_BINUTILS_AVX512],
+  [AC_CACHE_CHECK(
+    [binutils avx512 assembler checks passing],
+    [ovs_cv_binutils_avx512_good],
+    [dnl Assemble a short snippet to test for issue in "build-aux" dir:
+     OBJFILE=build-aux/binutils_avx512_check.o
+     GATHER_PARAMS='0x8(,%ymm1,1),%ymm0{%k2}'
+     echo "vpgatherqq $GATHER_PARAMS" | as --64 -o $OBJFILE -
+     if (objdump -d  --no-show-raw-insn $OBJFILE | grep -q $GATHER_PARAMS) >/dev/null 2>&1; then
+       ovs_cv_binutils_avx512_good=yes
+       CFLAGS="$CFLAGS -DHAVE_LD_AVX512_GOOD"
+     else
+       ovs_cv_binutils_avx512_good=no
+     fi])
+   AM_CONDITIONAL([HAVE_LD_AVX512_GOOD],
+                  [test "$ovs_cv_binutils_avx512_good" = yes])])
+
 dnl Checks for dot.
 AC_DEFUN([OVS_CHECK_DOT],
   [AC_CACHE_CHECK(