On Mon, Mar 29, 2021 at 4:14 AM Paolo Abeni <pab...@redhat.com> wrote: > > On Fri, 2021-03-26 at 14:15 -0400, Willem de Bruijn wrote: > > On Thu, Mar 25, 2021 at 1:24 PM Paolo Abeni <pab...@redhat.com> wrote: > > > Currently the UDP protocol delivers GSO_FRAGLIST packets to > > > the sockets without the expected segmentation. > > > > > > This change addresses the issue introducing and maintaining > > > a couple of new fields to explicitly accept SKB_GSO_UDP_L4 > > > or GSO_FRAGLIST packets. Additionally updates udp_unexpected_gso() > > > accordingly. > > > > > > UDP sockets enabling UDP_GRO stil keep accept_udp_fraglist > > > zeroed. > > > > > > v1 -> v2: > > > - use 2 bits instead of a whole GSO bitmask (Willem) > > > > > > Fixes: 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.") > > > Signed-off-by: Paolo Abeni <pab...@redhat.com> > > > > This looks good to me in principle, thanks for the revision. > > > > I hadn't fully appreciated that gro_enabled implies accept_udp_l4, but > > not necessarily vice versa. > > > > It is equivalent to (accept_udp_l4 && !up->gro_receive), right? > > In this series, yes. > > > Could the extra bit be avoided with > > > > " > > + /* Prefer fraglist GRO unless target is a socket with UDP_GRO, > > + * which requires all but last segments to be of same gso_size, > > passed in cmsg */ > > if (skb->dev->features & NETIF_F_GRO_FRAGLIST) > > - NAPI_GRO_CB(skb)->is_flist = sk ? > > !udp_sk(sk)->gro_enabled: 1; > > + NAPI_GRO_CB(skb)->is_flist = sk ? > > (!udp_sk(sk)->gro_enabled || udp_sk(sk)->accept_udp_fraglist) : 1; > > This is not ovious at all to me. > > > + /* Apply transport layer GRO if forwarding is enabled or the > > flow lands at a local socket */ > > if ((!sk && (skb->dev->features & NETIF_F_GRO_UDP_FWD)) || > > (sk && udp_sk(sk)->gro_enabled && !up->encap_rcv) || > > NAPI_GRO_CB(skb)->is_flist) { > > pp = call_gro_receive(udp_gro_receive_segment, head, skb); > > return pp; > > } > > > > + /* Continue with tunnel GRO */ > > " > > > > .. not that the extra bit matters a lot. And these two conditions with > > gro_enabled are not very obvious. > > > > Just a thought. > > Overall looks more complex to me. I would keep the extra bit, unless > you have strong opinion.
Sounds good. > Side note: I was wondering about a follow-up to simplify the condition: > > if ((!sk && (skb->dev->features & NETIF_F_GRO_UDP_FWD)) || > (sk && udp_sk(sk)->gro_enabled && !up->encap_rcv) || > NAPI_GRO_CB(skb)->is_flist) { > > Since UDP sockets could process (segmenting as needed) unexpected GSO > packets, we could always do 'NETIF_F_GRO_UDP_FWD', when enabled on the > device level. The above becomes: > > if (skb->dev->features & NETIF_F_GRO_UDP_FWD) || > (sk && udp_sk(sk)->gro_enabled && !up->encap_rcv) || > NAPI_GRO_CB(skb)->is_flist) { > > which is hopefully more clear (and simpler). As said, non for this > series anyhow. UDP sockets can segment, but it is expensive. In this case I think the simplification is not worth the possible regression.