Re: [dpdk-dev] [PATCH v2 4/4] vhost: fix offload flags in Rx path

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

On Thu, Apr 29, 2021 at 3:31 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
> On 4/29/21 3:30 PM, Maxime Coquelin wrote:
> >> The vhost library current configures Tx offloading (PKT_TX_*) on any
> >> packet received from a guest virtio device which asks for some offloading.
> >>
> >> This is problematic, as Tx offloading is something that the application
> >> must ask for: the application needs to configure devices
> >> to support every used offloads (ip, tcp checksumming, tso..), and the
> >> various l2/l3/l4 lengths must be set following any processing that
> >> happened in the application itself.
> >>
> >> On the other hand, the received packets are not marked wrt current
> >> packet l3/l4 checksumming info.
> >>
> >> Copy virtio rx processing to fix those offload flags but accepting
> >> VIRTIO_NET_HDR_GSO_ECN and VIRTIO_NET_HDR_GSO_UDP too.
> >>
> >> The vhost example has been updated accordingly: TSO is applied to any
> >> packet marked LRO.
> >>
> >> Fixes: 859b480d5afd ("vhost: add guest offload setting")
>
> As I understand it, this change kind of break the ABI, but it is
> actually fixing a misuse of the mbuf API, so I think we should
> take this patch.

Indeed, this breaks the v21 ABI.

But the only usecase I can think of is an application using TSO /
checksum offloads *only* for traffic coming from vhost.
I say *only* for traffic coming from vhost, because to have this
application do TSO / checksum offloaing for traffic coming from a
physical port, it would comply with the mbuf API and set the PKT_TX_*
flags.

Apart from the example/vhost, I am not sure there is such an
application that only does v2v or v2p but _not_ p2v TSO / checksum
offloading.
(Note: I am unable to use this example... it seems unhappy with the
mlx5 port I use => FPE because this driver does not support vmdq o_O)


I see three options:
- fix the vhost library and break the ABI that only works in an
example (this current patch),
- maintain the v21 ABI
  * using symbol versioning, this adds no branch, recompiled
application use the new ABI, this can't be backported to 20.11,
  * keeping the current behavior by default, but introducing a new
flag that an application would pass to rte_vhost_driver_register().
This new flag triggers this current patch behavior but it would add an
additional branch per packets bulk in vhost dequeue path. This *could*
be backported to 20.11.


-- 
David Marchand