[ovs-dev] [PATCH] dp-packet: Fix clearing/copying of memory layout flags.

[David Marchand]

On Mon, Nov 25, 2019 at 12:23 PM Ilya Maximets <i.maximets at ovn.org> wrote:
>
> On 24.11.2019 21:38, David Marchand wrote:
> > On Fri, Nov 22, 2019 at 3:04 PM Ilya Maximets <i.maximets at ovn.org> wrote:
> >>>>> 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
> >
> > - Well, as I said, if OVS touches the data (pops a vlan), it must
> > update the flags (PKT_RX_VLAN).
>
> I don't think that DPDK API requires that anyhow.  Otherwise, DPDK
> must ensure that RX flags are valid after receiving mbuf from the
> DPDK port, but that is not true for ring pmd.

Not sure I understand your point.
If an offload flag is set, then the metadata and the data must be
consistent with it.

The ring driver is just a "passe-plat".
If packets received from a ring driver have wrong flags, it means they
were wrong when pushed to it.
And hopefully, DPDK drivers provide valid offload flags in mbufs.


> > This is overkill if the mbuf does not leave OVS, so it could be
> > cleared unconditionally before jumping in the ring.
>
> That is unclear why we should act differently for ring ports if we
> could just clear everything always and there is already generic
> API for that (dp_packet_reset_offload).

You are right, it should be generic.


> > Asking, since we know that the multiprocess stuff is not really that
> > robust and/or usable with OVS.
>
> I had an intention to deprecate ring ports in OVS, firstly because of
> the poor level of support and testing,  but I'm not sure if anyone uses
> them or not.  I could prepare the deprecation patch to collect some
> opinions.
>
> BTW, I'm not sure if I ever used ring ports in OVS, maybe once for ivshmem.
> And I'm not sure if anyone tests them any regularly (this is highly
> unlikely since ring tests are not included even in VSPERF).

+1




--
David Marchand