[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20230214192257.47149-1-kuniyu@amazon.com>
Date: Tue, 14 Feb 2023 11:22:57 -0800
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <fhofmann@...udflare.com>
CC: <davem@...emloft.net>, <dsahern@...nel.org>, <edumazet@...gle.com>,
<fred@...udflare.com>, <kernel-team@...udflare.com>,
<kuba@...nel.org>, <netdev@...r.kernel.org>, <pabeni@...hat.com>,
<yoshfuji@...ux-ipv6.org>, <kuniyu@...zon.com>
Subject: Re: BUG: using __this_cpu_add() in preemptible in tcp_make_synack()
From: Frank Hofmann <fhofmann@...udflare.com>
Date: Tue, 14 Feb 2023 17:14:24 +0000
> Hi Eric,
>
> On Mon, Jan 23, 2023 at 3:49 PM 'Eric Dumazet' via
> kernel-team+notifications <kernel-team@...udflare.com> wrote:
> >
> > > On 1/18/23 11:07 AM, Eric Dumazet wrote:
> > [ ... ]
> > > > Thanks for the report
> > > >
> > > > I guess this part has been missed in commit 0a375c822497ed6a
> > > >
> > > > diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> > > > index 71d01cf3c13eb4bd3d314ef140568d2ffd6a499e..ba839e441450f195012a8d77cb9e5ed956962d2f
> > > > 100644
> > > > --- a/net/ipv4/tcp_output.c
> > > > +++ b/net/ipv4/tcp_output.c
> > > > @@ -3605,7 +3605,7 @@ struct sk_buff *tcp_make_synack(const struct
> > > > sock *sk, struct dst_entry *dst,
> [ ... ]
>
> we're still seeing this with a preempt-enabled kernel, in
> tcp_check_req() though, like:
>
> BUG: using __this_cpu_add() in preemptible [00000000] code: nginx-ssl/186233
> caller is tcp_check_req+0x49a/0x660
> CPU: 58 PID: 186233 Comm: nginx-ssl Kdump: loaded Tainted: G
> O 6.1.8-cloudflare-2023.1.16 #1
> Hardware name: ...
> Call Trace:
> <TASK>
> dump_stack_lvl+0x34/0x48
> check_preemption_disabled+0xdd/0xe0
> tcp_check_req+0x49a/0x660
> tcp_rcv_state_process+0xa3/0x1020
> ? tcp_sendmsg_locked+0x2a4/0xc50
> tcp_v4_do_rcv+0xc6/0x280
> __release_sock+0xb4/0xc0
> release_sock+0x2b/0x90
> tcp_sendmsg+0x33/0x40
> sock_sendmsg+0x5b/0x70
> sock_write_iter+0x97/0x100
> vfs_write+0x330/0x3d0
> ksys_write+0xab/0xe0
> ? syscall_trace_enter.constprop.0+0x164/0x170
> do_syscall_64+0x3b/0x90
> entry_SYSCALL_64_after_hwframe+0x4b/0xb5
>
> There's a notable number of "__"-marked stats updates in
> tcp_check_req(); I can't claim to understand the code well enough if
> all would have to be changed.
> The occurence is infrequent (we see about two a week).
How about converting __SNMP_XXX() only if the kernel is preemptible
like this ? Then we can keep the optimised code for other config.
untested & incomplete patch:
---8<---
diff --git a/include/net/snmp.h b/include/net/snmp.h
index 468a67836e2f..6ead61d9deb3 100644
--- a/include/net/snmp.h
+++ b/include/net/snmp.h
@@ -124,9 +124,13 @@ struct linux_tls_mib {
#define DECLARE_SNMP_STAT(type, name) \
extern __typeof__(type) __percpu *name
+#ifdef CONFIG_PREEMPT
+#define __SNMP_INC_STATS(mib, field) \
+ this_cpu_inc(mib->mibs[field])
+#else
#define __SNMP_INC_STATS(mib, field) \
__this_cpu_inc(mib->mibs[field])
-
+#endif
#define SNMP_INC_STATS_ATOMIC_LONG(mib, field) \
atomic_long_inc(&mib->mibs[field])
---8<---
Powered by blists - more mailing lists