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

Amber, Kumar kumar.amber at intel.com
Thu Jun 24 17:50:55 UTC 2021


Hi Ian,

Thanks again , replies Inline

<Snipped>

> >         which uses CPU SIMD ISA to parse specific traffic profiles efficiently.
> >         Refer to the documentation for details on how to enable it at
> runtime.
> The NEWS section in general here seems wrong, seems to have features from
> other patches in both this patch series and the DPIF patch series added
> which shouldn't be the case.
> Only features added by this patch should be added to NEWS.

Taken care of in v5 split news with each commit.
> 
> > +     * Cache results for CPU ISA checks, reduces overhead on repeated
> lookups.
> > +     * 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
> > +       and automatically chooses the best miniflow implementation for that
> > +       traffic.
> > +     * Add AVX512 based optimized miniflow extract function for traffic
> type
> > +       IP/UDP.
> 
> The line  below is the only item that should be added to NEWS in this patch.

Fixed in v5.
> 
> > +     * 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.
> > @@ -35,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 d86268a1d..2008e5ee5 100644
> > --- a/lib/dpif-netdev-private-extract.c
> > +++ b/lib/dpif-netdev-private-extract.c
> > @@ -230,3 +230,27 @@ dpif_miniflow_extract_autovalidator(struct
> > dp_packet_batch *packets,
> >       */
> >      return 0;
> >  }
> > +
> > +/* Variable to hold the defaualt mfex implementation. */
> Typo in  default above.
> > +static miniflow_extract_func default_mfex_func = NULL;
> I'd prefer if above was declared at the top of the file rather than here.
> 

Fixed in v5. 
> > +
> > +void
> > +dpif_miniflow_extract_set_default(miniflow_extract_func func)
> > +{
> > +    default_mfex_func = func;
> > +}
> > +
> > +miniflow_extract_func
> > +dpif_miniflow_extract_get_default(void)
> > +{
> > +
> > +#ifdef MFEX_AUTOVALIDATOR_DEFAULT
> > +    ovs_assert(mfex_impls[0].extract_func ==
> > +               dpif_miniflow_extract_autovalidator);
> > +    VLOG_INFO("Default miniflow Extract implementation %s \n",
> > +              mfex_impls[0].name);
> > +    return mfex_impls[0].extract_func;
> > +#else
> > +    return default_mfex_func;
> > +#endif
> > +}
> > diff --git a/lib/dpif-netdev-private-extract.h b/lib/dpif-netdev-private-
> extract.h
> > index 3ada413bb..d8a284db7 100644
> > --- a/lib/dpif-netdev-private-extract.h
> > +++ b/lib/dpif-netdev-private-extract.h
> > @@ -118,4 +118,14 @@ mfex_study_traffic(struct dp_packet_batch
> *packets,
> >                     uint32_t keys_size, odp_port_t in_port,
> >                     void *pmd_handle);
> >
> > +/* Retrieve the default miniflow extract or auto-validator
> > + * based upon build time configuration choosen by the user. */
> Typo above, chosen.

Fixed in v5.
> 
> > +miniflow_extract_func
> > +dpif_miniflow_extract_get_default(void);
> > +
> > +/* Returns the default MFEX which is first ./configure selected, but can be
> > + * overridden at runtime. */
> > +void
> > +dpif_miniflow_extract_set_default(miniflow_extract_func func);
> > +
> >  #endif /* DPIF_NETDEV_AVX512_EXTRACT */
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index 4f4ab2790..716e0debf 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -1177,6 +1177,9 @@ dpif_miniflow_extract_impl_set(struct
> unixctl_conn
> > *conn, int argc,
> >
> >      ovs_mutex_unlock(&dp_netdev_mutex);
> >
> > +    /* Set the default implementation for PMD threads created in the
> future. */
> > +    dpif_miniflow_extract_set_default(*new_func);
> > +
> >      /* Reply with success to command. */
> >      struct ds reply = DS_EMPTY_INITIALIZER;
> >      ds_put_format(&reply, "Miniflow implementation set to %s.\n",
> mfex_name);
> > @@ -6282,8 +6285,8 @@ dp_netdev_configure_pmd(struct
> > dp_netdev_pmd_thread *pmd, struct dp_netdev *dp,
> >      /* Initialize DPIF function pointer to the default configured version. */
> >      pmd->netdev_input_func = dp_netdev_impl_get_default();
> >
> > -    /*Init default miniflow_extract function */
> > -    pmd->miniflow_extract_opt = NULL;
> > +    /* Init default miniflow_extract function */
> 
> Minor, missing period at end of comment.

Fixed in v5.
> 
> 
> BR
> Ian
> > +    pmd->miniflow_extract_opt = dpif_miniflow_extract_get_default();
> >
> >      /* init the 'flow_cache' since there is no
> >       * actual thread created for NON_PMD_CORE_ID. */
> > --
> > 2.25.1
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev



More information about the dev mailing list