> 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. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
