[ovs-dev] [PATCH v1] configure: Allow opt-in to CPU ISA opts at compile time

[Van Haaren, Harry]

> -----Original Message-----
> From: Eelco Chaudron <echaudro at redhat.com>
> Sent: Friday, September 10, 2021 3:41 PM
> To: Van Haaren, Harry <harry.van.haaren at intel.com>; i.maximets at ovn.org;
> Stokes, Ian <ian.stokes at intel.com>; fbl at sysclose.org
> Cc: Amber, Kumar <kumar.amber at intel.com>; ovs-dev at openvswitch.org;
> ktraynor at redhat.com
> Subject: Re: [PATCH v1] configure: Allow opt-in to CPU ISA opts at compile time
> 
> 
> 
> On 8 Sep 2021, at 17:28, Van Haaren, Harry wrote:
> 
> >> -----Original Message-----
> >> From: Eelco Chaudron <echaudro at redhat.com>
> >> Sent: Wednesday, September 8, 2021 9:16 AM
> >> To: Amber, Kumar <kumar.amber at intel.com>
> >> Cc: ovs-dev at openvswitch.org; ktraynor at redhat.com; i.maximets at ovn.org;
> >> Stokes, Ian <ian.stokes at intel.com>; fbl at sysclose.org; Van Haaren, Harry
> >> <harry.van.haaren at intel.com>
> >> Subject: Re: [PATCH v1] configure: Allow opt-in to CPU ISA opts at compile
> time
> >>
> >> Not a real review of the patch, but just some comment/questions glancing
> over
> >> the patch.
> >
> > Sure, thanks for input.
> >
> >> On 3 Sep 2021, at 15:53, Kumar Amber wrote:
> >>
> >>> This commit allows "opt-in" to CPU ISA optimized implementations of
> >>> OVS SW datapath components at compile time. This can be useful in some
> >>> deployments where the CPU ISA optimized implementation is to be chosen
> >>> by default.
> >
> > <snip some contents>
> >
> >>> +Enabling all AVX512 options
> >>> +---------------------------
> >>> +
> >>> +A user can enable all the three DPIF, Miniflow Extract and DPLCS optimized
> >>> +AVX512 options at build time, if the CPU supports the required AVX512 ISA
> >>> +by using the following command ::
> >>> +
> >>> +    ./configure --enable-cpu-isa
> >>
> >> If we have different ISA architectures, i.e., i86 vs ARM, we are ok with a single
> >> option. Have you thought about AMD adding its own AMDXXX instructions in
> >> addition to AVX512? How would this configuration option work? Maybe an
> >> optional option to prioritize one over the other.
> >
> > The ISA enabling efforts have been generic so far, any reference to specific ISA
> (e.g. AVX512)
> > has been solely in the implementation choice - never in a general component.
> Intention here
> > is to stay in line with that - and "enable CPU ISA" seemed a logical string to
> achieve that to me..
> >
> > It is of course possible to provide multiple configure command lines, but I was
> hoping to avoid
> > creating too many compile time flags. Typically I think projects attempt to avoid
> due to expanding
> > testing & validation. A single flag would limit overhead to the minimum...
> >
> > Typically ISA sets have a "good - better - best" type relationship - which could
> lead to a general
> > acceptance of what ISA is best. We have runtime functions to switch
> implementation - so today
> > the code already enables a log of runtime/dynamic updating of
> implementation. If there's a
> > need to expose that at compile time too, then that's easy to add - but comes
> with a burden in
> > testing & validation...
> 
> The main reason to mention this is the inconsistent behavior across
> builds/releases. With this flag being as general as it is, if someone decides to add
> AVX1024, it now gets selected as the default isa function (assuming the target
> was already supporting this). This is a change in behavior that happens without
> any configure option change. The difference to any other general change is that
> this is not a global change, but something that changes based on your target.

Note that the default setting for this option is suggested as "off", meaning this is an entirely
*opt in* strategy, to allow people to deploy OVS and automatically benefit from CPU ISA.

To be more specific, this feature is a request from folks who intend to deploy with CPU ISA
enabled by default - it suited their CI/CD/QA tooling to have this enabled by default compile
time switch to ease validation as the CPU ISA will get picked up automatically when available.

Note that it is not a "change in behaviour", because functionally its identical.
(The fuzzing, autovalidation & unit tests are there to ensure it is functionally identical).
It correct that there is a change in the default *implementation* of the functionality,
which I think you meant (just clarifying the "change in behaviour" as not being "functional behaviour",
only "implementation of behaviour")

> Anyone else has an opinion on this? I think an alternative to this, is a proper
> configuration option, which will survive a restart.
>
> Thinking about it a bit more during my afternoon walk, I think we should minimize
> (not allow) any compile-time default behavior variations. It should all be based on
> the configuration, which if required to survive a reboot, be added to the ovsdb.
>
> >> Even if we decide a single option is right, this one to me looks more about
> >> compile-time flags than enable ISA-specific code. Maybe something more in
> line
> >> with --prioritize-cpu-isa-functions? Or some other catchy name.
> >
> > Sure - that string is fine for me too.
> >
> > <snip patch contents>
> >
> > Regards, -Harry