On 22.11.2019 13:27, David Marchand wrote: > On Fri, Nov 22, 2019 at 1:15 PM Ilya Maximets <i.maxim...@ovn.org> wrote: >> >> On 22.11.2019 12:42, David Marchand wrote: >>> On Thu, Nov 21, 2019 at 2:59 PM Ilya Maximets <i.maxim...@ovn.org> wrote: >>>> >>>> 'ol_flags' of DPDK mbuf could contain bits responsible for external >>>> or indirect buffers which are not actually offload flags in a common >>>> sense. Clearing/copying of these flags could lead to memory leaks of >>>> external memory chunks and crashes due to access to wrong memory. >>>> >>>> OVS should not clear these flags while resetting offloads and also >>>> should not copy them to the newly allocated packets. >>>> >>>> This change is required to support DPDK 19.11, as some drivers may >>>> return mbufs with these flags set. However, it might be good to do >>>> the same for DPDK 18.11, because these flags are present and should >>>> be taken into account. >>> >>> Did you consider inverting the logic? >>> >>> Rather than exclude a set of flags, I would touch/copy only the flags >>> that OVS uses/understands. >>> >>> When OVS uses a DPDK flag, it has an associated API and meaning wrt >>> the rte_mbuf and the data. >>> - when the flags are reset in OVS, that's because something has been >>> done to the data (and the checksums and the rss hash must be >>> reevaluated), >>> - when a buffer is copied, OVS copies the data and the context that >>> matters to OVS, >>> >>> Anything else should be discarded when copying to a new dp-packet. >> >> Yes, that was the first version I wanted to implement, i.e. to collect >> all the flags that OVS really uses and clear/copy only these flags. >> >> But then I started to think about 'ring' ports and that we might send >> mbufs with incorrect flags set after the packet modification to the >> external application. This doesn't sound good. Because if OVS doesn't >> use them doesn't mean that other applications doesn't. So, I've end up >> with the current logic with clearing everything except the memory layout >> flags. >> >> Does it make sense? What do you think? > > Before sending a packet through a dpdk ring, OVS will touch the data > and update the relevant flags as far as it is concerned. > > The application can read/touch those mbuf flags as long as it complies > with the DPDK APIs, this concerns both the flags that OVS is aware of, > and the rest. > So when getting this mbuf back, the flags should be valid. > > After this, OVS can do what it wants on the packet, it will only touch > the flags it understands. > > What am I missing?
I agree that OVS itself will work OK by only considering flags it really uses. I'm concerned about the second application that receives mbuf from the OVS via ring port. For example, I see that ixgbe driver almost unconditionally parses vlans and sets PKT_RX_VLAN along with mbuf->vlan_tci. If OVS will not clear this flag while sending to the ring port, other application that receives that mbuf will think that mbuf contains valid 'vlan_tci', which is not correct, because OVS might modify or strip that vlan from the packet before sending. > > >> BTW, it might be good to have a DPDK API for offload flags clear/get/copy. > > There is a rte_pktmbuf_copy that has been introduced recently, think a > helper copies metadata too... the best is to bother Olivier :-) > > >> >> BTW2, I don't really understand why memory layout flags are in ol_flags >> in the first place. I guess, there was just no room in mbuf structure? >> Is it possible that these flags will become regular or dynamic fields? > > I think it is the reason. > Again, Olivier ? _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
