On 24/07/2018 17:55, 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. >
Thanks for picking up the discussion. To me, it comes down to, is 128B not enough space for headers? Given the VXLAN example, it would be enough to push two headers. Is this too much of a constraint? What would be a good constraint? If we plan to support as many headers as the system's memory allows then we won't have much choice, and this option of malloc'ing from the system and copying to it might be the only one. My concerns would be on the performance penalty (as you mention), but also on users seeing weird performance benefits when using 2 VXLAN headers vs using 3 (for example). It would be yet another question to ask when debugging a system (how many headers are you pushing?) I see benefits on having a hard limit as well, and not allowing anything else to be pushed above those 128B, but since the 128B are set on DPDK at compile time, it seems this is calling for trouble in the future as if that changes between versions, we might start supporting less headers. In this case both this approach or the mbuf shifting would provide more reliability against future changes in DPDK's RTE_PKTMBUF_HEADROOM. Also, this is a bit aside, but ideally I would like to keep this discussion out of this patch series as I think this is creating confusion - this is an issue aside of the multi-segment work, it exists already for single mbufs DPDK packets. There's an RFC, "Don't resize DPBUF_DPDK packets.", which should be the ideal place. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
