On Mon, Sep 23, 2019 at 8:53 AM Willem de Bruijn
<willemdebruijn.ker...@gmail.com> wrote:
>
> On Fri, Sep 20, 2019 at 12:49 AM Steffen Klassert
> <steffen.klass...@secunet.com> wrote:
> >
> > This patch enables UDP GRO regardless if a GRO capable
> > socket is present. With this GRO is done by default
> > for the local input and forwarding path.
> >
> > Signed-off-by: Steffen Klassert <steffen.klass...@secunet.com>
> > struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
> > diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> > index a3908e55ed89..929b12fc7bc5 100644
> > --- a/net/ipv4/udp_offload.c
> > +++ b/net/ipv4/udp_offload.c
> > @@ -401,36 +401,25 @@ static struct sk_buff *udp_gro_receive_segment(struct
> > list_head *head,
> > return NULL;
> > }
> >
> > -INDIRECT_CALLABLE_DECLARE(struct sock *udp6_lib_lookup_skb(struct sk_buff
> > *skb,
> > - __be16 sport, __be16
> > dport));
> > struct sk_buff *udp_gro_receive(struct list_head *head, struct sk_buff
> > *skb,
> > - struct udphdr *uh, udp_lookup_t lookup)
> > + struct udphdr *uh, struct sock *sk)
> > {
> > struct sk_buff *pp = NULL;
> > struct sk_buff *p;
> > struct udphdr *uh2;
> > unsigned int off = skb_gro_offset(skb);
> > int flush = 1;
> > - struct sock *sk;
> >
> > - rcu_read_lock();
> > - sk = INDIRECT_CALL_INET(lookup, udp6_lib_lookup_skb,
> > - udp4_lib_lookup_skb, skb, uh->source,
> > uh->dest);
> > - if (!sk)
> > - goto out_unlock;
> > -
> > - if (udp_sk(sk)->gro_enabled) {
> > + if (!sk || !udp_sk(sk)->gro_receive) {
>
> Not critical, but the use of sk->gro_enabled and sk->gro_receive to
> signal whether sockets are willing to accept large packets or are udp
> tunnels, respectively, is subtle and possibly confusing.
>
> Wrappers udp_sock_is_tunnel and udp_sock_accepts_gso could perhaps
> help document the logic a bit.
>
> static inline bool udp_sock_is_tunnel(struct udp_sock *up)
> {
> return up->gro_receive;
> }
>
> And perhaps only pass a non-zero sk to udp_gro_receive if it is a
> tunnel and thus skips the new default path:
>
> static inline struct sock *sk = udp4_lookup_tunnel(const struct
> sk_buff *skb, __be16 sport, __be16_dport)
> {
> struct sock *sk;
>
> if (!static_branch_unlikely(&udp_encap_needed_key))
> return NULL;
>
> rcu_read_lock();
> sk = udp4_lib_lookup_skb(skb, source, dest);
> rcu_read_unlock();
>
> return udp_sock_is_tunnel(udp_sk(sk)) ? sk : NULL;
> }
>
> > pp = call_gro_receive(udp_gro_receive_segment, head, skb);
> > - rcu_read_unlock();
> > return pp;
> > }
>
> Just a suggestion. It may be too verbose as given.
.. and buggy, as it does not even test for NULL sk ;)
