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

Stokes, Ian ian.stokes at intel.com
Tue Jul 24 21:48:47 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?

No problem, I'll respin a new pull request now.

Thanks
Ian

> 
> Thanks,
> 
> Ben.


More information about the dev mailing list