[ovs-dev] [v13 04/12] dpif-avx512: Add ISA implementation of dpif.

Van Haaren, Harry harry.van.haaren at intel.com
Tue Jun 22 11:10:32 UTC 2021


> -----Original Message-----
> From: dev <ovs-dev-bounces at openvswitch.org> On Behalf Of Flavio Leitner
> Sent: Monday, June 21, 2021 5:39 PM
> To: Ferriter, Cian <cian.ferriter at intel.com>
> Cc: ovs-dev at openvswitch.org; Amber, Kumar <kumar.amber at intel.com>;
> i.maximets at ovn.org
> Subject: Re: [ovs-dev] [v13 04/12] dpif-avx512: Add ISA implementation of dpif.
> 
> On Mon, Jun 21, 2021 at 04:13:12PM +0000, Ferriter, Cian wrote:
> > Hi Flavio,

Hi Flavio & All,

Responses inline below.

Regards, -Harry


> > Thanks for the review. My responses are inline.
> >
> > Cian
> >
> > > -----Original Message-----
> > > From: Flavio Leitner <fbl at sysclose.org>
> > > Sent: Sunday 20 June 2021 21:09
> > > To: Ferriter, Cian <cian.ferriter at intel.com>
> > > Cc: ovs-dev at openvswitch.org; Amber, Kumar <kumar.amber at intel.com>;
> i.maximets at ovn.org
> > > Subject: Re: [ovs-dev] [v13 04/12] dpif-avx512: Add ISA implementation of dpif.
> > >
> > >
> > > Hi,
> > >
> > > I am still reviewing the patch, but I thought worth to discuss
> > > few items below.
> > >
> > > On Thu, Jun 17, 2021 at 05:18:17PM +0100, Cian Ferriter wrote:
> > > > From: Harry van Haaren <harry.van.haaren at intel.com>
> > > >
> > > > This commit adds the AVX512 implementation of DPIF functionality,
> > > > specifically the dp_netdev_input_outer_avx512 function. This function
> > > > only handles outer (no re-circulations), and is optimized to use the
> > > > AVX512 ISA for packet batching and other DPIF work.
> > > >
> > > > Sparse is not able to handle the AVX512 intrinsics, causing compile
> > > > time failures, so it is disabled for this file.
> > > >
> > > > Signed-off-by: Harry van Haaren <harry.van.haaren at intel.com>
> > > > Co-authored-by: Cian Ferriter <cian.ferriter at intel.com>
> > > > Signed-off-by: Cian Ferriter <cian.ferriter at intel.com>
> > > > Co-authored-by: Kumar Amber <kumar.amber at intel.com>
> > > > Signed-off-by: Kumar Amber <kumar.amber at intel.com>
> > > >
> > > > ---

<snip patch contents>

> > Good point. This can be cleaned up. I've included lib/dpif-netdev-private-hwol.h in
> lib/dpif-netdev-private.h and removed the headers included by lib/dpif-netdev-
> private.h from lib/dpif-netdev-avx512.c.
> >
> > I'll move the prototype for dpcls_lookup() too, it makes more sense if it's in
> lib/dpif-netdev-private-dpcls.h.
> 
> Before you spend time on it, please consider if the refactoring is
> really required. I think refactoring the code usually is a nice
> thing to do when the result is a clean interface

Refactoring code can be done for multiple reasons, indeed cleaner interfaces
is a noble goal, as is avoiding code-duplication, and general tidying up.
This refactoring is not a "nice to have" it is required, let me explain:

In this patchset as a whole, an ISA optimized DPIF implementation is added.
Before this refactor all DPIF related components (EMC, SMC, PartialHWOL,
and DPIF structs like flow-stats, dp_netdev_flow, dp_netdev_pmd_thread etc)
are defined & used only in a single .c file. There is no modularity, and there is
no possibility to re-use any of those components outside the .c file where they
are declared.

This patchset refactors those components into separate header files, allowing
re-use outside the .c that they were previously limited to. This allows EMC and
SMC to be re-used, and the ISA optimized DPIF is now viable, due to code reuse.

The result of the patches is a much more modular codebase, and indeed it avoids
much code duplication. The interface is kept as consistent as possible with the
previous implementation. I agree the interface is not as clean as it could be, but
this is the pragmatic approach to improve modularity and avoid code duplication.


> but it seems that will conflict with some other patches being reviewed.

Yes, any code changes can cause rebase-conflicts. As you know, this is an unfortunate
but unavoidable step in general software development. Various parties that may have
conflicting patches have been CC-ed, so have been made aware of potential rebasing.


> Then, instead of you and/or others have to fix patches approaching the deadline
> maybe it would be better to leave optional refactoring to a follow up patch.

As stated above, the refactoring is required to avoid code-duplication. Without the
refactoring, EMC and SMC (as well as other components) are not available. The DPIF
cannot compile without the modularity introduced by these patches, hence this
refactoring is not optional, it is required.

Regarding deadlines, improving the modularity of the DPIF code (EMC/SMC) has been
present since the first version of the patchset in October of 2020:
https://patchwork.ozlabs.org/project/openvswitch/patch/20201006145437.35124-3-harry.van.haaren@intel.com/ 


> Another point to consider is that this refactoring is affecting an
> important part of OVS, so it will require careful review and perhaps
> additional follow ups until everyone is happy. If you can reduce that
> impact, it would reduce the risk which helps to get the work accepted.

Yes I agree that careful review is a good thing, but note that given these patches
have been available for review for >6 months now. Can you commit to reviewing
and providing specific feedback by the end of this week (by Friday 25th?)

As you are aware this work is due for inclusion in OVS 2.16, and the soft-freeze
deadline is estimated at next Thursday 1 July, based on the release timelines
documented here: https://docs.openvswitch.org/en/latest/internals/release-process/


> What do you think?
> 
> Thanks,
> --
> fbl
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list