Re: [dpdk-dev] [PATCH 2/5] net/tap: do not touch Tx offload flags

[David Marchand <david.marchand@redhat.com>]

On Thu, Apr 8, 2021 at 9:53 AM Olivier Matz <olivier.matz@6wind.com> wrote:
>
> On Thu, Apr 01, 2021 at 11:52:40AM +0200, David Marchand wrote:
> > Tx offload flags are of the application responsibility.
> > Leave the mbuf alone and check for TSO where needed.
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>

Self nack on this patch.

>
> Maybe the problem being solved should be better described in the commit
> log. Is it a problem (other than cosmetic) to touch a mbuf in the Tx
> function of a driver, where we could expect that the mbuf is owned by
> the driver?
>
> The only problem I can think about is in case we transmit a direct mbuf
> whose refcnt is increased, but I wonder how much this is really
> supported: for instance, several drivers add vlans using
> rte_vlan_insert() in their Tx path.

This was my initial thought, as I suspected issues with applications
which keep a refcount on the mbuf.
But after more discussions offlist, it is hard to find a usecase for this.

The gso library already touches the ol_flags from the input mbuf and
this is documented in its API.

Plus, my patch also has an issue.
This library gives back an array of direct/indirect mbufs pointing at
the data from the original mbuf.
Those mbufs are populated with the original mbuf ol_flags but nothing
has been done on the checksum in this data.
The net/tap driver relies on this feature: mbuf_in is marked with
PKT_TX_TCP_CKSUM so that the generated mbufs given back by the rte_gso
library has this offload flag set and PKT_TX_TCP_SEG is not present
anymore.
Later in the net/tap tx handler, PKT_TX_TCP_CKSUM presence triggers
tcp checksum computation.
So this patch breaks the tso support in net/tap.


Just for the record.
As far as rte_vlan_insert() is concerned, it ensures that the mbuf is
not shared.
https://git.dpdk.org/dpdk/tree/lib/net/rte_ether.h#n352


-- 
David Marchand