Re: [PATCH 0/2] Objpool performance improvements
On Fri, Apr 26, 2024 at 7:25 AM Masami Hiramatsu wrote: > > Hi Andrii, > > On Wed, 24 Apr 2024 14:52:12 -0700 > Andrii Nakryiko wrote: > > > Improve objpool (used heavily in kretprobe hot path) performance with two > > improvements: > > - inlining performance critical objpool_push()/objpool_pop() operations; > > - avoiding re-calculating relatively expensive nr_possible_cpus(). > > Thanks for optimizing objpool. Both looks good to me. Great, thanks for applying. > > BTW, I don't intend to stop this short-term optimization attempts, > but I would like to ask you check the new fgraph based fprobe > (kretprobe-multi)[1] instead of objpool/rethook. You can see that I did :) There is tons of code and I'm not familiar with internals of function_graph infra, but you can see I did run benchmarks, so I'm paying attention. But as for the objpool itself, I think it's a performant and useful internal building block, and we might use it outside of rethook as well, so I think making it as fast as possible is good regardless. > > [1] > https://lore.kernel.org/all/171318533841.254850.15841395205784342850.stgit@devnote2/ > > I'm considering to obsolete the kretprobe (and rethook) with fprobe > and eventually remove it. Those have similar feature and we should > choose safer one. > Yep, I had a few more semi-ready patches, but I'll hold off for now given this move to function graph, plus some of the changes that Jiri is making in multi-kprobe code. I'll wait for things to settle down a bit before looking again. But just to give you some context, I'm an author of retsnoop tool, and one of the killer features of it is LBR capture in kretprobes, which is a tremendous help in investigating kernel failures, especially in unfamiliar code (LBR allows to "look back" and figure out "how did we get to this condition" after the fact). And so it's important to minimize the amount of wasted LBR records between some kernel function returns error (and thus is "an interesting event" and BPF program that captures LBR is triggered). Big part of that is ftrace/fprobe/rethook infra, so I was looking into making that part as "minimal" as possible, in the sense of eliminating as many function calls and jump as possible. This has an added benefit of making this hot path faster, but my main motivation is LBR. Anyways, just a bit of context for some of the other patches (like inlining arch_rethook_trampoline_callback). > Thank you, > > > > > These opportunities were found when benchmarking and profiling kprobes and > > kretprobes with BPF-based benchmarks. See individual patches for details and > > results. > > > > Andrii Nakryiko (2): > > objpool: enable inlining objpool_push() and objpool_pop() operations > > objpool: cache nr_possible_cpus() and avoid caching nr_cpu_ids > > > > include/linux/objpool.h | 105 +++-- > > lib/objpool.c | 112 +++- > > 2 files changed, 107 insertions(+), 110 deletions(-) > > > > -- > > 2.43.0 > > > > > -- > Masami Hiramatsu (Google)
Re: [PATCH 0/2] Objpool performance improvements
Hi Andrii, On Wed, 24 Apr 2024 14:52:12 -0700 Andrii Nakryiko wrote: > Improve objpool (used heavily in kretprobe hot path) performance with two > improvements: > - inlining performance critical objpool_push()/objpool_pop() operations; > - avoiding re-calculating relatively expensive nr_possible_cpus(). Thanks for optimizing objpool. Both looks good to me. BTW, I don't intend to stop this short-term optimization attempts, but I would like to ask you check the new fgraph based fprobe (kretprobe-multi)[1] instead of objpool/rethook. [1] https://lore.kernel.org/all/171318533841.254850.15841395205784342850.stgit@devnote2/ I'm considering to obsolete the kretprobe (and rethook) with fprobe and eventually remove it. Those have similar feature and we should choose safer one. Thank you, > > These opportunities were found when benchmarking and profiling kprobes and > kretprobes with BPF-based benchmarks. See individual patches for details and > results. > > Andrii Nakryiko (2): > objpool: enable inlining objpool_push() and objpool_pop() operations > objpool: cache nr_possible_cpus() and avoid caching nr_cpu_ids > > include/linux/objpool.h | 105 +++-- > lib/objpool.c | 112 +++- > 2 files changed, 107 insertions(+), 110 deletions(-) > > -- > 2.43.0 > -- Masami Hiramatsu (Google)
Re: [PATCH net-next v9 0/7] Implement reset reason mechanism to detect
Hello: This series was applied to netdev/net-next.git (main) by Paolo Abeni : On Thu, 25 Apr 2024 11:13:33 +0800 you wrote: > From: Jason Xing > > In production, there are so many cases about why the RST skb is sent but > we don't have a very convenient/fast method to detect the exact underlying > reasons. > > RST is implemented in two kinds: passive kind (like tcp_v4_send_reset()) > and active kind (like tcp_send_active_reset()). The former can be traced > carefully 1) in TCP, with the help of drop reasons, which is based on > Eric's idea[1], 2) in MPTCP, with the help of reset options defined in > RFC 8684. The latter is relatively independent, which should be > implemented on our own, such as active reset reasons which can not be > replace by skb drop reason or something like this. > > [...] Here is the summary with links: - [net-next,v9,1/7] net: introduce rstreason to detect why the RST is sent https://git.kernel.org/netdev/net-next/c/5cb2cb3cb20c - [net-next,v9,2/7] rstreason: prepare for passive reset https://git.kernel.org/netdev/net-next/c/6be49deaa095 - [net-next,v9,3/7] rstreason: prepare for active reset https://git.kernel.org/netdev/net-next/c/5691276b39da - [net-next,v9,4/7] tcp: support rstreason for passive reset https://git.kernel.org/netdev/net-next/c/120391ef9ca8 - [net-next,v9,5/7] mptcp: support rstreason for passive reset https://git.kernel.org/netdev/net-next/c/3e140491dd80 - [net-next,v9,6/7] mptcp: introducing a helper into active reset logic https://git.kernel.org/netdev/net-next/c/215d40248bde - [net-next,v9,7/7] rstreason: make it work in trace world https://git.kernel.org/netdev/net-next/c/b533fb9cf4f7 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
Re: [PATCH net-next v9 0/7] Implement reset reason mechanism to detect
On Thu, Apr 25, 2024 at 5:13 AM Jason Xing wrote: > > From: Jason Xing > > In production, there are so many cases about why the RST skb is sent but > we don't have a very convenient/fast method to detect the exact underlying > reasons. > > RST is implemented in two kinds: passive kind (like tcp_v4_send_reset()) > and active kind (like tcp_send_active_reset()). The former can be traced > carefully 1) in TCP, with the help of drop reasons, which is based on > Eric's idea[1], 2) in MPTCP, with the help of reset options defined in > RFC 8684. The latter is relatively independent, which should be > implemented on our own, such as active reset reasons which can not be > replace by skb drop reason or something like this. > > In this series, I focus on the fundamental implement mostly about how > the rstreason mechanism works and give the detailed passive part as an > example, not including the active reset part. In future, we can go > further and refine those NOT_SPECIFIED reasons. > > Here are some examples when tracing: > -0 [002] ..s1. 1830.262425: tcp_send_reset: skbaddr=x > skaddr=x src=x dest=x state=x reason=NOT_SPECIFIED > -0 [002] ..s1. 1830.262425: tcp_send_reset: skbaddr=x > skaddr=x src=x dest=x state=x reason=NO_SOCKET > > [1] > Link: > https://lore.kernel.org/all/CANn89iJw8x-LqgsWOeJQQvgVg6DnL5aBRLi10QN2WBdr+X4k=w...@mail.gmail.com/ Reviewed-by: Eric Dumazet Thanks !
Re: [PATCH net-next v9 7/7] rstreason: make it work in trace world
On Thu, Apr 25, 2024 at 5:14 AM Jason Xing wrote: > > From: Jason Xing > > At last, we should let it work by introducing this reset reason in > trace world. > > One of the possible expected outputs is: > ... tcp_send_reset: skbaddr=xxx skaddr=xxx src=xxx dest=xxx > state=TCP_ESTABLISHED reason=NOT_SPECIFIED > > Signed-off-by: Jason Xing > Reviewed-by: Steven Rostedt (Google) > --- Reviewed-by: Eric Dumazet
Re: [PATCH net-next v9 6/7] mptcp: introducing a helper into active reset logic
On Thu, Apr 25, 2024 at 5:14 AM Jason Xing wrote: > > From: Jason Xing > > Since we have mapped every mptcp reset reason definition in enum > sk_rst_reason, introducing a new helper can cover some missing places > where we have already set the subflow->reset_reason. > > Note: using SK_RST_REASON_NOT_SPECIFIED is the same as > SK_RST_REASON_MPTCP_RST_EUNSPEC. They are both unknown. So we can convert > it directly. > > Suggested-by: Paolo Abeni > Signed-off-by: Jason Xing > Reviewed-by: Matthieu Baerts (NGI0) Reviewed-by: Eric Dumazet
Re: [PATCH net-next v9 5/7] mptcp: support rstreason for passive reset
On Thu, Apr 25, 2024 at 5:14 AM Jason Xing wrote: > > From: Jason Xing > > It relies on what reset options in the skb are as rfc8684 says. Reusing > this logic can save us much energy. This patch replaces most of the prior > NOT_SPECIFIED reasons. > > Signed-off-by: Jason Xing > Reviewed-by: Matthieu Baerts (NGI0) Reviewed-by: Eric Dumazet
Re: [PATCH net-next v9 3/7] rstreason: prepare for active reset
On Thu, Apr 25, 2024 at 5:14 AM Jason Xing wrote: > > From: Jason Xing > > Like what we did to passive reset: > only passing possible reset reason in each active reset path. > > No functional changes. > > Signed-off-by: Jason Xing > Acked-by: Matthieu Baerts (NGI0) Reviewed-by: Eric Dumazet
Re: [PATCH net-next v9 2/7] rstreason: prepare for passive reset
On Thu, Apr 25, 2024 at 5:14 AM Jason Xing wrote: > > From: Jason Xing > > Adjust the parameter and support passing reason of reset which > is for now NOT_SPECIFIED. No functional changes. > > Signed-off-by: Jason Xing > Acked-by: Matthieu Baerts (NGI0) Reviewed-by: Eric Dumazet
Re: [PATCH net-next v9 1/7] net: introduce rstreason to detect why the RST is sent
On Thu, Apr 25, 2024 at 5:13 AM Jason Xing wrote: > > From: Jason Xing > > Add a new standalone file for the easy future extension to support > both active reset and passive reset in the TCP/DCCP/MPTCP protocols. > > This patch only does the preparations for reset reason mechanism, > nothing else changes. > > The reset reasons are divided into three parts: > 1) reuse drop reasons for passive reset in TCP > 2) our own independent reasons which aren't relying on other reasons at all > 3) reuse MP_TCPRST option for MPTCP > > The benefits of a standalone reset reason are listed here: > 1) it can cover more than one case, such as reset reasons in MPTCP, > active reset reasons. > 2) people can easily/fastly understand and maintain this mechanism. > 3) we get unified format of output with prefix stripped. > 4) more new reset reasons are on the way > ... > > I will implement the basic codes of active/passive reset reason in > those three protocols, which are not complete for this moment. For > passive reset part in TCP, I only introduce the NO_SOCKET common case > which could be set as an example. > > After this series applied, it will have the ability to open a new > gate to let other people contribute more reasons into it :) > > Signed-off-by: Jason Xing > Acked-by: Matthieu Baerts (NGI0) Reviewed-by: Eric Dumazet
