Re: [PATCH v2 net-next 1/2] udp_tunnel: create a fastpath GRO lookup.

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Paolo Abeni wrote:
> On 3/8/25 7:37 PM, Willem de Bruijn wrote:
> > Paolo Abeni wrote:
> >> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> >> index 2c0725583be39..054d4d4a8927f 100644
> >> --- a/net/ipv4/udp_offload.c
> >> +++ b/net/ipv4/udp_offload.c
> >> @@ -12,6 +12,38 @@
> >>  #include <net/udp.h>
> >>  #include <net/protocol.h>
> >>  #include <net/inet_common.h>
> >> +#include <net/udp_tunnel.h>
> >> +
> >> +#if IS_ENABLED(CONFIG_NET_UDP_TUNNEL)
> >> +static DEFINE_SPINLOCK(udp_tunnel_gro_lock);
> >> +
> >> +void udp_tunnel_update_gro_lookup(struct net *net, struct sock *sk, bool add)
> >> +{
> >> +	bool is_ipv6 = sk->sk_family == AF_INET6;
> >> +	struct udp_sock *tup, *up = udp_sk(sk);
> >> +	struct udp_tunnel_gro *udp_tunnel_gro;
> >> +
> >> +	spin_lock(&udp_tunnel_gro_lock);
> >> +	udp_tunnel_gro = &net->ipv4.udp_tunnel_gro[is_ipv6];
> >> +	if (add)
> >> +		hlist_add_head(&up->tunnel_list, &udp_tunnel_gro->list);
> >> +	else
> >> +		hlist_del_init(&up->tunnel_list);
> >> +
> >> +	if (udp_tunnel_gro->list.first &&
> >> +	    !udp_tunnel_gro->list.first->next) {
> >> +		tup = hlist_entry(udp_tunnel_gro->list.first, struct udp_sock,
> >> +				  tunnel_list);
> >> +
> >> +		rcu_assign_pointer(udp_tunnel_gro->sk, (struct sock *)tup);
> > 
> > If the targeted case is a single tunnel, is it worth maintaining the list?
> > 
> > If I understand correctly, it is only there to choose a fall-back when the
> > current tup is removed. But complicates the code quite a bit.
> 
> I'll try to answer the questions on both patches here.
> 
> I guess in the end there is a relevant amount of personal preferences.
> Overall accounting is ~20 lines, IMHO it's not much.

In the next patch almost the entire body of udp_tunnel_update_gro_rcv
is there to maintain the refcount and list of tunnels.

Agreed that in the end it is subjective. Just that both patches
mention optimizing for the common case of a single tunnel type.
If you feel strongly, keep the list, of course.

Specific to the implementation

+	if (enabled && !old_enabled) {

Does enabled imply !old_enabled, once we get here? All paths
that do not modify udp_tunnel_gro_type_nr goto out.

+		for (i = 0; i < UDP_MAX_TUNNEL_TYPES; i++) {
+			cur = &udp_tunnel_gro_types[i];
+			if (refcount_read(&cur->count)) {
+				static_call_update(udp_tunnel_gro_rcv,
+						   cur->gro_receive);
+				static_branch_enable(&udp_tunnel_static_call);
+			}
+		}

Can you use avail, rather than walk the list again?
 
> I think we should at least preserve the optimization when the relevant
> tunnel is deleted and re-created, and the minimal accounting required
> for that will drop just a bunch of lines from
> udp_tunnel_update_gro_lookup(), while keeping all the hooking.
> 
> Additionally I think it would be surprising transiently applying some
> unusual configuration and as a side effect get lower performances up to
> the next reboot (lacking complete accounting).
> 
> > Just curious: what does tup stand for?
> 
> Tunnel Udp Pointer. Suggestion for better name welcome!
> 
> Thanks,
> 
> Paolo
>