| Message ID | 20200618165354.87787-7-harry.van.haaren@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | DPCLS Subtable ISA Optimization | expand |
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
> -----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 --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(
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(+)