[ovs-dev] [PATCH v6 00/13] Support multi-segment mbufs

Ben Pfaff blp at ovn.org
Tue Jul 24 21:41:07 UTC 2018

On Tue, Jul 24, 2018 at 06:20:04PM +0000, Stokes, Ian wrote:
> > On Tue, Jul 24, 2018 at 07:55:39PM +0300, Ilya Maximets wrote:
> > > Hi.
> > > Just wanted to add some comments for the use-cases and testing
> > methodology.
> > > See inline.
> > >
> > > And I'm actually not sure if there any profit from this patch set?
> > > It looks like an internal mbuf handling rework that only degrades the
> > > performance and complicates the code.
> > >
> > > Please, don't consider me as merge blocker. I just want to understand
> > > why you think we need this 1200 LOCs?
> > >
> > > ---
> > > About 'resize()' related discussion:
> > > Maybe it's worth to allow dp_packet APIs to return different dp_packet.
> > > In this case we'll be able to just clone the packet to malloced memory
> > > and resize in cases of not enough headroom available.
> > > Like:
> > > 	packet = eth_push_vlan(packet, vlan->vlan_tpid, vlan->vlan_tci); or
> > > 	eth_push_vlan(&packet, vlan->vlan_tpid, vlan->vlan_tci);
> > >
> > > This will have a little performance penalty in compare with data
> > > shifting inside the mbuf, but will be much more elegant, and will
> > > allow to eliminate all the OVS_NOT_REACHED cases.
> > >
> > >
> > > Best regards, Ilya Maximets.
> > 
> > Hmm, this is the second person from whom I've heard serious misgivings
> > about this patch series.  Tiago, Ian, would you like to respond?  I'm a
> > little nervous about merging this patch series, especially relatively late
> > before branching, given that some people have technical objections to it.
> > 
> I think Tiago is currently responding to the queries Ilya has raised but I'd like to respond to this being included in the 2.10 release specifically.
> The feature has been in development for a while and up until Tiago had taken over the work 2 months had received little feedback.
> That being said we have had a number of people testing the feature over the last month+ who are familiar with OVS DPDK and have since signed off on it as it is enabling a common customer requirement for TSO/GRO offloads.
> This is just a stepping stone to TSO and GRO enablement for OVS DPDK which is called out as a feature gap between kernel Ovs and userspace OVS, the work will be continued from our side with the aim of for OVS 2.11 so any changes required can be refined.
> From a validation POV it's not breaking anything existing for OVS DPDK that I've have come across so the risk on that side is low.
> Would it be preferred to hold off merging this until the TSO/GRO aspects have been implemented also? I guess my view here was that I would like to enable users to begin using the multiseg feature with the 2.10 release and we can refine our approach if needed based on the community feedback.
> There are some interesting discussions such as changes to the dp_packet API above, but it's a pity it comes at a critical time window. I wonder are the API changes separate to this work? Impact would be felt outside of the multiseg also I would think as they are APIs. If there are changes required to the APIs I would envision we would update the mutliseg feature to work with these changes as they are introduced in the future.
> If you feel this is too much of a risk we can remove it from 2.10. I can respin a new pull request with the remaining patches.

After some consideration, I think we should leave this out of 2.10.  I'm
happy to merge it into master post-branch.  That way it'll have about 6
months to cook on master and possibly acquire these additional benefits
you mention.

Would you please respin the PR to leave this out?



More information about the dev mailing list