[ovs-dev] [v9 05/12] dpif-netdev: Add configure to enable autovalidator at build time.

[Amber, Kumar]

Hi Eelco,

Fixed all and reworked default.

> -----Original Message-----
> From: Eelco Chaudron <echaudro at redhat.com>
> Sent: Monday, July 12, 2021 6:50 PM
> To: Amber, Kumar <kumar.amber at intel.com>
> Cc: ovs-dev at openvswitch.org; fbl at sysclose.org; i.maximets at ovn.org; Van
> Haaren, Harry <harry.van.haaren at intel.com>; Ferriter, Cian
> <cian.ferriter at intel.com>; Stokes, Ian <ian.stokes at intel.com>
> Subject: Re: [v9 05/12] dpif-netdev: Add configure to enable autovalidator at
> build time.
> 
> 
> 
> On 12 Jul 2021, at 7:51, kumar Amber wrote:
> 
> > From: Kumar Amber <kumar.amber at 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 at intel.com>
> > Co-authored-by: Harry van Haaren <harry.van.haaren at intel.com>
> > Signed-off-by: Harry van Haaren <harry.van.haaren at intel.com>
> >
> > ---
> > v9:
> > - fix review comments Flavio
> > v7:
> > - fix review commens(Eelco, Flavio)
> > v5:
> > - fix review comments(Ian, Flavio, Eelco)
> > ---
> > ---
> >  Documentation/topics/dpdk/bridge.rst |  5 +++++
> >  NEWS                                 |  3 ++-
> >  acinclude.m4                         | 16 ++++++++++++++++
> >  configure.ac                         |  1 +
> >  lib/dpif-netdev-private-extract.c    |  8 ++++++--
> >  5 files changed, 30 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/topics/dpdk/bridge.rst
> > b/Documentation/topics/dpdk/bridge.rst
> > index 7c618cf1f..4db416ddd 100644
> > --- a/Documentation/topics/dpdk/bridge.rst
> > +++ b/Documentation/topics/dpdk/bridge.rst
> > @@ -307,3 +307,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 4a7b89409..581bff225 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -38,6 +38,8 @@ Post-v2.15.0
> >       * 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.
> > @@ -57,7 +59,6 @@ Post-v2.15.0
> >       whether the SNAT with all-zero IP address is supported.
> >       See ovs-vswitchd.conf.db(5) for details.
> >
> > -
> 
> You are removing a white space here unrelated to your changes. Please leave it
> in.
> 
> >  v2.15.0 - 15 Feb 2021
> >  ---------------------
> >     - OVSDB:
> > diff --git a/acinclude.m4 b/acinclude.m4 index 343303447..5a48f0335
> > 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 4ea111f94..ad71f238e 100644
> > --- a/lib/dpif-netdev-private-extract.c
> > +++ b/lib/dpif-netdev-private-extract.c
> > @@ -77,20 +77,24 @@ dp_mfex_impl_get_default(void)  {
> >      atomic_uintptr_t *mfex_func = (void *)&default_mfex_func;
> >      static bool default_mfex_func_set = false;
> > +#ifdef MFEX_AUTOVALIDATOR_DEFAULT
> > +    int mfex_idx = MFEX_IMPL_AUTOVALIDATOR; #else
> >      int mfex_idx = MFEX_IMPL_SCALAR;
> > +#endif
> >
> >      /* For the first call, this will be choosen based on the
> >       * compile time flag and if nor flag is set it is set to
> >       * default scalar.
> >       */
> >      if (OVS_UNLIKELY(!default_mfex_func_set)) {
> > -        VLOG_INFO("Default MFEX implementation is %s.\n",
> > +
> > +        VLOG_INFO("Default miniflow extract implementation%s.\n",
> 
> Guess the text should have been updated in the patch introducing it.
> 
> >                    mfex_impls[mfex_idx].name);
> >          atomic_store_relaxed(mfex_func, (uintptr_t) mfex_impls
> >                               [mfex_idx].extract_func);
> >          default_mfex_func_set = true;
> >      }
> 
> Like my comment in patch1, I think this need/can move to
> dpif_miniflow_extract_init().
> 
> > -
> >      return default_mfex_func;
> >  }
> >
> > --
> > 2.25.1