Hi Ian, Thanks for the review. My responses are inline.
> -----Original Message----- > From: Stokes, Ian <[email protected]> > Sent: Wednesday 2 June 2021 19:41 > To: Ferriter, Cian <[email protected]>; [email protected]; Van > Haaren, Harry <[email protected]> > Cc: [email protected] > 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 <[email protected]> > > 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 > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
