| Message ID | 20210903135316.2449305-1-kumar.amber@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v1] configure: Allow opt-in to CPU ISA opts at compile time | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
Not a real review of the patch, but just some comment/questions glancing over the patch. 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. > > If an subtable implementation is not available due to ISA not being available, > it will not be selected. > > With --enable-cpu-isa on an AVX512 capable CPU, the dpcls_avx512_gather, > the DPIF AVX512, MFEX AVX512 ISA optimized implementation is automatically > enabled. > > The default is off, so unless ./configure --enable-cpu-isa is passed, > the behaviour of the default OVS compile is not changed. > > Signed-off-by: Kumar Amber <kumar.amber@intel.com> > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> > Co-authored-by: Harry van Haaren <harry.van.haaren@intel.com> > --- > Documentation/topics/dpdk/bridge.rst | 9 +++++++++ > NEWS | 2 ++ > acinclude.m4 | 17 +++++++++++++++++ > configure.ac | 1 + > lib/dpif-netdev-lookup.c | 8 +++++++- > lib/dpif-netdev-private-dpif.c | 2 +- > lib/dpif-netdev-private-extract.c | 2 ++ > 7 files changed, 39 insertions(+), 2 deletions(-) > > diff --git a/Documentation/topics/dpdk/bridge.rst b/Documentation/topics/dpdk/bridge.rst > index f645b9ade..8568ec365 100644 > --- a/Documentation/topics/dpdk/bridge.rst > +++ b/Documentation/topics/dpdk/bridge.rst > @@ -436,3 +436,12 @@ the following test-case in tests/system-dpdk.at :: > > make check-dpdk TESTSUITEFLAGS='-k MFEX' > OVS-DPDK - MFEX Autovalidator Fuzzy > + > +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. 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. > diff --git a/NEWS b/NEWS > index 1f2adf718..8d700595b 100644 > --- a/NEWS > +++ b/NEWS > @@ -68,6 +68,8 @@ v2.16.0 - 16 Aug 2021 > * Add new 'pmd-rxq-isolate' option that can be set to 'false' in order > that pmd cores which are pinned with rxqs using 'pmd-rxq-affinity' > are available for assigning other non-pinned rxqs. > + * Add command and documentation to enable all AVX512 optimized options > + by default at build time. > - ovs-ctl: > * New option '--no-record-hostname' to disable hostname configuration > in ovsdb on startup. > diff --git a/acinclude.m4 b/acinclude.m4 > index dba365ea1..75098f980 100644 > --- a/acinclude.m4 > +++ b/acinclude.m4 > @@ -33,6 +33,23 @@ AC_DEFUN([OVS_CHECK_MFEX_AUTOVALIDATOR], [ > fi > ]) > > +dnl Set OVS DPCLS DPIF and MFEX to AVX512 opt at compile time? > +AC_DEFUN([OVS_CHECK_CPU_ISA_OPT_IN], [ > + AC_ARG_ENABLE([cpu-isa], > + [AC_HELP_STRING([--enable-cpu-isa], > + [Enable CPU ISA default enable.])], > + [isa=yes],[isa=no]) > + AC_MSG_CHECKING([whether CPU ISA should be enabled by default]) This description does not sound right? > + if test "$isa" != yes; then > + AC_MSG_RESULT([no]) > + else > + AC_DEFINE([CPU_ISA_OPT_IN], [1], > + [DPIF AVX512, DPCLS AVX512, MFEX AVX512 is a > + default implementation.]) > + AC_MSG_RESULT([yes]) > + fi > +]) > + > dnl Set OVS DPCLS Autovalidator as default subtable search at compile time? > dnl This enables automatically running all unit tests with all DPCLS > dnl implementations. > diff --git a/configure.ac b/configure.ac > index eaa9bf7ee..47e64a98c 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -182,6 +182,7 @@ OVS_CONDITIONAL_CC_OPTION([-Wno-unused-parameter], [HAVE_WNO_UNUSED_PARAMETER]) > OVS_ENABLE_WERROR > OVS_ENABLE_SPARSE > OVS_CTAGS_IDENTIFIERS > +OVS_CHECK_CPU_ISA_OPT_IN > OVS_CHECK_DPCLS_AUTOVALIDATOR > OVS_CHECK_DPIF_AVX512_DEFAULT > OVS_CHECK_MFEX_AUTOVALIDATOR > diff --git a/lib/dpif-netdev-lookup.c b/lib/dpif-netdev-lookup.c > index bd0a99abe..0989c6a5f 100644 > --- a/lib/dpif-netdev-lookup.c > +++ b/lib/dpif-netdev-lookup.c > @@ -45,7 +45,13 @@ static struct dpcls_subtable_lookup_info_t subtable_lookups[] = { > > #if (__x86_64__ && HAVE_AVX512F && HAVE_LD_AVX512_GOOD && __SSE4_2__) > /* Only available on x86_64 bit builds with SSE 4.2 used for OVS core. */ > - { .prio = 0, > + { > +#ifdef CPU_ISA_OPT_IN > + /* Allow Autovalidator to override, but higher than default scalar. */ > + .prio = 100, > +#else > + .prio = 0, > +#endif > .probe = dpcls_subtable_avx512_gather_probe, > .name = "avx512_gather", }, > #else > diff --git a/lib/dpif-netdev-private-dpif.c b/lib/dpif-netdev-private-dpif.c > index 84d4ec156..22761c10e 100644 > --- a/lib/dpif-netdev-private-dpif.c > +++ b/lib/dpif-netdev-private-dpif.c > @@ -60,7 +60,7 @@ dp_netdev_impl_get_default(void) > > /* Configure-time overriding to run test suite on all implementations. */ > #if (__x86_64__ && HAVE_AVX512F && HAVE_LD_AVX512_GOOD && __SSE4_2__) > -#ifdef DPIF_AVX512_DEFAULT > +#if defined(DPIF_AVX512_DEFAULT) || defined(CPU_ISA_OPT_IN) > dp_netdev_input_func_probe probe; > > /* Check if the compiled default is compatible. */ > diff --git a/lib/dpif-netdev-private-extract.c b/lib/dpif-netdev-private-extract.c > index 7a06dbf6f..482aa1f37 100644 > --- a/lib/dpif-netdev-private-extract.c > +++ b/lib/dpif-netdev-private-extract.c > @@ -105,6 +105,8 @@ dpif_miniflow_extract_init(void) > atomic_uintptr_t *mfex_func = (void *)&default_mfex_func; > #ifdef MFEX_AUTOVALIDATOR_DEFAULT > int mfex_idx = MFEX_IMPL_AUTOVALIDATOR; > +#elif CPU_ISA_OPT_IN > + int mfex_idx = MFEX_IMPL_STUDY; > #else > int mfex_idx = MFEX_IMPL_SCALAR; > #endif > -- > 2.25.1
> -----Original Message----- > From: Eelco Chaudron <echaudro@redhat.com> > Sent: Wednesday, September 8, 2021 9:16 AM > To: Amber, Kumar <kumar.amber@intel.com> > Cc: ovs-dev@openvswitch.org; ktraynor@redhat.com; i.maximets@ovn.org; > Stokes, Ian <ian.stokes@intel.com>; fbl@sysclose.org; Van Haaren, Harry > <harry.van.haaren@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... > 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
On 8 Sep 2021, at 17:28, Van Haaren, Harry wrote: >> -----Original Message----- >> From: Eelco Chaudron <echaudro@redhat.com> >> Sent: Wednesday, September 8, 2021 9:16 AM >> To: Amber, Kumar <kumar.amber@intel.com> >> Cc: ovs-dev@openvswitch.org; ktraynor@redhat.com; i.maximets@ovn.org; >> Stokes, Ian <ian.stokes@intel.com>; fbl@sysclose.org; Van Haaren, Harry >> <harry.van.haaren@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. 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
> -----Original Message----- > From: Eelco Chaudron <echaudro@redhat.com> > Sent: Friday, September 10, 2021 3:41 PM > To: Van Haaren, Harry <harry.van.haaren@intel.com>; i.maximets@ovn.org; > Stokes, Ian <ian.stokes@intel.com>; fbl@sysclose.org > Cc: Amber, Kumar <kumar.amber@intel.com>; ovs-dev@openvswitch.org; > ktraynor@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@redhat.com> > >> Sent: Wednesday, September 8, 2021 9:16 AM > >> To: Amber, Kumar <kumar.amber@intel.com> > >> Cc: ovs-dev@openvswitch.org; ktraynor@redhat.com; i.maximets@ovn.org; > >> Stokes, Ian <ian.stokes@intel.com>; fbl@sysclose.org; Van Haaren, Harry > >> <harry.van.haaren@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
On 13 Sep 2021, at 16:36, Van Haaren, Harry wrote: >> -----Original Message----- >> From: Eelco Chaudron <echaudro@redhat.com> >> Sent: Friday, September 10, 2021 3:41 PM >> To: Van Haaren, Harry <harry.van.haaren@intel.com>; i.maximets@ovn.org; >> Stokes, Ian <ian.stokes@intel.com>; fbl@sysclose.org >> Cc: Amber, Kumar <kumar.amber@intel.com>; ovs-dev@openvswitch.org; >> ktraynor@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@redhat.com> >>>> Sent: Wednesday, September 8, 2021 9:16 AM >>>> To: Amber, Kumar <kumar.amber@intel.com> >>>> Cc: ovs-dev@openvswitch.org; ktraynor@redhat.com; i.maximets@ovn.org; >>>> Stokes, Ian <ian.stokes@intel.com>; fbl@sysclose.org; Van Haaren, Harry >>>> <harry.van.haaren@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. Thinking about this more, I do not feel like we should introduce compile-time default configuration options. It should just be added to the ovsdb and picked up from there on startup. Same as for configuring the PMD cores/queues, etc. etc. Would be good to get some input from others (maintainers) on this?! >>>> 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
On Mon, Sep 13, 2021 at 02:36:41PM +0000, Van Haaren, Harry wrote: > > -----Original Message----- > > From: Eelco Chaudron <echaudro@redhat.com> > > Sent: Friday, September 10, 2021 3:41 PM > > To: Van Haaren, Harry <harry.van.haaren@intel.com>; i.maximets@ovn.org; > > Stokes, Ian <ian.stokes@intel.com>; fbl@sysclose.org > > Cc: Amber, Kumar <kumar.amber@intel.com>; ovs-dev@openvswitch.org; > > ktraynor@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@redhat.com> > > >> Sent: Wednesday, September 8, 2021 9:16 AM > > >> To: Amber, Kumar <kumar.amber@intel.com> > > >> Cc: ovs-dev@openvswitch.org; ktraynor@redhat.com; i.maximets@ovn.org; > > >> Stokes, Ian <ian.stokes@intel.com>; fbl@sysclose.org; Van Haaren, Harry > > >> <harry.van.haaren@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") It seems to me this is coming to address the distro release process requirement of not going back in the process to change the RPM package. To give others some background: An overview of release process is like: we build a RPM package, then send it to QA, and if it gets approved, then ship it to users/customers. Therefore, there is no way to go back to rebuild/change the RPM to enable/disable something, then ship to users/customers. That was a concern initially with AVX512 implementation. However, the implementation selection can be done at runtime, and it doesn't require changing the RPM. So, from the distro release process point of view, there would be no need for this build option. See below. > > 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. That sounds like a better solution to me. OVS stores persistent settings in the ovsdb, and that's what we look for when we need to support OVS. Does that address the original requirements of this patch? > > >> 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. Yeah, the concern is more like how we could expand and express that in the future in case there is a new optimization available. Because --enable-cpu-isa seems way too generic and we won't be able to change that without breaking stuff. Also that, it may open doors to change other defaults, and that makes life harder to support OVS because we will need to find out how OVS was built instead of just checking the ovsdb. Thanks,
diff --git a/Documentation/topics/dpdk/bridge.rst b/Documentation/topics/dpdk/bridge.rst index f645b9ade..8568ec365 100644 --- a/Documentation/topics/dpdk/bridge.rst +++ b/Documentation/topics/dpdk/bridge.rst @@ -436,3 +436,12 @@ the following test-case in tests/system-dpdk.at :: make check-dpdk TESTSUITEFLAGS='-k MFEX' OVS-DPDK - MFEX Autovalidator Fuzzy + +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 diff --git a/NEWS b/NEWS index 1f2adf718..8d700595b 100644 --- a/NEWS +++ b/NEWS @@ -68,6 +68,8 @@ v2.16.0 - 16 Aug 2021 * Add new 'pmd-rxq-isolate' option that can be set to 'false' in order that pmd cores which are pinned with rxqs using 'pmd-rxq-affinity' are available for assigning other non-pinned rxqs. + * Add command and documentation to enable all AVX512 optimized options + by default at build time. - ovs-ctl: * New option '--no-record-hostname' to disable hostname configuration in ovsdb on startup. diff --git a/acinclude.m4 b/acinclude.m4 index dba365ea1..75098f980 100644 --- a/acinclude.m4 +++ b/acinclude.m4 @@ -33,6 +33,23 @@ AC_DEFUN([OVS_CHECK_MFEX_AUTOVALIDATOR], [ fi ]) +dnl Set OVS DPCLS DPIF and MFEX to AVX512 opt at compile time? +AC_DEFUN([OVS_CHECK_CPU_ISA_OPT_IN], [ + AC_ARG_ENABLE([cpu-isa], + [AC_HELP_STRING([--enable-cpu-isa], + [Enable CPU ISA default enable.])], + [isa=yes],[isa=no]) + AC_MSG_CHECKING([whether CPU ISA should be enabled by default]) + if test "$isa" != yes; then + AC_MSG_RESULT([no]) + else + AC_DEFINE([CPU_ISA_OPT_IN], [1], + [DPIF AVX512, DPCLS AVX512, MFEX AVX512 is a + default implementation.]) + AC_MSG_RESULT([yes]) + fi +]) + dnl Set OVS DPCLS Autovalidator as default subtable search at compile time? dnl This enables automatically running all unit tests with all DPCLS dnl implementations. diff --git a/configure.ac b/configure.ac index eaa9bf7ee..47e64a98c 100644 --- a/configure.ac +++ b/configure.ac @@ -182,6 +182,7 @@ OVS_CONDITIONAL_CC_OPTION([-Wno-unused-parameter], [HAVE_WNO_UNUSED_PARAMETER]) OVS_ENABLE_WERROR OVS_ENABLE_SPARSE OVS_CTAGS_IDENTIFIERS +OVS_CHECK_CPU_ISA_OPT_IN OVS_CHECK_DPCLS_AUTOVALIDATOR OVS_CHECK_DPIF_AVX512_DEFAULT OVS_CHECK_MFEX_AUTOVALIDATOR diff --git a/lib/dpif-netdev-lookup.c b/lib/dpif-netdev-lookup.c index bd0a99abe..0989c6a5f 100644 --- a/lib/dpif-netdev-lookup.c +++ b/lib/dpif-netdev-lookup.c @@ -45,7 +45,13 @@ static struct dpcls_subtable_lookup_info_t subtable_lookups[] = { #if (__x86_64__ && HAVE_AVX512F && HAVE_LD_AVX512_GOOD && __SSE4_2__) /* Only available on x86_64 bit builds with SSE 4.2 used for OVS core. */ - { .prio = 0, + { +#ifdef CPU_ISA_OPT_IN + /* Allow Autovalidator to override, but higher than default scalar. */ + .prio = 100, +#else + .prio = 0, +#endif .probe = dpcls_subtable_avx512_gather_probe, .name = "avx512_gather", }, #else diff --git a/lib/dpif-netdev-private-dpif.c b/lib/dpif-netdev-private-dpif.c index 84d4ec156..22761c10e 100644 --- a/lib/dpif-netdev-private-dpif.c +++ b/lib/dpif-netdev-private-dpif.c @@ -60,7 +60,7 @@ dp_netdev_impl_get_default(void) /* Configure-time overriding to run test suite on all implementations. */ #if (__x86_64__ && HAVE_AVX512F && HAVE_LD_AVX512_GOOD && __SSE4_2__) -#ifdef DPIF_AVX512_DEFAULT +#if defined(DPIF_AVX512_DEFAULT) || defined(CPU_ISA_OPT_IN) dp_netdev_input_func_probe probe; /* Check if the compiled default is compatible. */ diff --git a/lib/dpif-netdev-private-extract.c b/lib/dpif-netdev-private-extract.c index 7a06dbf6f..482aa1f37 100644 --- a/lib/dpif-netdev-private-extract.c +++ b/lib/dpif-netdev-private-extract.c @@ -105,6 +105,8 @@ dpif_miniflow_extract_init(void) atomic_uintptr_t *mfex_func = (void *)&default_mfex_func; #ifdef MFEX_AUTOVALIDATOR_DEFAULT int mfex_idx = MFEX_IMPL_AUTOVALIDATOR; +#elif CPU_ISA_OPT_IN + int mfex_idx = MFEX_IMPL_STUDY; #else int mfex_idx = MFEX_IMPL_SCALAR; #endif