| Message ID | 20210706131150.45513-6-cian.ferriter@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | MFEX Infrastructure + Optimizations | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
On 6 Jul 2021, at 15:11, Cian Ferriter wrote: > From: Kumar Amber <kumar.amber@intel.com> > > This commit adds a new command to allow the user to enable > autovalidatior by default at build time thus allowing for > runnig unit test by default. > > $ ./configure --enable-mfex-default-autovalidator > > Signed-off-by: Kumar Amber <kumar.amber@intel.com> > Co-authored-by: Harry van Haaren <harry.van.haaren@intel.com> > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> > > --- > > v5: > - fix review comments(Ian, Flavio, Eelco) > --- > --- > Documentation/topics/dpdk/bridge.rst | 5 +++++ > NEWS | 5 +++-- > acinclude.m4 | 16 ++++++++++++++++ > configure.ac | 1 + > lib/dpif-netdev-private-extract.c | 8 +++++++- > 5 files changed, 32 insertions(+), 3 deletions(-) > > diff --git a/Documentation/topics/dpdk/bridge.rst > b/Documentation/topics/dpdk/bridge.rst > index 2901e8096..c79e108b7 100644 > --- a/Documentation/topics/dpdk/bridge.rst > +++ b/Documentation/topics/dpdk/bridge.rst > @@ -305,3 +305,8 @@ implementations provide the same results. > To set the Miniflow autovalidator, use this command :: > > $ ovs-appctl dpif-netdev/miniflow-parser-set autovalidator > + > +A compile time option is available in order to test it with the OVS > unit > +test suite. Use the following configure option :: > + > + $ ./configure --enable-mfex-default-autovalidator > diff --git a/NEWS b/NEWS > index 275aa1868..608c5a32f 100644 > --- a/NEWS > +++ b/NEWS > @@ -25,9 +25,11 @@ Post-v2.15.0 > * Add command line option to switch between mfex function > pointers. > * Add miniflow extract auto-validator function to compare > different > miniflow extract implementations against default > implementation. > - * Add study function to miniflow function table which studies > packet > + * Add study function to miniflow function table which studies > packet > and automatically chooses the best miniflow implementation for > that > traffic. > + * Add build time configure command to enable auto-validatior as > default > + miniflow implementation at build time. > - ovs-ctl: > * New option '--no-record-hostname' to disable hostname > configuration > in ovsdb on startup. > @@ -44,7 +46,6 @@ Post-v2.15.0 > * New option '--election-timer' to the 'create-cluster' command > to set the > leader election timer during cluster creation. > > - > v2.15.0 - 15 Feb 2021 > --------------------- > - OVSDB: > diff --git a/acinclude.m4 b/acinclude.m4 > index 5fbcd9872..e2704cfda 100644 > --- a/acinclude.m4 > +++ b/acinclude.m4 > @@ -14,6 +14,22 @@ > # See the License for the specific language governing permissions and > # limitations under the License. > > +dnl Set OVS MFEX Autovalidator as default miniflow extract at compile > time? > +dnl This enables automatically running all unit tests with all MFEX > +dnl implementations. > +AC_DEFUN([OVS_CHECK_MFEX_AUTOVALIDATOR], [ > + AC_ARG_ENABLE([mfex-default-autovalidator], > + > [AC_HELP_STRING([--enable-mfex-default-autovalidator], [Enable MFEX > autovalidator as default miniflow_extract implementation.])], > + [autovalidator=yes],[autovalidator=no]) > + AC_MSG_CHECKING([whether MFEX Autovalidator is default > implementation]) > + if test "$autovalidator" != yes; then > + AC_MSG_RESULT([no]) > + else > + OVS_CFLAGS="$OVS_CFLAGS -DMFEX_AUTOVALIDATOR_DEFAULT" > + 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 e45685a6c..46c402892 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -186,6 +186,7 @@ OVS_ENABLE_SPARSE > OVS_CTAGS_IDENTIFIERS > OVS_CHECK_DPCLS_AUTOVALIDATOR > OVS_CHECK_DPIF_AVX512_DEFAULT > +OVS_CHECK_MFEX_AUTOVALIDATOR > OVS_CHECK_BINUTILS_AVX512 > > AC_ARG_VAR(KARCH, [Kernel Architecture String]) > diff --git a/lib/dpif-netdev-private-extract.c > b/lib/dpif-netdev-private-extract.c > index eaddeceaf..6ae91a24d 100644 > --- a/lib/dpif-netdev-private-extract.c > +++ b/lib/dpif-netdev-private-extract.c > @@ -76,6 +76,12 @@ dpif_miniflow_extract_init(void) > miniflow_extract_func > dp_mfex_impl_get_default(void) > { > + > +#ifdef MFEX_AUTOVALIDATOR_DEFAULT > + VLOG_INFO("Default miniflow Extract implementation %s", > + mfex_impls[MFEX_IMPL_AUTOVALIDATOR].name); > + default_mfex_func = > mfex_impls[MFEX_IMPL_AUTOVALIDATOR].extract_func; > +#else The change above forces you to always use the autovalidator, even if you configure another option. I think this would make it impossible to run potential tests cases with the autovalidator as default. I think this should probably be protected by ovsthread_once_start(), so it will only be run once at startup time. It might be even better to do this at init time? For example (did not test): 102 void 103 dpif_miniflow_extract_init(void) 104 { 105 /* Call probe on each impl, and cache the result. */ 106 uint32_t i; 107 for (i = 0; i < ARRAY_SIZE(mfex_impls); i++) { 108 bool avail = true; 109 if (mfex_impls[i].probe) { 110 /* Return zero is success, non-zero means error. */ 111 avail = (mfex_impls[i].probe() == 0); 112 } 113 VLOG_INFO("Miniflow Extract implementation %s (available: %s)\n", 114 mfex_impls[i].name, avail ? "True" : "False"); 115 mfex_impls[i].available = avail; 116 } 117 118 #ifdef MFEX_AUTOVALIDATOR_DEFAULT 119 VLOG_INFO("Default miniflow Extract implementation %s", 120 mfex_impls[MFEX_IMPL_AUTOVALIDATOR].name); 121 default_mfex_func = mfex_impls[MFEX_IMPL_AUTOVALIDATOR].extract_func; 122 #else 123 VLOG_INFO("Default MFEX implementation is %s.\n", 124 mfex_impls[MFEX_IMPL_SCALAR].name); 125 default_mfex_func = mfex_impls[MFEX_IMPL_SCALAR].extract_func; 126 #endif 127 } 128 129 miniflow_extract_func 130 dp_mfex_impl_get_default(void) 131 { 132 return default_mfex_func; 133 } > /* For the first call, this will be NULL. Compute the compile > time default. > */ > if (!default_mfex_func) { > @@ -84,7 +90,7 @@ dp_mfex_impl_get_default(void) > mfex_impls[MFEX_IMPL_SCALAR].name); > default_mfex_func = > mfex_impls[MFEX_IMPL_SCALAR].extract_func; > } > - > +#endif > return default_mfex_func; > } > > -- > 2.32.0
Hi Eelco,
Ah great catch :
+#ifdef MFEX_AUTOVALIDATOR_DEFAULT
+ VLOG_INFO("Default miniflow Extract implementation %s",
+ mfex_impls[MFEX_IMPL_AUTOVALIDATOR].name);
+ default_mfex_func = mfex_impls[MFEX_IMPL_AUTOVALIDATOR].extract_func;
+#else
The change above forces you to always use the autovalidator, even if you configure another option. I think this would make it impossible to run potential tests cases with the autovalidator as default.
I think this should probably be protected by ovsthread_once_start(), so it will only be run once at startup time. It might be even better to do this at init time? For example (did not test):
I think this should be protected by the :
if (!default_mfex_func) { }
that way we will only set it at default time
will fix it in v7.
<Snip>
Regards
Amber
diff --git a/Documentation/topics/dpdk/bridge.rst b/Documentation/topics/dpdk/bridge.rst index 2901e8096..c79e108b7 100644 --- a/Documentation/topics/dpdk/bridge.rst +++ b/Documentation/topics/dpdk/bridge.rst @@ -305,3 +305,8 @@ implementations provide the same results. To set the Miniflow autovalidator, use this command :: $ ovs-appctl dpif-netdev/miniflow-parser-set autovalidator + +A compile time option is available in order to test it with the OVS unit +test suite. Use the following configure option :: + + $ ./configure --enable-mfex-default-autovalidator diff --git a/NEWS b/NEWS index 275aa1868..608c5a32f 100644 --- a/NEWS +++ b/NEWS @@ -25,9 +25,11 @@ Post-v2.15.0 * Add command line option to switch between mfex function pointers. * Add miniflow extract auto-validator function to compare different miniflow extract implementations against default implementation. - * Add study function to miniflow function table which studies packet + * Add study function to miniflow function table which studies packet and automatically chooses the best miniflow implementation for that traffic. + * Add build time configure command to enable auto-validatior as default + miniflow implementation at build time. - ovs-ctl: * New option '--no-record-hostname' to disable hostname configuration in ovsdb on startup. @@ -44,7 +46,6 @@ Post-v2.15.0 * New option '--election-timer' to the 'create-cluster' command to set the leader election timer during cluster creation. - v2.15.0 - 15 Feb 2021 --------------------- - OVSDB: diff --git a/acinclude.m4 b/acinclude.m4 index 5fbcd9872..e2704cfda 100644 --- a/acinclude.m4 +++ b/acinclude.m4 @@ -14,6 +14,22 @@ # See the License for the specific language governing permissions and # limitations under the License. +dnl Set OVS MFEX Autovalidator as default miniflow extract at compile time? +dnl This enables automatically running all unit tests with all MFEX +dnl implementations. +AC_DEFUN([OVS_CHECK_MFEX_AUTOVALIDATOR], [ + AC_ARG_ENABLE([mfex-default-autovalidator], + [AC_HELP_STRING([--enable-mfex-default-autovalidator], [Enable MFEX autovalidator as default miniflow_extract implementation.])], + [autovalidator=yes],[autovalidator=no]) + AC_MSG_CHECKING([whether MFEX Autovalidator is default implementation]) + if test "$autovalidator" != yes; then + AC_MSG_RESULT([no]) + else + OVS_CFLAGS="$OVS_CFLAGS -DMFEX_AUTOVALIDATOR_DEFAULT" + 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 e45685a6c..46c402892 100644 --- a/configure.ac +++ b/configure.ac @@ -186,6 +186,7 @@ OVS_ENABLE_SPARSE OVS_CTAGS_IDENTIFIERS OVS_CHECK_DPCLS_AUTOVALIDATOR OVS_CHECK_DPIF_AVX512_DEFAULT +OVS_CHECK_MFEX_AUTOVALIDATOR OVS_CHECK_BINUTILS_AVX512 AC_ARG_VAR(KARCH, [Kernel Architecture String]) diff --git a/lib/dpif-netdev-private-extract.c b/lib/dpif-netdev-private-extract.c index eaddeceaf..6ae91a24d 100644 --- a/lib/dpif-netdev-private-extract.c +++ b/lib/dpif-netdev-private-extract.c @@ -76,6 +76,12 @@ dpif_miniflow_extract_init(void) miniflow_extract_func dp_mfex_impl_get_default(void) { + +#ifdef MFEX_AUTOVALIDATOR_DEFAULT + VLOG_INFO("Default miniflow Extract implementation %s", + mfex_impls[MFEX_IMPL_AUTOVALIDATOR].name); + default_mfex_func = mfex_impls[MFEX_IMPL_AUTOVALIDATOR].extract_func; +#else /* For the first call, this will be NULL. Compute the compile time default. */ if (!default_mfex_func) { @@ -84,7 +90,7 @@ dp_mfex_impl_get_default(void) mfex_impls[MFEX_IMPL_SCALAR].name); default_mfex_func = mfex_impls[MFEX_IMPL_SCALAR].extract_func; } - +#endif return default_mfex_func; }