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