| Message ID | 20241024093742.87681-3-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | dbd5e2e79ed8653ac2ae255e42d1189283343a0c |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add noinline_for_tracing and apply it to tcp_drop_reason | expand |
From: Yafang Shao <laoar.shao@gmail.com> Date: Thu, 24 Oct 2024 17:37:42 +0800 > We previously hooked the tcp_drop_reason() function using BPF to monitor > TCP drop reasons. However, after upgrading our compiler from GCC 9 to GCC > 11, tcp_drop_reason() is now inlined, preventing us from hooking into it. > To address this, it would be beneficial to make noinline explicitly for > tracing. > > Link: https://lore.kernel.org/netdev/CANn89iJuShCmidCi_ZkYABtmscwbVjhuDta1MS5LxV_4H9tKOA@mail.gmail.com/ > Suggested-by: Eric Dumazet <edumazet@google.com> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > Cc: Menglong Dong <menglong8.dong@gmail.com> I saw a somewhat related post yesterday. https://x.com/__dxu/status/1849271647989068107 Daniel, could we apply your approach to this issue in the near future ? Thread link: https://lore.kernel.org/netdev/20241024093742.87681-1-laoar.shao@gmail.com/ > --- > net/ipv4/tcp_input.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c > index cc05ec1faac8..cf1e2e3a7407 100644 > --- a/net/ipv4/tcp_input.c > +++ b/net/ipv4/tcp_input.c > @@ -4894,8 +4894,8 @@ static bool tcp_ooo_try_coalesce(struct sock *sk, > return res; > } > > -static void tcp_drop_reason(struct sock *sk, struct sk_buff *skb, > - enum skb_drop_reason reason) > +noinline_for_tracing static void > +tcp_drop_reason(struct sock *sk, struct sk_buff *skb, enum skb_drop_reason reason) > { > sk_drops_add(sk, skb); > sk_skb_reason_drop(sk, skb, reason); > -- > 2.43.5 >
Hi Kuniyuki, On Thu, Oct 24, 2024, at 12:13 PM, Kuniyuki Iwashima wrote: > From: Yafang Shao <laoar.shao@gmail.com> > Date: Thu, 24 Oct 2024 17:37:42 +0800 >> We previously hooked the tcp_drop_reason() function using BPF to monitor >> TCP drop reasons. However, after upgrading our compiler from GCC 9 to GCC >> 11, tcp_drop_reason() is now inlined, preventing us from hooking into it. >> To address this, it would be beneficial to make noinline explicitly for >> tracing. >> >> Link: https://lore.kernel.org/netdev/CANn89iJuShCmidCi_ZkYABtmscwbVjhuDta1MS5LxV_4H9tKOA@mail.gmail.com/ >> Suggested-by: Eric Dumazet <edumazet@google.com> >> Signed-off-by: Yafang Shao <laoar.shao@gmail.com> >> Cc: Menglong Dong <menglong8.dong@gmail.com> > > I saw a somewhat related post yesterday. > https://x.com/__dxu/status/1849271647989068107 Glad to hear you're interested! > > Daniel, could we apply your approach to this issue in the near future ? > I suppose that depends on how you define "near". I'm being vague in that thread b/c we're still experimenting and have a couple things to look at still. But eventually, hopefully yes. Perhaps some time within a year if you had to press me. > Thread link: > https://lore.kernel.org/netdev/20241024093742.87681-1-laoar.shao@gmail.com/ > > >> --- >> net/ipv4/tcp_input.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c >> index cc05ec1faac8..cf1e2e3a7407 100644 >> --- a/net/ipv4/tcp_input.c >> +++ b/net/ipv4/tcp_input.c >> @@ -4894,8 +4894,8 @@ static bool tcp_ooo_try_coalesce(struct sock *sk, >> return res; >> } >> >> -static void tcp_drop_reason(struct sock *sk, struct sk_buff *skb, >> - enum skb_drop_reason reason) >> +noinline_for_tracing static void >> +tcp_drop_reason(struct sock *sk, struct sk_buff *skb, enum skb_drop_reason reason) >> { >> sk_drops_add(sk, skb); >> sk_skb_reason_drop(sk, skb, reason); >> -- >> 2.43.5 >>
Hi Yafang, On Thu, Oct 24, 2024, at 2:37 AM, Yafang Shao wrote: > We previously hooked the tcp_drop_reason() function using BPF to monitor > TCP drop reasons. However, after upgrading our compiler from GCC 9 to GCC > 11, tcp_drop_reason() is now inlined, preventing us from hooking into it. > To address this, it would be beneficial to make noinline explicitly for > tracing. It looks like kfree_skb() tracepoint has rx_sk field now. Added in c53795d48ee8 ("net: add rx_sk to trace_kfree_skb"). Between sk and skb, is there enough information to monitor TCP drops? Or do you need something particular about tcp_drop_reason()? Thanks, Daniel
From: "Daniel Xu" <dxu@dxuuu.xyz> Date: Thu, 24 Oct 2024 13:52:06 -0700 > Hi Kuniyuki, > > On Thu, Oct 24, 2024, at 12:13 PM, Kuniyuki Iwashima wrote: > > From: Yafang Shao <laoar.shao@gmail.com> > > Date: Thu, 24 Oct 2024 17:37:42 +0800 > >> We previously hooked the tcp_drop_reason() function using BPF to monitor > >> TCP drop reasons. However, after upgrading our compiler from GCC 9 to GCC > >> 11, tcp_drop_reason() is now inlined, preventing us from hooking into it. > >> To address this, it would be beneficial to make noinline explicitly for > >> tracing. > >> > >> Link: https://lore.kernel.org/netdev/CANn89iJuShCmidCi_ZkYABtmscwbVjhuDta1MS5LxV_4H9tKOA@mail.gmail.com/ > >> Suggested-by: Eric Dumazet <edumazet@google.com> > >> Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > >> Cc: Menglong Dong <menglong8.dong@gmail.com> > > > > I saw a somewhat related post yesterday. > > https://x.com/__dxu/status/1849271647989068107 > > Glad to hear you're interested! > > > > > Daniel, could we apply your approach to this issue in the near future ? > > > > I suppose that depends on how you define "near". I'm being vague > in that thread b/c we're still experimenting and have a couple things > to look at still. But eventually, hopefully yes. Perhaps some time > within a year if you had to press me. Sorry, I didn't intend to rush you. I was just curious if we would need to revert the patch soon after being merged, so please take your time :) Thanks!
On Fri, Oct 25, 2024 at 4:57 AM Daniel Xu <dxu@dxuuu.xyz> wrote: > > Hi Yafang, > > On Thu, Oct 24, 2024, at 2:37 AM, Yafang Shao wrote: > > We previously hooked the tcp_drop_reason() function using BPF to monitor > > TCP drop reasons. However, after upgrading our compiler from GCC 9 to GCC > > 11, tcp_drop_reason() is now inlined, preventing us from hooking into it. > > To address this, it would be beneficial to make noinline explicitly for > > tracing. > > It looks like kfree_skb() tracepoint has rx_sk field now. Added in > c53795d48ee8 ("net: add rx_sk to trace_kfree_skb"). This commit is helpful. Thank you for providing the information. I plan to backport it to our local kernel. > > Between sk and skb, is there enough information to monitor TCP drops? > Or do you need something particular about tcp_drop_reason()? There's nothing else specific to mention. The @rx_sk introduced in the commit you referred to will be beneficial to us. BTW, it would be fantastic if we could trace inline functions. Additionally, can your feature[0] also allow for live patching of inline functions? [0] https://x.com/__dxu/status/1849271647989068107
Hi, On 10/25/24 07:58, Yafang Shao wrote: > On Fri, Oct 25, 2024 at 4:57 AM Daniel Xu <dxu@dxuuu.xyz> wrote: >> On Thu, Oct 24, 2024, at 2:37 AM, Yafang Shao wrote: >>> We previously hooked the tcp_drop_reason() function using BPF to monitor >>> TCP drop reasons. However, after upgrading our compiler from GCC 9 to GCC >>> 11, tcp_drop_reason() is now inlined, preventing us from hooking into it. >>> To address this, it would be beneficial to make noinline explicitly for >>> tracing. >> >> It looks like kfree_skb() tracepoint has rx_sk field now. Added in >> c53795d48ee8 ("net: add rx_sk to trace_kfree_skb"). > > This commit is helpful. Thank you for providing the information. I > plan to backport it to our local kernel. > >> >> Between sk and skb, is there enough information to monitor TCP drops? >> Or do you need something particular about tcp_drop_reason()? > > There's nothing else specific to mention. The @rx_sk introduced in the > commit you referred to will be beneficial to us. The implications of the above statement are not clear to me. Do you mean this patchset is not needed anymore? Thanks! Paolo
On Thu, Oct 31, 2024 at 10:24 PM Paolo Abeni <pabeni@redhat.com> wrote: > > Hi, > > On 10/25/24 07:58, Yafang Shao wrote: > > On Fri, Oct 25, 2024 at 4:57 AM Daniel Xu <dxu@dxuuu.xyz> wrote: > >> On Thu, Oct 24, 2024, at 2:37 AM, Yafang Shao wrote: > >>> We previously hooked the tcp_drop_reason() function using BPF to monitor > >>> TCP drop reasons. However, after upgrading our compiler from GCC 9 to GCC > >>> 11, tcp_drop_reason() is now inlined, preventing us from hooking into it. > >>> To address this, it would be beneficial to make noinline explicitly for > >>> tracing. > >> > >> It looks like kfree_skb() tracepoint has rx_sk field now. Added in > >> c53795d48ee8 ("net: add rx_sk to trace_kfree_skb"). > > > > This commit is helpful. Thank you for providing the information. I > > plan to backport it to our local kernel. > > > >> > >> Between sk and skb, is there enough information to monitor TCP drops? > >> Or do you need something particular about tcp_drop_reason()? > > > > There's nothing else specific to mention. The @rx_sk introduced in the > > commit you referred to will be beneficial to us. > > The implications of the above statement are not clear to me. Do you mean > this patchset is not needed anymore? The introduction of a dedicated tcp_drop_reason() function is intended to enhance the traceability of TCP drops for the user, providing more convenient and detailed insights. Additionally, since tcp_drop_reason() does not impact the critical path, marking it as noinline is acceptable. For these reasons, I believe this patchset remains necessary.
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index cc05ec1faac8..cf1e2e3a7407 100644 --- a/net/ipv4/tcp_input.c +++ b/net/ipv4/tcp_input.c @@ -4894,8 +4894,8 @@ static bool tcp_ooo_try_coalesce(struct sock *sk, return res; } -static void tcp_drop_reason(struct sock *sk, struct sk_buff *skb, - enum skb_drop_reason reason) +noinline_for_tracing static void +tcp_drop_reason(struct sock *sk, struct sk_buff *skb, enum skb_drop_reason reason) { sk_drops_add(sk, skb); sk_skb_reason_drop(sk, skb, reason);
We previously hooked the tcp_drop_reason() function using BPF to monitor TCP drop reasons. However, after upgrading our compiler from GCC 9 to GCC 11, tcp_drop_reason() is now inlined, preventing us from hooking into it. To address this, it would be beneficial to make noinline explicitly for tracing. Link: https://lore.kernel.org/netdev/CANn89iJuShCmidCi_ZkYABtmscwbVjhuDta1MS5LxV_4H9tKOA@mail.gmail.com/ Suggested-by: Eric Dumazet <edumazet@google.com> Signed-off-by: Yafang Shao <laoar.shao@gmail.com> Cc: Menglong Dong <menglong8.dong@gmail.com> --- net/ipv4/tcp_input.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)