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 <[email protected]> On Behalf Of Flavio Leitner > > Sent: Monday, June 21, 2021 5:39 PM > > To: Ferriter, Cian <[email protected]> > > Cc: [email protected]; Amber, Kumar <[email protected]>; > > [email protected] > > 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 <[email protected]> > > > > Sent: Sunday 20 June 2021 21:09 > > > > To: Ferriter, Cian <[email protected]> > > > > Cc: [email protected]; Amber, Kumar <[email protected]>; > > [email protected] > > > > 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 <[email protected]> > > > > > > > > > > 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 <[email protected]> > > > > > Co-authored-by: Cian Ferriter <[email protected]> > > > > > Signed-off-by: Cian Ferriter <[email protected]> > > > > > Co-authored-by: Kumar Amber <[email protected]> > > > > > Signed-off-by: Kumar Amber <[email protected]> > > > > > > > > > > --- > > <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/[email protected]/ > > > > > 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 > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev -- fbl _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
