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

[Paolo Abeni <pabeni@redhat.com>]

On 3/10/25 4:51 AM, Willem de Bruijn wrote:
> 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.

You are right, this can be simplified a bit just checking for the
current `udp_tunnel_gro_type_nr` value.

> 
> +		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?

When we reach the above code due to a tunnel deletion, `avail` will
point to an unused array entry - that is "available" to store new tunnel
info. Instead we are looking for the only entry currently in use.

WRT the code complexity in patch 2/2, I attempted a few simplified
tracking approaches, and the only one leading to measurably simpler code
requires permanently (up to the next reboot) disabling the optimization
as soon as any additional tunnel of any type is created in the system
(child netns included).

I think the code complexity delta is worthy avoiding such restriction.

Thanks,

Paolo