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

[Van Haaren, Harry]

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
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?


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.

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.
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?


Regards, -Harry

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