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

Tue Jun 22 16:22:35 UTC 2021

Hi Harry,

All good points. I made a suggestion and left to the authors to
decide the best course of action. It was a suggestion to accommodate
everyone and to reduce the churn. That's all.

Anyways, my plan is to continue reviewing the patches and, as always,
I appreciate your support.

Thanks,
fbl

On Tue, Jun 22, 2021 at 11:10:32AM +0000, Van Haaren, Harry wrote:
> > -----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
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

-- 
fbl