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

From: David Marchand <david.marchand@redhat.com>
To: Olivier Matz <olivier.matz@6wind.com>
Cc: dev <dev@dpdk.org>, Maxime Coquelin <maxime.coquelin@redhat.com>,
	 Flavio Leitner <fbl@sysclose.org>,
	Ilya Maximets <i.maximets@ovn.org>,
	 Keith Wiles <keith.wiles@intel.com>
Subject: Re: [dpdk-dev] [PATCH 2/5] net/tap: do not touch Tx offload flags
Date: Wed, 28 Apr 2021 14:12:20 +0200	[thread overview]
Message-ID: <CAJFAV8zbhYRp8ntXg+poJAVKVm=_-yfTqDDk0CtQQnkfxPFYDg@mail.gmail.com> (raw)
In-Reply-To: <20210408075352.GR1650@platinum>

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