[ovs-dev] [PATCH] dpif-netdev-lookup: Allow AVX512 dpcls without building core OVS with SSE4.2.

Van Haaren, Harry harry.van.haaren at intel.com
Tue Mar 23 14:11:19 UTC 2021


> -----Original Message-----
> From: Ilya Maximets <i.maximets at ovn.org>
> Sent: Monday, March 22, 2021 12:55 PM
> To: Van Haaren, Harry <harry.van.haaren at intel.com>; Ilya Maximets
> <i.maximets at ovn.org>; ovs-dev at openvswitch.org
> Cc: Stokes, Ian <ian.stokes at intel.com>; Timothy Redaelli
> <tredaelli at redhat.com>; Flavio Leitner <fbl at sysclose.org>
> Subject: Re: [PATCH] dpif-netdev-lookup: Allow AVX512 dpcls without building
> core OVS with SSE4.2.
> 
> On 3/22/21 12:45 PM, Van Haaren, Harry wrote:
> > Hi Ilya,
> >
> >> -----Original Message-----
> >> From: Ilya Maximets <i.maximets at ovn.org>
> >> Sent: Monday, March 22, 2021 10:59 AM
> >> To: ovs-dev at openvswitch.org; Van Haaren, Harry
> >> <harry.van.haaren at intel.com>
> >> Cc: Stokes, Ian <ian.stokes at intel.com>; Timothy Redaelli
> >> <tredaelli at redhat.com>; Flavio Leitner <fbl at sysclose.org>; Ilya Maximets
> >> <i.maximets at ovn.org>
> >> Subject: [PATCH] dpif-netdev-lookup: Allow AVX512 dpcls without building
> core
> >> OVS with SSE4.2.
> >>
> >> It's only required to build lib/dpif-netdev-lookup-avx512-gather.c
> >> with SSE4.2.  All hash functions in lib/hash.h are defined as
> >> 'static inline', so they will use fast crc instructions while building
> >> this module.  The minimal requirement for core OVS could be left
> >> intact.
> >>
> >> With this change support for SSE4.2 is checked in ./configure and
> >> -msse4.2  passed to build libopenvswitchavx512.  The rest of OVS is
> >> built with default flags provided by user.  So, AVX512 dpcls could
> >> be enabled in a build with default CFLAGS and the rest of the OVS
> >> could run on older CPUs or in virtual environments without support
> >> for sse4.2 instructions.
> >>
> >> Same for popcnt.
> >>
> >> Since the whole lib/dpif-netdev-lookup-avx512-gather.c is built with
> >> special instruction sets enabled it's not safe to call any function
> >> from where unless we're sure that these ISA is supported by current
> >> CPU.  For this reason runtime ISA checks moved to common initialization
> >> code and performed only once.  avx512-gather implementation will
> >> not be registered and will not be available for a user if current
> >> CPU doesn't support it.  Log in dpdk-stub lowered to DBG level because
> >> now it will be printed always on startup and it's not really useful
> >> because user will not have ability to enable avx512 implementation.
> >>
> >> Signed-off-by: Ilya Maximets <i.maximets at ovn.org>
> >
> >
> > Thanks for taking a look at the DPCLS AVX512 optimizations & functionality.
> > From the commit message I understand "what" you're changing, but I failed
> > to understand a clear "why"?
> >
> > From initial review, it seems to do a few things:
> > 1) Rework CPU flags (sse4.2,popcnt) from compile-time checks to runtime
> 
> Some compile time checks are still there, but moved around a little.

Yep agreed.


> > 2) Rework DPCLS impl from static registration to runtime/dynamic
> >
> > Is the big-picture goal to avoid compiling core OVS with SSE4.2/popcnt enabled?
> > Perhaps a better question - what is the big picture goal?
> 
> Yes, the big-picture goal was to avoid compiling core OVS with SSE4.2,
> i.e. being able to run the same binary on old or virtual hardware and
> being able to enable avx512 optimizations while running on high-end hardware.

I'm failing to see the requirement for compiling ore OVS without SSE4.2 support.
Is this a "nice to have", or a real requirement?

Assuming these CPUs are actually going to *run* OVS, having SSE4.2 support enabled
means that performance actually increases for "free" by using the CRC32Q for hashing...
which I expect is desired in that case. Even when running a virtualized OVS, surely we
don't want to limit performance artificially by reducing baseline ISA without a benefit?


> > Regarding 1) the requirement for SSE4.2 for CRC32Q instruction based hashing,
> > I think you have mistook the reason for *requiring* core OVS to be built with
> > SSE4.2 enabled. The hashing done in OVS core and DPCLS must be identical.
> > Allowing OVS core to use murmurhash (no SSE4.2), and DPCLS using CRC32 (yes
> SSE4.2)
> > means that upcall hash values, and datapath hash values don't match. For this
> > reason, it is invalid to mix-and-match SSE4.2 support in *compliation units*
> across
> > OVS.
> 
> Thanks for pointing that out.  I missed that completely.  Basically, I forgot
> about this.  We should probably add this information to code comments as it's
> easy to miss that fact.  Or do we have it already and I just messed it?

Its noted in the docs that AVX512 disables if sse4.2 and popcnt are missing, and
it refers to the below source-code snippet;
https://docs.openvswitch.org/en/latest/intro/install/dpdk/#install-ovs

There's a comment in  lib/dpif-netdev-lookup.c + 52  about compile time requirements,
but it doesn't explicitly call out the reason (hashing code changes based on compile time isa).

We can document this better - if ovs community think that would be useful?


> Would be nice to have a build assertion.  I'm not sure how to implement it,
> though.
> 
> The thing that we need is that hash computation in netdev_flow_mask/key_init()
> needs to match with hash in dpif-netdev-lookup implementation.
> And this is likely achievable if we will create a helper static inline function
> implemented somewhere inside dpif-netdev-lookup.h to calculate flow hash.
> And since miniflow structure already exists at the point of hash calculation
> we can choose the implementation the same way as we're choosing lookup
> implementation.

I dislike this idea of "fixing up" one of the places where hashing is different.
There are numerous places where hashing is/could-be used, and the compiler
would not give warnings/errors on where hash-mis-matches due to ISA occur.

I noticed & fixed this issue when building AVX512 DPCLS in the way I did because
it was difficult to debug, and one would not suspect the hash_add() function in
OVS from providing "invalid" results due to the ISA enabled in the compilation unit.

If we did take this "fixing up" approach, all future hashing code in upcalls would have to
be manually reviewed in the context of "is a similar hash recomputed in datapath".
Some locations where I can imagine hashing is used in upcalls is for features like
conn-track, load-balancing actions, LAG... these would all *silently* break if we
didn't go and do runtime-testing with ISA on/off, and play "whack-a-mole" with
the resulting bugs... sounds nasty to me.


> But, yes, this will be a bigger change.

All the above complexity/effort, to gain what benefit? As per above question,
I don't see specific value in reducing core OVS compile-time ISA requirements
to below SSE4.2, as per first topic.


> > To be clear, this is a side-effect of compile-time ISA-based hash function
> selection,
> > and has actually nothing to do with DPCLS per-se: it just shows up here first as it
> > is the first location ISA specialization is being done in OVS.
> >
> > I don't see issues with 2) reworking registration, it can be a separate patch to
> > the 1) CPU-flag rework topic. Although related, it does not depend on it I think?
> >
> > All in all, I'm OK with the registration rework if split into a separate patch.
> 
> It could be that rework is required as a bug fix, because current runtime
> checks are done inside the code that already built with higher ISA enabled,
> so it might trigger illegal instruction exception during the check, in theory.

Ok, lets handle that as a separate topic; how about we annotate the probe()
function to use only "generic CPU with 64-bit extensions" ISA? Much smaller
diff, achieves the same thing, probe() function prototype becomes this:

dpcls_subtable_lookup_func  __attribute__((__target__(arch="x86-64"))
dpcls_subtable_<name>_probe(uint32_t u0, uint32_t u1)
{
     // ISA safe here, due to __target__ attribute
}

See target() attribute docs, and -march= options on 2nd link, and nice ISA table on wikipedia:
https://gcc.gnu.org/onlinedocs/gcc-4.7.2/gcc/Function-Attributes.html
https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html
https://en.wikipedia.org/wiki/X86-64#Microarchitecture_Levels


> 
> > Regarding topic 1) SSE4.2 ISA-changes, I think it is not valid.
> >
> >> ---
> >>
> >> I only tested the build, didn't check in runtime.
> >>
> >
> > You state this wasn't runtime checked - likely issues would have shown up
> (DPCLS
> > misses on installed keys, resulting in every packet getting an upcall) if it was
> tested.
> > What needs to be done to ensure you have access to a machine with AVX512?
> 
> In practice, I just have no enough time for proper runtime checking.
> Writing this patch was pretty fast, OTOH. :)

Hah, okay... Most open-source communities don't like reviewing patches
that haven't been tested. As the saying goes "untested code is broken code" :)


> > Regards, -Harry
> >
> > <snip patch contents, as high-level review didn't investigate details>
> >



More information about the dev mailing list