[ovs-dev] [v12 09/16] docs/dpdk/bridge: Add dpif performance section.

Ferriter, Cian cian.ferriter at intel.com
Thu Jun 10 14:46:41 UTC 2021


Hi Ian,

Thanks for the review. My responses are inline.

> -----Original Message-----
> From: Stokes, Ian <ian.stokes at intel.com>
> Sent: Wednesday 2 June 2021 19:41
> To: Ferriter, Cian <cian.ferriter at intel.com>; ovs-dev at openvswitch.org; Van Haaren, Harry <harry.van.haaren at intel.com>
> Cc: i.maximets at ovn.org
> Subject: RE: [ovs-dev] [v12 09/16] docs/dpdk/bridge: Add dpif performance section.
> 
> > This section details how two new commands can be used to list and select
> > the different dpif implementations. It also details how a non default
> > dpif implementation can be tested with the OVS unit test suite.
> >
> > Add NEWS updates for the dpif-netdev.c refactor and the new dpif
> > implementations/commands.
> >
> > Signed-off-by: Cian Ferriter <cian.ferriter at intel.com>
> 
> Thanks for the patch Cian, some comments inline below.
> 
> One thing to flag is that for reviewing this is fine as a commit on it's own focusing on updating the documentation.
> 
> But I think it would be better to split this patch among the previous patches and update the documentation as features are added,
> code refactored etc. in the code base.
> 
> That way the documentation is an accurate reflection of the codebase at that particular commit for each change. Does this make
> sense?
> 

That makes sense. I'll fix this for the next version. I'll add the documentation relevant to each commit in that commit.

> >
> > ---
> >
> > v8:
> > - Merge NEWS file items into one Userspace Datapath: heading
> > ---
> >  Documentation/topics/dpdk/bridge.rst | 37
> > ++++++++++++++++++++++++++++
> >  NEWS                                 |  4 +++
> >  2 files changed, 41 insertions(+)
> >
> > diff --git a/Documentation/topics/dpdk/bridge.rst
> > b/Documentation/topics/dpdk/bridge.rst
> > index 526d5c959..ca90d7bdb 100644
> > --- a/Documentation/topics/dpdk/bridge.rst
> > +++ b/Documentation/topics/dpdk/bridge.rst
> > @@ -214,3 +214,40 @@ implementation ::
> >
> >  Compile OVS in debug mode to have `ovs_assert` statements error out if
> >  there is a mis-match in the DPCLS lookup implementation.
> > +
> > +Datapath Interface Performance
> > +------------------------------
> > +
> > +The datapath interface (DPIF) or dp_netdev_input() is responsible for
> > taking
> > +packets through the major components of the userspace datapath; such as
> > +miniflow_extract, EMC, SMC and DPCLS lookups, and a lot of the
> > performance
> > +stats associated with the datapath.
> > +
> > +Just like with the SIMD DPCLS work above, SIMD can be applied to the DPIF
> 
> Minor, would replace "work" with "feature".
> 

I'll fix this in the next version.

> > to
> > +improve performance.
> > +
> > +OVS provides multiple implementations of the DPIF. These can be listed
> > with the
> > +following command ::
> > +
> > +    $ ovs-appctl dpif-netdev/dpif-get
> > +    Available DPIF implementations:
> > +      dpif_scalar
> > +      dpif_avx512
> > +
> > +By default, dpif_scalar is used. The DPIF implementation can be selected by
> > +name ::
> > +
> > +    $ ovs-appctl dpif-netdev/dpif-set dpif_avx512
> > +    DPIF implementation set to dpif_avx512.
> > +
> > +    $ ovs-appctl dpif-netdev/dpif-set dpif_scalar
> > +    DPIF implementation set to dpif_scalar.
> > +
> 
> Maybe this is later in the series but have you given consideration to updating the man pages for these dpif-netdev commands also?
> 

I'm happy to add to man pages. I'm just wondering which man pages the information should go into. Do you mean the " lib/dpif-netdev-unixctl.man" file. If so, I'll add some info there.

> > +Running Unit Tests with AVX512 DPIF
> > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > +
> > +Since the AVX512 DPIF is disabled by default, a compile time option is
> > +available in order to test it with the OVS unit test suite. When building with
> > +a CPU that supports AVX512, use the following configure option ::
> > +
> > +    $ ./configure --enable-dpif-default-avx512
> 
> Would it be useful to provide the log output the user would expect to see during configuration to confirm AVX512 DPIF is in use?
> 

Good idea. I'll add that in the next version.

> > diff --git a/NEWS b/NEWS
> > index 3eab5f4fa..5e847a95e 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -7,6 +7,10 @@ Post-v2.15.0
> >         cases, e.g if all PMD threads are running on the same NUMA node.
> >       * Add a partial HWOL PMD statistic counting hits similar to existing
> >       * EMC/SMC/DPCLS stats.
> > +     * Refactor lib/dpif-netdev.c to multiple header files.
> > +     * Add avx512 implementation of dpif which can process non recirculated
> > +       packets. It supports partial HWOL, EMC, SMC and DPCLS lookups.
> > +     * Add commands to get and set the dpif implementations.
> 
> As flagged above these news entries could be split into the previous patches that made those changes.
> 

I'll fix this in the next version.

> Thanks
> Ian
> >     - ovs-ctl:
> >       * New option '--no-record-hostname' to disable hostname configuration
> >         in ovsdb on startup.
> > --
> > 2.31.1
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev



More information about the dev mailing list