Yuri Benditovich
2021-May-12 06:09 UTC
[PATCH 2/4] virtio-net: add support of UDP segmentation (USO) on the host
On Tue, May 11, 2021 at 8:48 PM Willem de Bruijn <willemdebruijn.kernel at gmail.com> wrote:> > On Tue, May 11, 2021 at 12:43 AM Yuri Benditovich > <yuri.benditovich at 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 at 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
2021-May-12 14:32 UTC
[PATCH 2/4] virtio-net: add support of UDP segmentation (USO) on the host
On Wed, May 12, 2021 at 2:10 AM Yuri Benditovich <yuri.benditovich at daynix.com> wrote:> > On Tue, May 11, 2021 at 8:48 PM Willem de Bruijn > <willemdebruijn.kernel at gmail.com> wrote: > > > > On Tue, May 11, 2021 at 12:43 AM Yuri Benditovich > > <yuri.benditovich at 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 at 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?