[ovs-dev] [PATCH v4 5/7] dpif-lookup: add avx512 gather implementation.

Van Haaren, Harry harry.van.haaren at intel.com
Tue Jun 30 10:00:57 UTC 2020


> -----Original Message-----
> From: William Tu <u9012063 at gmail.com>
> Sent: Saturday, June 27, 2020 7:27 PM
> To: Van Haaren, Harry <harry.van.haaren at intel.com>
> Cc: ovs-dev <ovs-dev at openvswitch.org>; Stokes, Ian <ian.stokes at intel.com>;
> Ilya Maximets <i.maximets at ovn.org>; Federico Iezzi <fiezzi at redhat.com>
> Subject: Re: [PATCH v4 5/7] dpif-lookup: add avx512 gather implementation.
> 
> On Thu, Jun 18, 2020 at 9:53 AM Harry van Haaren
> <harry.van.haaren at intel.com> wrote:
> >
> > This commit adds an AVX-512 dpcls lookup implementation.
> > It uses the AVX-512 SIMD ISA to perform multiple miniflow
> > operations in parallel.
> >
> > To run this implementation, the "avx512f" and "bmi2" ISAs are
> > required. These ISA checks are performed at runtime while
> > probing the subtable implementation. If a CPU does not provide
> > both "avx512f" and "bmi2", then this code does not execute.
> >
> > The avx512 code is built as a seperate static library, with added
> > CFLAGS to enable the required ISA features. By building only this
> > static library with avx512 enabled, it is ensured that the main OVS
> > core library is *not* using avx512, and that OVS continues to run
> > as before on CPUs that do not support avx512.
> >
> > The approach taken in this implementation is to use the
> > gather instruction to access the packet miniflow, allowing
> > any miniflow blocks to be loaded into an AVX-512 register.
> > This maximises the usefulness of the register, and hence this
> > implementation handles any subtable with up to miniflow 8 bits.
> >
> > Note that specialization of these avx512 lookup routines
> > still provides performance value, as the hashing of the
> > resulting data is performed in scalar code, and compile-time
> > loop unrolling occurs when specialized to miniflow bits.
> >
> > Signed-off-by: Harry van Haaren <harry.van.haaren at intel.com>
> >
> > ---
> >
> > v4:
> > - Remove TODO comment on prio-set command (was accidentally
> >   added to this commit in v3)
> > - Fixup v3 changlog to not include #warning comment (William Tu)
> > - Remove #define for debugging in lookup.h
> > - Fix builds on older gcc versions that don't support -mavx512f.
> >   Solution involves CC_CHECK and #ifdefs in code (OVS Robot, William Tu)
> >
> > v3:
> > - Improve function name for _any subtable lookup
> > - Use "" include not <> for immintrin.h
> > - Add checks for SSE42 instructions in core OVS for CRC32 based hashing
> >   If not available, disable AVX512 lookup implementation as it requires
> >   uses CRC32 for hashing, and the hashing algorithm must match core OVS.
> > - Rework ovs_asserts() into function selection time check
> > - Add #define for magic number 8, number of u64 blocks in AVX512 register
> > - Add #if CHECKER around AVX code, sparse doesn't like checking it
> > - Simplify avx512 enabled building, fixes builds with --enable-shared
> > ---
> >  configure.ac                           |   2 +
> >  lib/automake.mk                        |  17 ++
> >  lib/dpif-netdev-lookup-avx512-gather.c | 265 +++++++++++++++++++++++++
> >  lib/dpif-netdev-lookup.c               |  17 ++
> >  lib/dpif-netdev-lookup.h               |   4 +
> >  5 files changed, 305 insertions(+)
> >  create mode 100644 lib/dpif-netdev-lookup-avx512-gather.c
> >
> > diff --git a/configure.ac b/configure.ac
> > index 81893e56e..1367c868b 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -178,6 +178,8 @@ OVS_ENABLE_OPTION([-Wno-null-pointer-arithmetic])
> >  OVS_ENABLE_OPTION([-Warray-bounds-pointer-arithmetic])
> >  OVS_CONDITIONAL_CC_OPTION([-Wno-unused], [HAVE_WNO_UNUSED])
> >  OVS_CONDITIONAL_CC_OPTION([-Wno-unused-parameter],
> [HAVE_WNO_UNUSED_PARAMETER])
> > +OVS_CONDITIONAL_CC_OPTION([-mavx512f], [HAVE_AVX512F])
> > +OVS_CHECK_CC_OPTION([-mavx512f], [CFLAGS="$CFLAGS -DHAVE_AVX512F"])
> 
> Do you need both checks?
> I thought the first one OVS_CONDITIONAL_CC_OPTION([-mavx512f],
> [HAVE_AVX512F])
> is good enough since at lib/automake.mk, you add the -mavx512f to CFLAGS.

From testing during development, both are required.
CONDITIONAL_CC_OPTION adds a build-system flag, indicating its present, but doesn't
seem to add a C #define for it, that can be used for conditional compilation?

The CHECK_CC_OPTION is used to manually add a #define via command-line -D parameter, it is used to add the avx512_gather probe function in the available lookup function struct.

There may be a more elegant way to achieve both in the same line, my AC-fu is somewhat outdated, suggestions welcome if you know of a better method :)

<snip some patch contents>

> > +#include "immintrin.h"
> > +
> > +/* Each AVX512 register (zmm register in assembly notation) can contain up
> to
> > + * 512 bits, which is equivelent to 8 uint64_t variables. This is the maximum
> 
> typo: equivalent

Will fix.


> > + * number of miniflow blocks that can be processed in a single pass of the
> > + * AVX512 code at a time.
> > + */
> > +#define NUM_U64_IN_ZMM_REG (8)
> > +#define BLOCKS_CACHE_SIZE (NETDEV_MAX_BURST *
> NUM_U64_IN_ZMM_REG)
> > +
> > +
> > +VLOG_DEFINE_THIS_MODULE(dpif_lookup_avx512_gather);
> > +
> > +static inline __m512i
> > +_mm512_popcnt_epi64_manual(__m512i v_in)
> > +{
> > +    static const uint8_t pop_lut[64] = {
> > +        0, 1, 1, 2, 1, 2, 2, 3, 1, 2, 2, 3, 2, 3, 3, 4,
> > +        0, 1, 1, 2, 1, 2, 2, 3, 1, 2, 2, 3, 2, 3, 3, 4,
> > +        0, 1, 1, 2, 1, 2, 2, 3, 1, 2, 2, 3, 2, 3, 3, 4,
> > +        0, 1, 1, 2, 1, 2, 2, 3, 1, 2, 2, 3, 2, 3, 3, 4,
> > +    };
> > +    __m512i v_pop_lut = _mm512_loadu_si512(pop_lut);
> > +
> > +    __m512i v_in_srl8 = _mm512_srli_epi64(v_in, 4);
> > +    __m512i v_nibble_mask = _mm512_set1_epi8(0xF);
> > +    __m512i v_in_lo = _mm512_and_si512(v_in, v_nibble_mask);
> > +    __m512i v_in_hi = _mm512_and_si512(v_in_srl8, v_nibble_mask);
> > +
> > +    __m512i v_lo_pop = _mm512_shuffle_epi8(v_pop_lut, v_in_lo);
> > +    __m512i v_hi_pop = _mm512_shuffle_epi8(v_pop_lut, v_in_hi);
> > +    __m512i v_u8_pop = _mm512_add_epi8(v_lo_pop, v_hi_pop);
> > +
> > +    return _mm512_sad_epu8(v_u8_pop, _mm512_setzero_si512());
> > +}
> 
> I forgot whether you mentioned this or not.
> But why create this manual popcnt?
> Isn't there a _mm512_popcnt_* in the library?

To answer your question directly:
The vector popcount instruction requires AVX512VPOPCNTDQ. Skylake does not include
the VPOPCNTDQ AVX512 extension. The "_manual" version enables the DPCLS to execute
on all AVX512 CPUs available today. In future, support for the AVX512 vector popcount can
be added with little effort.

The intrinsic guide for   _mm512_popcnt_epi64()  has more details: 
https://software.intel.com/sites/landingpage/IntrinsicsGuide/#text=vpopcnt&expand=4368

Note that it lists "CPUID Flags: AVX512VPOPCNTDQ", indicating a requirement on that ISA level.
It becomes available in the Ice Lake microarchitecture, more ISA details available here for those interested:
https://software.intel.com/content/www/us/en/develop/download/10th-generation-intel-core-processor-instruction-throughput-and-latency-docs.html


> The rest looks good to me,
> Thanks

Thanks for review.


More information about the dev mailing list