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.
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.
Thanks,
Paolo
