Re: [PATCH v4] virtio-net: define UDP tunnel offload feature

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Jun 05, 2024 at 12:59:58PM +0200, Paolo Abeni wrote:
> On Mon, 2024-06-03 at 13:41 -0400, Willem de Bruijn wrote:
> > On Mon, Jun 3, 2024 at 10:17 AM Paolo Abeni <pabeni@redhat.com> wrote:
> > > On Fri, 2024-05-31 at 11:13 -0400, Willem de Bruijn wrote:
> > > > On Thu, May 30, 2024 at 1:15 PM Paolo Abeni <pabeni@redhat.com> wrote:
> > > > > On Thu, 2024-05-30 at 12:23 -0400, Willem de Bruijn wrote:
> > > > > > > > > > > inner transport header offset from the provided information, as it's
> > > > > > > > > > > currently the case for plain (not UDP tunneled) GSO packets.
> > > > > > > > > > > 
> > > > > > > > > > > The outer UDP header may carry a second checksum, which can be offloaded
> > > > > > > > > > > independently from the inner one. Since UDP tunnel checksum offload
> > > > > > > > 
> > > > > > > > Just require local checksum offload? Computing the outer checksum is
> > > > > > > > always cheap. Optional double checksum offload might add complexity
> > > > > > > > that is not needed, as it is so cheap.
> > > > > > > 
> > > > > > > The rationale behind the making the outer csum offload manadatory was
> > > > > > > to keep the specification change simple and prevent the exhaustion of
> > > > > > > the virtio_net_hdr flags and gso_type exhaustion. With this change,
> > > > > > > 'flags' has 4 bits used out of 8 and will raise to 5. 'gso_type' has 3
> > > > > > > 5 bits used and will raise to 6 (even if only 3 of them will be
> > > > > > > bitfields).
> > > > > > 
> > > > > > Perhaps we're talking about something different. I'm suggesting that
> > > > > > the outer checksum is not offloaded at all, because outer checksum
> > > > > > generation and validation is always cheap in software. This is the
> > > > > > premise of LCO.
> > > > > 
> > > > > If the guess produces GSO packets with outer checksum, and the virtio
> > > > > driver does not support the outer csum offload, the guest will have to
> > > > > do s/w segmentation. Even if computing the outer csum will be cheap,
> > > > > the segmentation will be bad enough by itself.
> > > > > 
> > > > > Am I missing something?
> > > > 
> > > > Do you mean virtio driver (in the guest) or device (in the host)?
> > > > 
> > > > All segmentation offload packets imply checksum offload. So the
> > > > virtual device must support checksum offload. In this case, of both
> > > > checksum fields.
> > > 
> > > I feel confused. I think the starting point was your request to avoid
> > > supporting (in the virtual device) outer checksum offload, and I read
> > > the above as quite the opposite?!?
> > 
> > Sorry for the confusion.
> > 
> > I agree that the outer transport offset is needed, after all.
> > 
> > Without segmentation offload, the outer checksum can be calculated
> > cheaply software.
> > 
> > But with segmentation offload, this either
> > - has to happen after segmentation, or
> > - the outer headers must already be setup for segments (GSO_PARTIAL)
> > 
> > The first approach requires the field.
> > 
> > The second is probably too much to ask of a virtio interface. Demanding
> > GSO_PARTIAL.
> > 
> > It would require the driver to
> > - split a tunnel GSO packet into a GSO and
> >   non-GSO pair if it payload length is not a multiple of mss.
> > - rewrite the outer headers to adjust the length fields
> > - compute the outer csum using LCO
> > 
> > It's not entirely crazy. The Linux kernel already can already do this for
> > drivers that advertise tunnel offload only as part of gso_partial_features.
> 
> I'm sorry for the latency.
> 
> Wrapping-up the above, I (mis?) understand you are ok with the proposed
> virtio_net_hdr layout changes.
> 
> WRT UDP tunnel outer header csum offload, I guess I could add (back,
> IIRC was there in a previous revision) the feature negotiation for the
> outer UDP header checksum. 
> 
> I think such feature negotiation makes sense only in the guest ->
> hypervisor direction, that is, device receive path, where we could hit
> possibly H/W implementation constraints. 
> 
> In the opposite direction it's just a matter of s/w support, it would
> be strange to me introduce UDP tunnel GSO and UDP tunnel outer checksum
> support separately.
> 
> WDYT, does the above makes sense to you? Or do you think we should
> negotiate the csum feature in both directions, for better symmetry?
> 
> Thanks!
> 
> Paolo
> 

We generally do negotiate csums in both directions, yes.
I'd rather we kept things consistent and we are not short on
feature bits.

-- 
MST