diff mbox series

[2/4] virtio-net: add support of UDP segmentation (USO) on the host

Message ID 20210511044253.469034-3-yuri.benditovich@daynix.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series Add host USO support to TUN device | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Guessed tree name to be net-next
netdev/subject_prefix success Link
netdev/cc_maintainers success CCed 3 of 3 maintainers
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 2 this patch: 2
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 11 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 2 this patch: 2
netdev/header_inline success Link

Commit Message

Yuri Benditovich May 11, 2021, 4:42 a.m. UTC
Large UDP packet provided by the guest with GSO type set to
VIRTIO_NET_HDR_GSO_UDP_L4 will be divided to several UDP
packets according to the gso_size field.

Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
---
 include/linux/virtio_net.h | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Jason Wang May 11, 2021, 6:47 a.m. UTC | #1
在 2021/5/11 下午12:42, Yuri Benditovich 写道:
> Large UDP packet provided by the guest with GSO type set to
> VIRTIO_NET_HDR_GSO_UDP_L4 will be divided to several UDP
> packets according to the gso_size field.
>
> Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
> ---
>   include/linux/virtio_net.h | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index b465f8f3e554..4ecf9a1ca912 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -51,6 +51,11 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
>   			ip_proto = IPPROTO_UDP;
>   			thlen = sizeof(struct udphdr);
>   			break;
> +		case VIRTIO_NET_HDR_GSO_UDP_L4:
> +			gso_type = SKB_GSO_UDP_L4;
> +			ip_proto = IPPROTO_UDP;
> +			thlen = sizeof(struct udphdr);
> +			break;


This is only for rx, how about tx?

Thanks



>   		default:
>   			return -EINVAL;
>   		}
Yuri Benditovich May 11, 2021, 8:23 a.m. UTC | #2
On Tue, May 11, 2021 at 9:47 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2021/5/11 下午12:42, Yuri Benditovich 写道:
> > Large UDP packet provided by the guest with GSO type set to
> > VIRTIO_NET_HDR_GSO_UDP_L4 will be divided to several UDP
> > packets according to the gso_size field.
> >
> > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
> > ---
> >   include/linux/virtio_net.h | 5 +++++
> >   1 file changed, 5 insertions(+)
> >
> > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> > index b465f8f3e554..4ecf9a1ca912 100644
> > --- a/include/linux/virtio_net.h
> > +++ b/include/linux/virtio_net.h
> > @@ -51,6 +51,11 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
> >                       ip_proto = IPPROTO_UDP;
> >                       thlen = sizeof(struct udphdr);
> >                       break;
> > +             case VIRTIO_NET_HDR_GSO_UDP_L4:
> > +                     gso_type = SKB_GSO_UDP_L4;
> > +                     ip_proto = IPPROTO_UDP;
> > +                     thlen = sizeof(struct udphdr);
> > +                     break;
>
>
> This is only for rx, how about tx?

In terms of the guest this is only for TX.
Guest RX is a different thing, this is actually coalescing of
segmented UDP packets into a large one.
This feature is not defined in the virtio spec yet and the support of
it first of all depends on the OS.
For example: TCP LSO (guest TX) is supported almost by all the
versions of Windows.
TCP RSC (coalescing of TCP segments) is supported by Win 8 / Server 2012 and up.
UDP segmentation is supported by Windows kernels 1903+
UDP coalescing is defined by Windows kernels 2004+ and not supported
by the driver yet.

>
> Thanks
>
>
>
> >               default:
> >                       return -EINVAL;
> >               }
>
Jason Wang May 11, 2021, 8:31 a.m. UTC | #3
On Tue, May 11, 2021 at 4:24 PM Yuri Benditovich
<yuri.benditovich@daynix.com> wrote:
>
> On Tue, May 11, 2021 at 9:47 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> >
> > 在 2021/5/11 下午12:42, Yuri Benditovich 写道:
> > > Large UDP packet provided by the guest with GSO type set to
> > > VIRTIO_NET_HDR_GSO_UDP_L4 will be divided to several UDP
> > > packets according to the gso_size field.
> > >
> > > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
> > > ---
> > >   include/linux/virtio_net.h | 5 +++++
> > >   1 file changed, 5 insertions(+)
> > >
> > > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> > > index b465f8f3e554..4ecf9a1ca912 100644
> > > --- a/include/linux/virtio_net.h
> > > +++ b/include/linux/virtio_net.h
> > > @@ -51,6 +51,11 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
> > >                       ip_proto = IPPROTO_UDP;
> > >                       thlen = sizeof(struct udphdr);
> > >                       break;
> > > +             case VIRTIO_NET_HDR_GSO_UDP_L4:
> > > +                     gso_type = SKB_GSO_UDP_L4;
> > > +                     ip_proto = IPPROTO_UDP;
> > > +                     thlen = sizeof(struct udphdr);
> > > +                     break;
> >
> >
> > This is only for rx, how about tx?
>
> In terms of the guest this is only for TX.

So virtio_net_hdr_to_skb() can be called by all the followings:

1) receive_buf() which is guest RX.
2) tun_get_user() which is guest TX
3) tap_get_user() which is guest TX
4) {t}packet_send() which is userspace TX

So it touches for both RX and TX.

> Guest RX is a different thing, this is actually coalescing of
> segmented UDP packets into a large one.

Another case, the packet could be sent from another VM (like the UFO case).

Supporting that for both TX and RX and greatly improve the performance
of VM2VM traffic.

Thanks

> This feature is not defined in the virtio spec yet and the support of
> it first of all depends on the OS.
> For example: TCP LSO (guest TX) is supported almost by all the
> versions of Windows.
> TCP RSC (coalescing of TCP segments) is supported by Win 8 / Server 2012 and up.
> UDP segmentation is supported by Windows kernels 1903+
> UDP coalescing is defined by Windows kernels 2004+ and not supported
> by the driver yet.
>
> >
> > Thanks
> >
> >
> >
> > >               default:
> > >                       return -EINVAL;
> > >               }
> >
>
Willem de Bruijn May 11, 2021, 5:47 p.m. UTC | #4
On Tue, May 11, 2021 at 12:43 AM Yuri Benditovich
<yuri.benditovich@daynix.com> wrote:
>
> Large UDP packet provided by the guest with GSO type set to
> VIRTIO_NET_HDR_GSO_UDP_L4 will be divided to several UDP
> packets according to the gso_size field.
>
> Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
> ---
>  include/linux/virtio_net.h | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index b465f8f3e554..4ecf9a1ca912 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -51,6 +51,11 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
>                         ip_proto = IPPROTO_UDP;
>                         thlen = sizeof(struct udphdr);
>                         break;
> +               case VIRTIO_NET_HDR_GSO_UDP_L4:
> +                       gso_type = SKB_GSO_UDP_L4;
> +                       ip_proto = IPPROTO_UDP;
> +                       thlen = sizeof(struct udphdr);
> +                       break;

If adding a new VIRTIO_NET_HDR type I suggest adding separate IPv4 and
IPv6 variants, analogous to VIRTIO_NET_HDR_GSO_TCPV[46]. To avoid
having to infer protocol again, as for UDP fragmentation offload (the
retry case below this code).
Yuri Benditovich May 12, 2021, 6:09 a.m. UTC | #5
On Tue, May 11, 2021 at 8:48 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> On Tue, May 11, 2021 at 12:43 AM Yuri Benditovich
> <yuri.benditovich@daynix.com> wrote:
> >
> > Large UDP packet provided by the guest with GSO type set to
> > VIRTIO_NET_HDR_GSO_UDP_L4 will be divided to several UDP
> > packets according to the gso_size field.
> >
> > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
> > ---
> >  include/linux/virtio_net.h | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> > index b465f8f3e554..4ecf9a1ca912 100644
> > --- a/include/linux/virtio_net.h
> > +++ b/include/linux/virtio_net.h
> > @@ -51,6 +51,11 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
> >                         ip_proto = IPPROTO_UDP;
> >                         thlen = sizeof(struct udphdr);
> >                         break;
> > +               case VIRTIO_NET_HDR_GSO_UDP_L4:
> > +                       gso_type = SKB_GSO_UDP_L4;
> > +                       ip_proto = IPPROTO_UDP;
> > +                       thlen = sizeof(struct udphdr);
> > +                       break;
>
> If adding a new VIRTIO_NET_HDR type I suggest adding separate IPv4 and
> IPv6 variants, analogous to VIRTIO_NET_HDR_GSO_TCPV[46]. To avoid
> having to infer protocol again, as for UDP fragmentation offload (the
> retry case below this code).

Thank you for denoting this important point of distinguishing between v4 and v6.
Let's try to take a deeper look to see what is the correct thing to do
and please correct me if I'm wrong:
1. For USO we do not need to guess the protocol as it is used with
VIRTIO_NET_HDR_F_NEEDS_CSUM (unlike UFO) and the USO packets
transmitted by the guest are under the same clause as both
VIRTIO_NET_HDR_GSO_TCP, i.e. under if (hdr->flags &
VIRTIO_NET_HDR_F_NEEDS_CSUM) {
2. If we even define VIRTIO_NET_HDR_GSO_UDPv4_L4 and
VIRTIO_NET_HDR_GSO_UDPv6_L4 - both will be translated to
SKB_GSO_UDP_L4, so this information is immediately lost (the code will
look like:
case VIRTIO_NET_HDR_GSO_UDP4_L4: case VIRTIO_NET_HDR_GSO_UDP6_L4
    gso_type = SKB_GSO_UDP;

3. When we will define the respective guest features (like
VIRTIO_NET_F_HOST_USO4 VIRTIO_NET_F_HOST_USO6) we will need to
recreate the virtio_net header from the skb when both v4 and v6 have
the same SKB_GSO_UDP_L4, (see virtio_net_hdr_from_skb) and I'm not
sure whether somebody needs the exact v4 or v6 information on guest RX
path.
4. What is completely correct is that when we will start working with
the guest RX path we will need to define something like NETIF_F_USO4
and NETIF_F_USO6 and configure them according to exact guest offload
capabilities.
Do you agree?
Willem de Bruijn May 12, 2021, 2:32 p.m. UTC | #6
On Wed, May 12, 2021 at 2:10 AM Yuri Benditovich
<yuri.benditovich@daynix.com> wrote:
>
> On Tue, May 11, 2021 at 8:48 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > On Tue, May 11, 2021 at 12:43 AM Yuri Benditovich
> > <yuri.benditovich@daynix.com> wrote:
> > >
> > > Large UDP packet provided by the guest with GSO type set to
> > > VIRTIO_NET_HDR_GSO_UDP_L4 will be divided to several UDP
> > > packets according to the gso_size field.
> > >
> > > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
> > > ---
> > >  include/linux/virtio_net.h | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> > > index b465f8f3e554..4ecf9a1ca912 100644
> > > --- a/include/linux/virtio_net.h
> > > +++ b/include/linux/virtio_net.h
> > > @@ -51,6 +51,11 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
> > >                         ip_proto = IPPROTO_UDP;
> > >                         thlen = sizeof(struct udphdr);
> > >                         break;
> > > +               case VIRTIO_NET_HDR_GSO_UDP_L4:
> > > +                       gso_type = SKB_GSO_UDP_L4;
> > > +                       ip_proto = IPPROTO_UDP;
> > > +                       thlen = sizeof(struct udphdr);
> > > +                       break;
> >
> > If adding a new VIRTIO_NET_HDR type I suggest adding separate IPv4 and
> > IPv6 variants, analogous to VIRTIO_NET_HDR_GSO_TCPV[46]. To avoid
> > having to infer protocol again, as for UDP fragmentation offload (the
> > retry case below this code).
>
> Thank you for denoting this important point of distinguishing between v4 and v6.
> Let's try to take a deeper look to see what is the correct thing to do
> and please correct me if I'm wrong:
> 1. For USO we do not need to guess the protocol as it is used with
> VIRTIO_NET_HDR_F_NEEDS_CSUM (unlike UFO)

Enforcing that is a good start. We should also enforce that
skb->protocol is initialized to one of htons(ETH_P_IP) or
htons(ETH_P_IPV6), so that it does not have to be inferred by parsing.

These requirements were not enforced for previous values, and cannot
be introduced afterwards, which has led to have to add that extra code
to handle these obscure edge cases.

I agree that with well behaved configurations, the need for separate
_V4 and _V6 variants is not needed.

> and the USO packets
> transmitted by the guest are under the same clause as both
> VIRTIO_NET_HDR_GSO_TCP, i.e. under if (hdr->flags &
> VIRTIO_NET_HDR_F_NEEDS_CSUM) {
> 2. If we even define VIRTIO_NET_HDR_GSO_UDPv4_L4 and
> VIRTIO_NET_HDR_GSO_UDPv6_L4 - both will be translated to
> SKB_GSO_UDP_L4, so this information is immediately lost (the code will
> look like:
> case VIRTIO_NET_HDR_GSO_UDP4_L4: case VIRTIO_NET_HDR_GSO_UDP6_L4
>     gso_type = SKB_GSO_UDP;
>
> 3. When we will define the respective guest features (like
> VIRTIO_NET_F_HOST_USO4 VIRTIO_NET_F_HOST_USO6) we will need to
> recreate the virtio_net header from the skb when both v4 and v6 have
> the same SKB_GSO_UDP_L4, (see virtio_net_hdr_from_skb) and I'm not
> sure whether somebody needs the exact v4 or v6 information on guest RX
> path.

FWIW, it is good to keep in mind that virtio_net_hdr is also used
outside virtio, in both ingress and egress paths.

> 4. What is completely correct is that when we will start working with
> the guest RX path we will need to define something like NETIF_F_USO4
> and NETIF_F_USO6 and configure them according to exact guest offload
> capabilities.
> Do you agree?

I don't immediately see the need for advertising this device feature
on a per-protocol basis. Can you elaborate?
Yuri Benditovich May 12, 2021, 6:56 p.m. UTC | #7
On Wed, May 12, 2021 at 5:33 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> On Wed, May 12, 2021 at 2:10 AM Yuri Benditovich
> <yuri.benditovich@daynix.com> wrote:
> >
> > On Tue, May 11, 2021 at 8:48 PM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > >
> > > On Tue, May 11, 2021 at 12:43 AM Yuri Benditovich
> > > <yuri.benditovich@daynix.com> wrote:
> > > >
> > > > Large UDP packet provided by the guest with GSO type set to
> > > > VIRTIO_NET_HDR_GSO_UDP_L4 will be divided to several UDP
> > > > packets according to the gso_size field.
> > > >
> > > > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
> > > > ---
> > > >  include/linux/virtio_net.h | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> > > > index b465f8f3e554..4ecf9a1ca912 100644
> > > > --- a/include/linux/virtio_net.h
> > > > +++ b/include/linux/virtio_net.h
> > > > @@ -51,6 +51,11 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
> > > >                         ip_proto = IPPROTO_UDP;
> > > >                         thlen = sizeof(struct udphdr);
> > > >                         break;
> > > > +               case VIRTIO_NET_HDR_GSO_UDP_L4:
> > > > +                       gso_type = SKB_GSO_UDP_L4;
> > > > +                       ip_proto = IPPROTO_UDP;
> > > > +                       thlen = sizeof(struct udphdr);
> > > > +                       break;
> > >
> > > If adding a new VIRTIO_NET_HDR type I suggest adding separate IPv4 and
> > > IPv6 variants, analogous to VIRTIO_NET_HDR_GSO_TCPV[46]. To avoid
> > > having to infer protocol again, as for UDP fragmentation offload (the
> > > retry case below this code).
> >
> > Thank you for denoting this important point of distinguishing between v4 and v6.
> > Let's try to take a deeper look to see what is the correct thing to do
> > and please correct me if I'm wrong:
> > 1. For USO we do not need to guess the protocol as it is used with
> > VIRTIO_NET_HDR_F_NEEDS_CSUM (unlike UFO)
>
> Enforcing that is a good start. We should also enforce that
> skb->protocol is initialized to one of htons(ETH_P_IP) or
> htons(ETH_P_IPV6), so that it does not have to be inferred by parsing.

As this feature is new and is not used in any public release of any
misbehaving driver, probably it is enough to state in the spec that
VIRTIO_NET_HDR_F_NEEDS_CSUM is required for USO packets.
The spec states that the USO feature requires checksumming feature.

>
> These requirements were not enforced for previous values, and cannot
> be introduced afterwards, which has led to have to add that extra code
> to handle these obscure edge cases.
>
> I agree that with well behaved configurations, the need for separate
> _V4 and _V6 variants is not needed.
>
> > and the USO packets
> > transmitted by the guest are under the same clause as both
> > VIRTIO_NET_HDR_GSO_TCP, i.e. under if (hdr->flags &
> > VIRTIO_NET_HDR_F_NEEDS_CSUM) {
> > 2. If we even define VIRTIO_NET_HDR_GSO_UDPv4_L4 and
> > VIRTIO_NET_HDR_GSO_UDPv6_L4 - both will be translated to
> > SKB_GSO_UDP_L4, so this information is immediately lost (the code will
> > look like:
> > case VIRTIO_NET_HDR_GSO_UDP4_L4: case VIRTIO_NET_HDR_GSO_UDP6_L4
> >     gso_type = SKB_GSO_UDP;
> >
> > 3. When we will define the respective guest features (like
> > VIRTIO_NET_F_HOST_USO4 VIRTIO_NET_F_HOST_USO6) we will need to
This is my typo: VIRTIO_NET_F_GUEST_USO4...
> > recreate the virtio_net header from the skb when both v4 and v6 have
> > the same SKB_GSO_UDP_L4, (see virtio_net_hdr_from_skb) and I'm not
> > sure whether somebody needs the exact v4 or v6 information on guest RX
> > path.
>
> FWIW, it is good to keep in mind that virtio_net_hdr is also used
> outside virtio, in both ingress and egress paths.

Can you please elaborate in which scenarios we do not have any virtio
device in path but need virtio_net_hdr?

>
> > 4. What is completely correct is that when we will start working with
> > the guest RX path we will need to define something like NETIF_F_USO4
> > and NETIF_F_USO6 and configure them according to exact guest offload
> > capabilities.
> > Do you agree?
>
> I don't immediately see the need for advertising this device feature
> on a per-protocol basis. Can you elaborate?

Separate offload setting (controlled by the guest) for v4 and v6 in
guest RX path is mandatory, at least Windows always requires this for
any offload.
In this case it seems easy to have also virtio-net device features to
be indicated separately (the TAP/TUN should report its capabilities).
Willem de Bruijn May 12, 2021, 7:53 p.m. UTC | #8
On Wed, May 12, 2021 at 2:56 PM Yuri Benditovich
<yuri.benditovich@daynix.com> wrote:
>
> On Wed, May 12, 2021 at 5:33 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > On Wed, May 12, 2021 at 2:10 AM Yuri Benditovich
> > <yuri.benditovich@daynix.com> wrote:
> > >
> > > On Tue, May 11, 2021 at 8:48 PM Willem de Bruijn
> > > <willemdebruijn.kernel@gmail.com> wrote:
> > > >
> > > > On Tue, May 11, 2021 at 12:43 AM Yuri Benditovich
> > > > <yuri.benditovich@daynix.com> wrote:
> > > > >
> > > > > Large UDP packet provided by the guest with GSO type set to
> > > > > VIRTIO_NET_HDR_GSO_UDP_L4 will be divided to several UDP
> > > > > packets according to the gso_size field.
> > > > >
> > > > > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
> > > > > ---
> > > > >  include/linux/virtio_net.h | 5 +++++
> > > > >  1 file changed, 5 insertions(+)
> > > > >
> > > > > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> > > > > index b465f8f3e554..4ecf9a1ca912 100644
> > > > > --- a/include/linux/virtio_net.h
> > > > > +++ b/include/linux/virtio_net.h
> > > > > @@ -51,6 +51,11 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
> > > > >                         ip_proto = IPPROTO_UDP;
> > > > >                         thlen = sizeof(struct udphdr);
> > > > >                         break;
> > > > > +               case VIRTIO_NET_HDR_GSO_UDP_L4:
> > > > > +                       gso_type = SKB_GSO_UDP_L4;
> > > > > +                       ip_proto = IPPROTO_UDP;
> > > > > +                       thlen = sizeof(struct udphdr);
> > > > > +                       break;
> > > >
> > > > If adding a new VIRTIO_NET_HDR type I suggest adding separate IPv4 and
> > > > IPv6 variants, analogous to VIRTIO_NET_HDR_GSO_TCPV[46]. To avoid
> > > > having to infer protocol again, as for UDP fragmentation offload (the
> > > > retry case below this code).
> > >
> > > Thank you for denoting this important point of distinguishing between v4 and v6.
> > > Let's try to take a deeper look to see what is the correct thing to do
> > > and please correct me if I'm wrong:
> > > 1. For USO we do not need to guess the protocol as it is used with
> > > VIRTIO_NET_HDR_F_NEEDS_CSUM (unlike UFO)
> >
> > Enforcing that is a good start. We should also enforce that
> > skb->protocol is initialized to one of htons(ETH_P_IP) or
> > htons(ETH_P_IPV6), so that it does not have to be inferred by parsing.
>
> As this feature is new and is not used in any public release of any
> misbehaving driver, probably it is enough to state in the spec that
> VIRTIO_NET_HDR_F_NEEDS_CSUM is required for USO packets.
> The spec states that the USO feature requires checksumming feature.

The spec is not sufficient. These rules need to be enforced in the
kernel code, too.

> >
> > These requirements were not enforced for previous values, and cannot
> > be introduced afterwards, which has led to have to add that extra code
> > to handle these obscure edge cases.
> >
> > I agree that with well behaved configurations, the need for separate
> > _V4 and _V6 variants is not needed.
> >
> > > and the USO packets
> > > transmitted by the guest are under the same clause as both
> > > VIRTIO_NET_HDR_GSO_TCP, i.e. under if (hdr->flags &
> > > VIRTIO_NET_HDR_F_NEEDS_CSUM) {
> > > 2. If we even define VIRTIO_NET_HDR_GSO_UDPv4_L4 and
> > > VIRTIO_NET_HDR_GSO_UDPv6_L4 - both will be translated to
> > > SKB_GSO_UDP_L4, so this information is immediately lost (the code will
> > > look like:
> > > case VIRTIO_NET_HDR_GSO_UDP4_L4: case VIRTIO_NET_HDR_GSO_UDP6_L4
> > >     gso_type = SKB_GSO_UDP;
> > >
> > > 3. When we will define the respective guest features (like
> > > VIRTIO_NET_F_HOST_USO4 VIRTIO_NET_F_HOST_USO6) we will need to
> This is my typo: VIRTIO_NET_F_GUEST_USO4...
> > > recreate the virtio_net header from the skb when both v4 and v6 have
> > > the same SKB_GSO_UDP_L4, (see virtio_net_hdr_from_skb) and I'm not
> > > sure whether somebody needs the exact v4 or v6 information on guest RX
> > > path.
> >
> > FWIW, it is good to keep in mind that virtio_net_hdr is also used
> > outside virtio, in both ingress and egress paths.
>
> Can you please elaborate in which scenarios we do not have any virtio
> device in path but need virtio_net_hdr?

Packet sockets, tuntap.

> > > 4. What is completely correct is that when we will start working with
> > > the guest RX path we will need to define something like NETIF_F_USO4
> > > and NETIF_F_USO6 and configure them according to exact guest offload
> > > capabilities.
> > > Do you agree?
> >
> > I don't immediately see the need for advertising this device feature
> > on a per-protocol basis. Can you elaborate?
>
> Separate offload setting (controlled by the guest) for v4 and v6 in
> guest RX path is mandatory, at least Windows always requires this for
> any offload.
> In this case it seems easy to have also virtio-net device features to
> be indicated separately (the TAP/TUN should report its capabilities).

Ah, ok.
diff mbox series

Patch

diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index b465f8f3e554..4ecf9a1ca912 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -51,6 +51,11 @@  static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
 			ip_proto = IPPROTO_UDP;
 			thlen = sizeof(struct udphdr);
 			break;
+		case VIRTIO_NET_HDR_GSO_UDP_L4:
+			gso_type = SKB_GSO_UDP_L4;
+			ip_proto = IPPROTO_UDP;
+			thlen = sizeof(struct udphdr);
+			break;
 		default:
 			return -EINVAL;
 		}