[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?

Thanks,

Ben.


More information about the dev mailing list