From: Nathan Chancellor <nathan@kernel.org>
To: Paolo Abeni <pabeni@redhat.com>
Cc: netdev@vger.kernel.org,
Willem de Bruijn <willemdebruijn.kernel@gmail.com>,
"David S. Miller" <davem@davemloft.net>,
Eric Dumazet <edumazet@google.com>,
Jakub Kicinski <kuba@kernel.org>, Simon Horman <horms@kernel.org>,
David Ahern <dsahern@kernel.org>,
kuniyu@amazon.com
Subject: Re: [PATCH v4 net-next 2/2] udp_tunnel: use static call for GRO hooks when possible
Date: Thu, 20 Mar 2025 21:16:12 -0700 [thread overview]
Message-ID: <20250321041612.GA2679131@ax162> (raw)
In-Reply-To: <6fd1f9c7651151493ecab174e7b8386a1534170d.1741718157.git.pabeni@redhat.com>
Hi Paolo,
On Tue, Mar 11, 2025 at 09:42:29PM +0100, Paolo Abeni wrote:
> It's quite common to have a single UDP tunnel type active in the
> whole system. In such a case we can replace the indirect call for
> the UDP tunnel GRO callback with a static call.
>
> Add the related accounting in the control path and switch to static
> call when possible. To keep the code simple use a static array for
> the registered tunnel types, and size such array based on the kernel
> config.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> v3 -> v4:
> - fix CONFIG_FOE typo
> - avoid gaps in gro types array
> - drop WARN_ON in dummy_gro_rcv()
>
> v2 -> v3:
> - avoid unneeded checks in udp_tunnel_update_gro_rcv()
>
> v1 -> v2:
> - fix UDP_TUNNEL=n build
> ---
> include/net/udp_tunnel.h | 4 ++
> net/ipv4/udp_offload.c | 130 ++++++++++++++++++++++++++++++++++++-
> net/ipv4/udp_tunnel_core.c | 2 +
> 3 files changed, 135 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/udp_tunnel.h b/include/net/udp_tunnel.h
> index eda0f3e2f65fa..a7b230867eb14 100644
> --- a/include/net/udp_tunnel.h
> +++ b/include/net/udp_tunnel.h
> @@ -205,9 +205,11 @@ static inline void udp_tunnel_encap_enable(struct sock *sk)
>
> #if IS_ENABLED(CONFIG_NET_UDP_TUNNEL)
> void udp_tunnel_update_gro_lookup(struct net *net, struct sock *sk, bool add);
> +void udp_tunnel_update_gro_rcv(struct sock *sk, bool add);
> #else
> static inline void udp_tunnel_update_gro_lookup(struct net *net,
> struct sock *sk, bool add) {}
> +static inline void udp_tunnel_update_gro_rcv(struct sock *sk, bool add) {}
> #endif
>
> static inline void udp_tunnel_cleanup_gro(struct sock *sk)
> @@ -215,6 +217,8 @@ static inline void udp_tunnel_cleanup_gro(struct sock *sk)
> struct udp_sock *up = udp_sk(sk);
> struct net *net = sock_net(sk);
>
> + udp_tunnel_update_gro_rcv(sk, false);
> +
> if (!up->tunnel_list.pprev)
> return;
>
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index e36d8a234848f..088aa8cb8ac0c 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -15,6 +15,37 @@
> #include <net/udp_tunnel.h>
>
> #if IS_ENABLED(CONFIG_NET_UDP_TUNNEL)
> +
> +/*
> + * Dummy GRO tunnel callback, exists mainly to avoid dangling/NULL
> + * values for the udp tunnel static call.
> + */
> +static struct sk_buff *dummy_gro_rcv(struct sock *sk,
> + struct list_head *head,
> + struct sk_buff *skb)
> +{
> + NAPI_GRO_CB(skb)->flush = 1;
> + return NULL;
> +}
> +
> +typedef struct sk_buff *(*udp_tunnel_gro_rcv_t)(struct sock *sk,
> + struct list_head *head,
> + struct sk_buff *skb);
> +
> +struct udp_tunnel_type_entry {
> + udp_tunnel_gro_rcv_t gro_receive;
> + refcount_t count;
> +};
> +
> +#define UDP_MAX_TUNNEL_TYPES (IS_ENABLED(CONFIG_GENEVE) + \
> + IS_ENABLED(CONFIG_VXLAN) * 2 + \
> + IS_ENABLED(CONFIG_NET_FOU) * 2)
I am seeing a warning in one particular configuration in my matrix when
building with clang:
$ make -skj"$(nproc)" ARCH=mips LLVM=1 mrproper malta_defconfig net/ipv4/udp_offload.o
net/ipv4/udp_offload.c:130:8: warning: array index 0 is past the end of the array (that has type 'struct udp_tunnel_type_entry[0]') [-Warray-bounds]
130 | udp_tunnel_gro_types[0].gro_receive);
| ^ ~
include/linux/static_call.h:154:42: note: expanded from macro 'static_call_update'
154 | typeof(&STATIC_CALL_TRAMP(name)) __F = (func); \
| ^~~~
net/ipv4/udp_offload.c:47:1: note: array 'udp_tunnel_gro_types' declared here
47 | static struct udp_tunnel_type_entry udp_tunnel_gro_types[UDP_MAX_TUNNEL_TYPES];
| ^
1 warning generated.
GCC is more noisy but -Warray-bounds is not on by default yet.
$ make -skj"$(nproc)" ARCH=mips CROSS_COMPILE=mips-linux- KCFLAGS=-Warray-bounds mrproper malta_defconfig net/ipv4/udp_offload.o
In function 'udp_tunnel_update_gro_rcv',
inlined from 'udp_tunnel_update_gro_rcv' at net/ipv4/udp_offload.c:78:6:
net/ipv4/udp_offload.c:125:44: warning: array subscript <unknown> is outside array bounds of 'struct udp_tunnel_type_entry[0]' [-Warray-bounds=]
125 | *cur = udp_tunnel_gro_types[--udp_tunnel_gro_type_nr];
| ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~
net/ipv4/udp_offload.c: In function 'udp_tunnel_update_gro_rcv':
net/ipv4/udp_offload.c:47:37: note: while referencing 'udp_tunnel_gro_types'
47 | static struct udp_tunnel_type_entry udp_tunnel_gro_types[UDP_MAX_TUNNEL_TYPES];
| ^~~~~~~~~~~~~~~~~~~~
In function 'udp_tunnel_update_gro_rcv',
inlined from 'udp_tunnel_update_gro_rcv' at net/ipv4/udp_offload.c:78:6:
net/ipv4/udp_offload.c:111:17: warning: array subscript <unknown> is outside array bounds of 'struct udp_tunnel_type_entry[0]' [-Warray-bounds=]
111 | refcount_set(&cur->count, 1);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
net/ipv4/udp_offload.c: In function 'udp_tunnel_update_gro_rcv':
net/ipv4/udp_offload.c:47:37: note: while referencing 'udp_tunnel_gro_types'
47 | static struct udp_tunnel_type_entry udp_tunnel_gro_types[UDP_MAX_TUNNEL_TYPES];
| ^~~~~~~~~~~~~~~~~~~~
In file included from ./arch/mips/include/generated/asm/rwonce.h:1,
from include/linux/compiler.h:390,
from include/linux/array_size.h:5,
from include/linux/kernel.h:16,
from include/linux/skbuff.h:13,
from net/ipv4/udp_offload.c:9:
In function 'arch_atomic_set',
inlined from 'raw_atomic_set' at include/linux/atomic/atomic-arch-fallback.h:503:2,
inlined from 'atomic_set' at include/linux/atomic/atomic-instrumented.h:68:2,
inlined from 'refcount_set' at include/linux/refcount.h:134:2,
inlined from 'udp_tunnel_update_gro_rcv' at net/ipv4/udp_offload.c:111:3,
inlined from 'udp_tunnel_update_gro_rcv' at net/ipv4/udp_offload.c:78:6:
include/asm-generic/rwonce.h:55:37: warning: array subscript [3, 536870912] is outside array bounds of 'struct udp_tunnel_type_entry[0]' [-Warray-bounds=]
55 | *(volatile typeof(x) *)&(x) = (val); \
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
include/asm-generic/rwonce.h:61:9: note: in expansion of macro '__WRITE_ONCE'
61 | __WRITE_ONCE(x, val); \
| ^~~~~~~~~~~~
arch/mips/include/asm/atomic.h:34:9: note: in expansion of macro 'WRITE_ONCE'
34 | WRITE_ONCE(v->counter, i); \
| ^~~~~~~~~~
arch/mips/include/asm/atomic.h:37:1: note: in expansion of macro 'ATOMIC_OPS'
37 | ATOMIC_OPS(atomic, int)
| ^~~~~~~~~~
net/ipv4/udp_offload.c: In function 'udp_tunnel_update_gro_rcv':
net/ipv4/udp_offload.c:47:37: note: at offset 12 into object 'udp_tunnel_gro_types' of size 0
47 | static struct udp_tunnel_type_entry udp_tunnel_gro_types[UDP_MAX_TUNNEL_TYPES];
| ^~~~~~~~~~~~~~~~~~~~
In function 'udp_tunnel_update_gro_rcv',
inlined from 'udp_tunnel_update_gro_rcv' at net/ipv4/udp_offload.c:78:6:
net/ipv4/udp_offload.c:110:44: warning: array subscript <unknown> is outside array bounds of 'struct udp_tunnel_type_entry[0]' [-Warray-bounds=]
110 | cur = &udp_tunnel_gro_types[udp_tunnel_gro_type_nr++];
| ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~
net/ipv4/udp_offload.c: In function 'udp_tunnel_update_gro_rcv':
net/ipv4/udp_offload.c:47:37: note: while referencing 'udp_tunnel_gro_types'
47 | static struct udp_tunnel_type_entry udp_tunnel_gro_types[UDP_MAX_TUNNEL_TYPES];
| ^~~~~~~~~~~~~~~~~~~~
In file included from include/linux/bpf.h:30,
from include/linux/security.h:35,
from include/net/scm.h:9,
from include/linux/netlink.h:9,
from include/uapi/linux/neighbour.h:6,
from include/linux/netdevice.h:44,
from include/net/sock.h:46,
from include/linux/tcp.h:19,
from include/linux/ipv6.h:102,
from include/net/gro.h:8,
from net/ipv4/udp_offload.c:10:
In function 'udp_tunnel_update_gro_rcv',
inlined from 'udp_tunnel_update_gro_rcv' at net/ipv4/udp_offload.c:78:6:
net/ipv4/udp_offload.c:130:56: warning: array subscript 0 is outside array bounds of 'struct udp_tunnel_type_entry[0]' [-Warray-bounds=]
130 | udp_tunnel_gro_types[0].gro_receive);
| ~~~~~~~~~~~~~~~~~~~~^~~
include/linux/static_call.h:154:49: note: in definition of macro 'static_call_update'
154 | typeof(&STATIC_CALL_TRAMP(name)) __F = (func); \
| ^~~~
net/ipv4/udp_offload.c: In function 'udp_tunnel_update_gro_rcv':
net/ipv4/udp_offload.c:47:37: note: while referencing 'udp_tunnel_gro_types'
47 | static struct udp_tunnel_type_entry udp_tunnel_gro_types[UDP_MAX_TUNNEL_TYPES];
| ^~~~~~~~~~~~~~~~~~~~
In function 'udp_tunnel_update_gro_rcv',
inlined from 'udp_tunnel_update_gro_rcv' at net/ipv4/udp_offload.c:78:6:
net/ipv4/udp_offload.c:89:41: warning: array subscript i is outside array bounds of 'struct udp_tunnel_type_entry[0]' [-Warray-bounds=]
89 | if (udp_tunnel_gro_types[i].gro_receive == up->gro_receive)
| ~~~~~~~~~~~~~~~~~~~~^~~
net/ipv4/udp_offload.c: In function 'udp_tunnel_update_gro_rcv':
net/ipv4/udp_offload.c:47:37: note: while referencing 'udp_tunnel_gro_types'
47 | static struct udp_tunnel_type_entry udp_tunnel_gro_types[UDP_MAX_TUNNEL_TYPES];
| ^~~~~~~~~~~~~~~~~~~~
In function 'udp_tunnel_update_gro_rcv',
inlined from 'udp_tunnel_update_gro_rcv' at net/ipv4/udp_offload.c:78:6:
net/ipv4/udp_offload.c:90:31: warning: array subscript i is outside array bounds of 'struct udp_tunnel_type_entry[0]' [-Warray-bounds=]
90 | cur = &udp_tunnel_gro_types[i];
| ^~~~~~~~~~~~~~~~~~~~~~~~
net/ipv4/udp_offload.c: In function 'udp_tunnel_update_gro_rcv':
net/ipv4/udp_offload.c:47:37: note: while referencing 'udp_tunnel_gro_types'
47 | static struct udp_tunnel_type_entry udp_tunnel_gro_types[UDP_MAX_TUNNEL_TYPES];
| ^~~~~~~~~~~~~~~~~~~~
Should UDP_MAX_TUNNEL_TYPES be at least 1?
Cheers,
Nathan
> +DEFINE_STATIC_CALL(udp_tunnel_gro_rcv, dummy_gro_rcv);
> +static DEFINE_STATIC_KEY_FALSE(udp_tunnel_static_call);
> +static struct mutex udp_tunnel_gro_type_lock;
> +static struct udp_tunnel_type_entry udp_tunnel_gro_types[UDP_MAX_TUNNEL_TYPES];
> +static unsigned int udp_tunnel_gro_type_nr;
> static DEFINE_SPINLOCK(udp_tunnel_gro_lock);
>
> void udp_tunnel_update_gro_lookup(struct net *net, struct sock *sk, bool add)
> @@ -43,6 +74,101 @@ void udp_tunnel_update_gro_lookup(struct net *net, struct sock *sk, bool add)
> spin_unlock(&udp_tunnel_gro_lock);
> }
> EXPORT_SYMBOL_GPL(udp_tunnel_update_gro_lookup);
> +
> +void udp_tunnel_update_gro_rcv(struct sock *sk, bool add)
> +{
> + struct udp_tunnel_type_entry *cur = NULL;
> + struct udp_sock *up = udp_sk(sk);
> + int i, old_gro_type_nr;
> +
> + if (!up->gro_receive)
> + return;
> +
> + mutex_lock(&udp_tunnel_gro_type_lock);
> + for (i = 0; i < udp_tunnel_gro_type_nr; i++)
> + if (udp_tunnel_gro_types[i].gro_receive == up->gro_receive)
> + cur = &udp_tunnel_gro_types[i];
> +
> + old_gro_type_nr = udp_tunnel_gro_type_nr;
> + if (add) {
> + /*
> + * Update the matching entry, if found, or add a new one
> + * if needed
> + */
> + if (cur) {
> + refcount_inc(&cur->count);
> + goto out;
> + }
> +
> + if (unlikely(udp_tunnel_gro_type_nr == UDP_MAX_TUNNEL_TYPES)) {
> + pr_err_once("Too many UDP tunnel types, please increase UDP_MAX_TUNNEL_TYPES\n");
> + /* Ensure static call will never be enabled */
> + udp_tunnel_gro_type_nr = UDP_MAX_TUNNEL_TYPES + 2;
> + goto out;
> + }
> +
> + cur = &udp_tunnel_gro_types[udp_tunnel_gro_type_nr++];
> + refcount_set(&cur->count, 1);
> + cur->gro_receive = up->gro_receive;
> + } else {
> + /*
> + * The stack cleanups only successfully added tunnel, the
> + * lookup on removal should never fail.
> + */
> + if (WARN_ON_ONCE(!cur))
> + goto out;
> +
> + if (!refcount_dec_and_test(&cur->count))
> + goto out;
> +
> + /* avoid gaps, so that the enable tunnel has always id 0 */
> + *cur = udp_tunnel_gro_types[--udp_tunnel_gro_type_nr];
> + }
> +
> + if (udp_tunnel_gro_type_nr == 1) {
> + static_call_update(udp_tunnel_gro_rcv,
> + udp_tunnel_gro_types[0].gro_receive);
> + static_branch_enable(&udp_tunnel_static_call);
> + } else if (old_gro_type_nr == 1) {
> + static_branch_disable(&udp_tunnel_static_call);
> + static_call_update(udp_tunnel_gro_rcv, dummy_gro_rcv);
> + }
> +
> +out:
> + mutex_unlock(&udp_tunnel_gro_type_lock);
> +}
> +EXPORT_SYMBOL_GPL(udp_tunnel_update_gro_rcv);
> +
> +static void udp_tunnel_gro_init(void)
> +{
> + mutex_init(&udp_tunnel_gro_type_lock);
> +}
> +
> +static struct sk_buff *udp_tunnel_gro_rcv(struct sock *sk,
> + struct list_head *head,
> + struct sk_buff *skb)
> +{
> + if (static_branch_likely(&udp_tunnel_static_call)) {
> + if (unlikely(gro_recursion_inc_test(skb))) {
> + NAPI_GRO_CB(skb)->flush |= 1;
> + return NULL;
> + }
> + return static_call(udp_tunnel_gro_rcv)(sk, head, skb);
> + }
> + return call_gro_receive_sk(udp_sk(sk)->gro_receive, sk, head, skb);
> +}
> +
> +#else
> +
> +static void udp_tunnel_gro_init(void) {}
> +
> +static struct sk_buff *udp_tunnel_gro_rcv(struct sock *sk,
> + struct list_head *head,
> + struct sk_buff *skb)
> +{
> + return call_gro_receive_sk(udp_sk(sk)->gro_receive, sk, head, skb);
> +}
> +
> #endif
>
> static struct sk_buff *__skb_udp_tunnel_segment(struct sk_buff *skb,
> @@ -654,7 +780,7 @@ struct sk_buff *udp_gro_receive(struct list_head *head, struct sk_buff *skb,
>
> skb_gro_pull(skb, sizeof(struct udphdr)); /* pull encapsulating udp header */
> skb_gro_postpull_rcsum(skb, uh, sizeof(struct udphdr));
> - pp = call_gro_receive_sk(udp_sk(sk)->gro_receive, sk, head, skb);
> + pp = udp_tunnel_gro_rcv(sk, head, skb);
>
> out:
> skb_gro_flush_final(skb, pp, flush);
> @@ -804,5 +930,7 @@ int __init udpv4_offload_init(void)
> .gro_complete = udp4_gro_complete,
> },
> };
> +
> + udp_tunnel_gro_init();
> return inet_add_offload(&net_hotdata.udpv4_offload, IPPROTO_UDP);
> }
> diff --git a/net/ipv4/udp_tunnel_core.c b/net/ipv4/udp_tunnel_core.c
> index b5695826e57ad..c49fceea83139 100644
> --- a/net/ipv4/udp_tunnel_core.c
> +++ b/net/ipv4/udp_tunnel_core.c
> @@ -90,6 +90,8 @@ void setup_udp_tunnel_sock(struct net *net, struct socket *sock,
>
> udp_tunnel_encap_enable(sk);
>
> + udp_tunnel_update_gro_rcv(sock->sk, true);
> +
> if (!sk->sk_dport && !sk->sk_bound_dev_if && sk_saddr_any(sock->sk))
> udp_tunnel_update_gro_lookup(net, sock->sk, true);
> }
> --
> 2.48.1
>
next prev parent reply other threads:[~2025-03-21 4:16 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-03-11 20:42 [PATCH v4 net-next 0/2] udp_tunnel: GRO optimizations Paolo Abeni
2025-03-11 20:42 ` [PATCH v4 net-next 1/2] udp_tunnel: create a fastpath GRO lookup Paolo Abeni
2025-03-12 17:46 ` Willem de Bruijn
2025-03-11 20:42 ` [PATCH v4 net-next 2/2] udp_tunnel: use static call for GRO hooks when possible Paolo Abeni
2025-03-12 17:57 ` Willem de Bruijn
2025-03-21 4:16 ` Nathan Chancellor [this message]
2025-03-21 9:11 ` Paolo Abeni
2025-03-18 10:50 ` [PATCH v4 net-next 0/2] udp_tunnel: GRO optimizations patchwork-bot+netdevbpf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20250321041612.GA2679131@ax162 \
--to=nathan@kernel.org \
--cc=davem@davemloft.net \
--cc=dsahern@kernel.org \
--cc=edumazet@google.com \
--cc=horms@kernel.org \
--cc=kuba@kernel.org \
--cc=kuniyu@amazon.com \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=willemdebruijn.kernel@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).