> -----Original Message----- > From: Eelco Chaudron <[email protected]> > Sent: Friday, September 10, 2021 3:41 PM > To: Van Haaren, Harry <[email protected]>; [email protected]; > Stokes, Ian <[email protected]>; [email protected] > Cc: Amber, Kumar <[email protected]>; [email protected]; > [email protected] > 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 <[email protected]> > >> Sent: Wednesday, September 8, 2021 9:16 AM > >> To: Amber, Kumar <[email protected]> > >> Cc: [email protected]; [email protected]; [email protected]; > >> Stokes, Ian <[email protected]>; [email protected]; Van Haaren, Harry > >> <[email protected]> > >> 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
