* (no subject)
[not found] <20220515203653.4039075-1-jolsa@kernel.org>
@ 2023-05-20 9:47 ` Ze Gao
2023-05-21 3:58 ` Yonghong Song
2023-05-21 8:08 ` Jiri Olsa
0 siblings, 2 replies; 15+ messages in thread
From: Ze Gao @ 2023-05-20 9:47 UTC (permalink / raw)
To: jolsa
Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, Hao Luo,
John Fastabend, KP Singh, Martin KaFai Lau, Masami Hiramatsu,
Song Liu, Stanislav Fomichev, Steven Rostedt, Yonghong Song, bpf,
linux-kernel, linux-trace-kernel, kafai, kpsingh, netdev, paulmck,
songliubraving, Ze Gao
Hi Jiri,
Would you like to consider to add rcu_is_watching check in
to solve this from the viewpoint of kprobe_multi_link_prog_run
itself? And accounting of missed runs can be added as well
to imporve observability.
Regards,
Ze
-----------------
From 29fd3cd713e65461325c2703cf5246a6fae5d4fe Mon Sep 17 00:00:00 2001
From: Ze Gao <zegao@tencent.com>
Date: Sat, 20 May 2023 17:32:05 +0800
Subject: [PATCH] bpf: kprobe_multi runs bpf progs only when rcu_is_watching
From the perspective of kprobe_multi_link_prog_run, any traceable
functions can be attached while bpf progs need specical care and
ought to be under rcu protection. To solve the likely rcu lockdep
warns once for good, when (future) functions in idle path were
attached accidentally, we better paying some cost to check at least
in kernel-side, and return when rcu is not watching, which helps
to avoid any unpredictable results.
Signed-off-by: Ze Gao <zegao@tencent.com>
---
kernel/trace/bpf_trace.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 9a050e36dc6c..3e6ea7274765 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2622,7 +2622,7 @@ kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link,
struct bpf_run_ctx *old_run_ctx;
int err;
- if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1)) {
+ if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1 || !rcu_is_watching())) {
err = 0;
goto out;
}
--
2.40.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re:
2023-05-20 9:47 ` Ze Gao
@ 2023-05-21 3:58 ` Yonghong Song
2023-05-21 15:10 ` Re: Ze Gao
2023-05-21 8:08 ` Jiri Olsa
1 sibling, 1 reply; 15+ messages in thread
From: Yonghong Song @ 2023-05-21 3:58 UTC (permalink / raw)
To: Ze Gao, jolsa
Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, Hao Luo,
John Fastabend, KP Singh, Martin KaFai Lau, Masami Hiramatsu,
Song Liu, Stanislav Fomichev, Steven Rostedt, Yonghong Song, bpf,
linux-kernel, linux-trace-kernel, kafai, kpsingh, netdev, paulmck,
songliubraving, Ze Gao
On 5/20/23 2:47 AM, Ze Gao wrote:
>
> Hi Jiri,
>
> Would you like to consider to add rcu_is_watching check in
> to solve this from the viewpoint of kprobe_multi_link_prog_run
> itself? And accounting of missed runs can be added as well
> to imporve observability.
>
> Regards,
> Ze
>
>
> -----------------
> From 29fd3cd713e65461325c2703cf5246a6fae5d4fe Mon Sep 17 00:00:00 2001
> From: Ze Gao <zegao@tencent.com>
> Date: Sat, 20 May 2023 17:32:05 +0800
> Subject: [PATCH] bpf: kprobe_multi runs bpf progs only when rcu_is_watching
>
> From the perspective of kprobe_multi_link_prog_run, any traceable
> functions can be attached while bpf progs need specical care and
> ought to be under rcu protection. To solve the likely rcu lockdep
> warns once for good, when (future) functions in idle path were
> attached accidentally, we better paying some cost to check at least
> in kernel-side, and return when rcu is not watching, which helps
> to avoid any unpredictable results.
kprobe_multi/fprobe share the same set of attachments with fentry.
Currently, fentry does not filter with !rcu_is_watching, maybe
because this is an extreme corner case. Not sure whether it is
worthwhile or not.
Maybe if you can give a concrete example (e.g., attachment point)
with current code base to show what the issue you encountered and
it will make it easier to judge whether adding !rcu_is_watching()
is necessary or not.
>
> Signed-off-by: Ze Gao <zegao@tencent.com>
> ---
> kernel/trace/bpf_trace.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 9a050e36dc6c..3e6ea7274765 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2622,7 +2622,7 @@ kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link,
> struct bpf_run_ctx *old_run_ctx;
> int err;
>
> - if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1)) {
> + if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1 || !rcu_is_watching())) {
> err = 0;
> goto out;
> }
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re:
2023-05-20 9:47 ` Ze Gao
2023-05-21 3:58 ` Yonghong Song
@ 2023-05-21 8:08 ` Jiri Olsa
2023-05-21 10:09 ` Re: Masami Hiramatsu
1 sibling, 1 reply; 15+ messages in thread
From: Jiri Olsa @ 2023-05-21 8:08 UTC (permalink / raw)
To: Ze Gao
Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, Hao Luo,
John Fastabend, KP Singh, Martin KaFai Lau, Masami Hiramatsu,
Song Liu, Stanislav Fomichev, Steven Rostedt, Yonghong Song, bpf,
linux-kernel, linux-trace-kernel, kafai, kpsingh, netdev, paulmck,
songliubraving, Ze Gao
On Sat, May 20, 2023 at 05:47:24PM +0800, Ze Gao wrote:
>
> Hi Jiri,
>
> Would you like to consider to add rcu_is_watching check in
> to solve this from the viewpoint of kprobe_multi_link_prog_run
I think this was discussed in here:
https://lore.kernel.org/bpf/20230321020103.13494-1-laoar.shao@gmail.com/
and was considered a bug, there's fix mentioned later in the thread
there's also this recent patchset:
https://lore.kernel.org/bpf/20230517034510.15639-3-zegao@tencent.com/
that solves related problems
> itself? And accounting of missed runs can be added as well
> to imporve observability.
right, we count fprobe->nmissed but it's not exposed, we should allow
to get 'missed' stats from both fprobe and kprobe_multi later, which
is missing now, will check
thanks,
jirka
>
> Regards,
> Ze
>
>
> -----------------
> From 29fd3cd713e65461325c2703cf5246a6fae5d4fe Mon Sep 17 00:00:00 2001
> From: Ze Gao <zegao@tencent.com>
> Date: Sat, 20 May 2023 17:32:05 +0800
> Subject: [PATCH] bpf: kprobe_multi runs bpf progs only when rcu_is_watching
>
> From the perspective of kprobe_multi_link_prog_run, any traceable
> functions can be attached while bpf progs need specical care and
> ought to be under rcu protection. To solve the likely rcu lockdep
> warns once for good, when (future) functions in idle path were
> attached accidentally, we better paying some cost to check at least
> in kernel-side, and return when rcu is not watching, which helps
> to avoid any unpredictable results.
>
> Signed-off-by: Ze Gao <zegao@tencent.com>
> ---
> kernel/trace/bpf_trace.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 9a050e36dc6c..3e6ea7274765 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2622,7 +2622,7 @@ kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link,
> struct bpf_run_ctx *old_run_ctx;
> int err;
>
> - if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1)) {
> + if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1 || !rcu_is_watching())) {
> err = 0;
> goto out;
> }
> --
> 2.40.1
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re:
2023-05-21 8:08 ` Jiri Olsa
@ 2023-05-21 10:09 ` Masami Hiramatsu
2023-05-21 14:19 ` Re: Ze Gao
0 siblings, 1 reply; 15+ messages in thread
From: Masami Hiramatsu @ 2023-05-21 10:09 UTC (permalink / raw)
To: Jiri Olsa
Cc: Ze Gao, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Hao Luo, John Fastabend, KP Singh, Martin KaFai Lau,
Masami Hiramatsu, Song Liu, Stanislav Fomichev, Steven Rostedt,
Yonghong Song, bpf, linux-kernel, linux-trace-kernel, kafai,
kpsingh, netdev, paulmck, songliubraving, Ze Gao
On Sun, 21 May 2023 10:08:46 +0200
Jiri Olsa <olsajiri@gmail.com> wrote:
> On Sat, May 20, 2023 at 05:47:24PM +0800, Ze Gao wrote:
> >
> > Hi Jiri,
> >
> > Would you like to consider to add rcu_is_watching check in
> > to solve this from the viewpoint of kprobe_multi_link_prog_run
>
> I think this was discussed in here:
> https://lore.kernel.org/bpf/20230321020103.13494-1-laoar.shao@gmail.com/
>
> and was considered a bug, there's fix mentioned later in the thread
>
> there's also this recent patchset:
> https://lore.kernel.org/bpf/20230517034510.15639-3-zegao@tencent.com/
>
> that solves related problems
I think this rcu_is_watching() is a bit different issue. This rcu_is_watching()
check is required if the kprobe_multi_link_prog_run() uses any RCU API.
E.g. rethook_try_get() is also checks rcu_is_watching() because it uses
call_rcu().
Thank you,
>
> > itself? And accounting of missed runs can be added as well
> > to imporve observability.
>
> right, we count fprobe->nmissed but it's not exposed, we should allow
> to get 'missed' stats from both fprobe and kprobe_multi later, which
> is missing now, will check
>
> thanks,
> jirka
>
> >
> > Regards,
> > Ze
> >
> >
> > -----------------
> > From 29fd3cd713e65461325c2703cf5246a6fae5d4fe Mon Sep 17 00:00:00 2001
> > From: Ze Gao <zegao@tencent.com>
> > Date: Sat, 20 May 2023 17:32:05 +0800
> > Subject: [PATCH] bpf: kprobe_multi runs bpf progs only when rcu_is_watching
> >
> > From the perspective of kprobe_multi_link_prog_run, any traceable
> > functions can be attached while bpf progs need specical care and
> > ought to be under rcu protection. To solve the likely rcu lockdep
> > warns once for good, when (future) functions in idle path were
> > attached accidentally, we better paying some cost to check at least
> > in kernel-side, and return when rcu is not watching, which helps
> > to avoid any unpredictable results.
> >
> > Signed-off-by: Ze Gao <zegao@tencent.com>
> > ---
> > kernel/trace/bpf_trace.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index 9a050e36dc6c..3e6ea7274765 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -2622,7 +2622,7 @@ kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link,
> > struct bpf_run_ctx *old_run_ctx;
> > int err;
> >
> > - if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1)) {
> > + if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1 || !rcu_is_watching())) {
> > err = 0;
> > goto out;
> > }
> > --
> > 2.40.1
> >
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re:
2023-05-21 10:09 ` Re: Masami Hiramatsu
@ 2023-05-21 14:19 ` Ze Gao
0 siblings, 0 replies; 15+ messages in thread
From: Ze Gao @ 2023-05-21 14:19 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Jiri Olsa, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Hao Luo, John Fastabend, KP Singh, Martin KaFai Lau, Song Liu,
Stanislav Fomichev, Steven Rostedt, Yonghong Song, bpf,
linux-kernel, linux-trace-kernel, kafai, kpsingh, netdev, paulmck,
songliubraving, Ze Gao
On Sun, May 21, 2023 at 6:09 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> On Sun, 21 May 2023 10:08:46 +0200
> Jiri Olsa <olsajiri@gmail.com> wrote:
>
> > On Sat, May 20, 2023 at 05:47:24PM +0800, Ze Gao wrote:
> > >
> > > Hi Jiri,
> > >
> > > Would you like to consider to add rcu_is_watching check in
> > > to solve this from the viewpoint of kprobe_multi_link_prog_run
> >
> > I think this was discussed in here:
> > https://lore.kernel.org/bpf/20230321020103.13494-1-laoar.shao@gmail.com/
> >
> > and was considered a bug, there's fix mentioned later in the thread
> >
> > there's also this recent patchset:
> > https://lore.kernel.org/bpf/20230517034510.15639-3-zegao@tencent.com/
> >
> > that solves related problems
>
> I think this rcu_is_watching() is a bit different issue. This rcu_is_watching()
> check is required if the kprobe_multi_link_prog_run() uses any RCU API.
> E.g. rethook_try_get() is also checks rcu_is_watching() because it uses
> call_rcu().
Yes, that's my point!
Regards,
Ze
>
> >
> > > itself? And accounting of missed runs can be added as well
> > > to imporve observability.
> >
> > right, we count fprobe->nmissed but it's not exposed, we should allow
> > to get 'missed' stats from both fprobe and kprobe_multi later, which
> > is missing now, will check
> >
> > thanks,
> > jirka
> >
> > >
> > > Regards,
> > > Ze
> > >
> > >
> > > -----------------
> > > From 29fd3cd713e65461325c2703cf5246a6fae5d4fe Mon Sep 17 00:00:00 2001
> > > From: Ze Gao <zegao@tencent.com>
> > > Date: Sat, 20 May 2023 17:32:05 +0800
> > > Subject: [PATCH] bpf: kprobe_multi runs bpf progs only when rcu_is_watching
> > >
> > > From the perspective of kprobe_multi_link_prog_run, any traceable
> > > functions can be attached while bpf progs need specical care and
> > > ought to be under rcu protection. To solve the likely rcu lockdep
> > > warns once for good, when (future) functions in idle path were
> > > attached accidentally, we better paying some cost to check at least
> > > in kernel-side, and return when rcu is not watching, which helps
> > > to avoid any unpredictable results.
> > >
> > > Signed-off-by: Ze Gao <zegao@tencent.com>
> > > ---
> > > kernel/trace/bpf_trace.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > > index 9a050e36dc6c..3e6ea7274765 100644
> > > --- a/kernel/trace/bpf_trace.c
> > > +++ b/kernel/trace/bpf_trace.c
> > > @@ -2622,7 +2622,7 @@ kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link,
> > > struct bpf_run_ctx *old_run_ctx;
> > > int err;
> > >
> > > - if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1)) {
> > > + if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1 || !rcu_is_watching())) {
> > > err = 0;
> > > goto out;
> > > }
> > > --
> > > 2.40.1
> > >
>
>
> --
> Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re:
2023-05-21 3:58 ` Yonghong Song
@ 2023-05-21 15:10 ` Ze Gao
2023-05-21 20:26 ` Re: Jiri Olsa
0 siblings, 1 reply; 15+ messages in thread
From: Ze Gao @ 2023-05-21 15:10 UTC (permalink / raw)
To: Yonghong Song
Cc: jolsa, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Hao Luo, John Fastabend, KP Singh, Martin KaFai Lau,
Masami Hiramatsu, Song Liu, Stanislav Fomichev, Steven Rostedt,
Yonghong Song, bpf, linux-kernel, linux-trace-kernel, kafai,
kpsingh, netdev, paulmck, songliubraving, Ze Gao
> kprobe_multi/fprobe share the same set of attachments with fentry.
> Currently, fentry does not filter with !rcu_is_watching, maybe
> because this is an extreme corner case. Not sure whether it is
> worthwhile or not.
Agreed, it's rare, especially after Peter's patches which push narrow
down rcu eqs regions
in the idle path and reduce the chance of any traceable functions
happening in between.
However, from RCU's perspective, we ought to check if rcu_is_watching
theoretically
when there's a chance our code will run in the idle path and also we
need rcu to be alive,
And also we cannot simply make assumptions for any future changes in
the idle path.
You know, just like what was hit in the thread.
> Maybe if you can give a concrete example (e.g., attachment point)
> with current code base to show what the issue you encountered and
> it will make it easier to judge whether adding !rcu_is_watching()
> is necessary or not.
I can reproduce likely warnings on v6.1.18 where arch_cpu_idle is
traceable but not on the latest version
so far. But as I state above, in theory we need it. So here is a
gentle ping :) .
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re:
2023-05-21 15:10 ` Re: Ze Gao
@ 2023-05-21 20:26 ` Jiri Olsa
2023-05-22 1:36 ` Re: Masami Hiramatsu
2023-05-22 2:07 ` Re: Ze Gao
0 siblings, 2 replies; 15+ messages in thread
From: Jiri Olsa @ 2023-05-21 20:26 UTC (permalink / raw)
To: Ze Gao
Cc: Yonghong Song, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Hao Luo, John Fastabend, KP Singh,
Martin KaFai Lau, Masami Hiramatsu, Song Liu, Stanislav Fomichev,
Steven Rostedt, Yonghong Song, bpf, linux-kernel,
linux-trace-kernel, kafai, kpsingh, netdev, paulmck,
songliubraving, Ze Gao
On Sun, May 21, 2023 at 11:10:16PM +0800, Ze Gao wrote:
> > kprobe_multi/fprobe share the same set of attachments with fentry.
> > Currently, fentry does not filter with !rcu_is_watching, maybe
> > because this is an extreme corner case. Not sure whether it is
> > worthwhile or not.
>
> Agreed, it's rare, especially after Peter's patches which push narrow
> down rcu eqs regions
> in the idle path and reduce the chance of any traceable functions
> happening in between.
>
> However, from RCU's perspective, we ought to check if rcu_is_watching
> theoretically
> when there's a chance our code will run in the idle path and also we
> need rcu to be alive,
> And also we cannot simply make assumptions for any future changes in
> the idle path.
> You know, just like what was hit in the thread.
>
> > Maybe if you can give a concrete example (e.g., attachment point)
> > with current code base to show what the issue you encountered and
> > it will make it easier to judge whether adding !rcu_is_watching()
> > is necessary or not.
>
> I can reproduce likely warnings on v6.1.18 where arch_cpu_idle is
> traceable but not on the latest version
> so far. But as I state above, in theory we need it. So here is a
> gentle ping :) .
hum, this change [1] added rcu_is_watching check to ftrace_test_recursion_trylock,
which we use in fprobe_handler and is coming to fprobe_exit_handler in [2]
I might be missing something, but it seems like we don't need another
rcu_is_watching call on kprobe_multi level
jirka
[1] d099dbfd3306 cpuidle: tracing: Warn about !rcu_is_watching()
[2] https://lore.kernel.org/bpf/20230517034510.15639-4-zegao@tencent.com/
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re:
2023-05-21 20:26 ` Re: Jiri Olsa
@ 2023-05-22 1:36 ` Masami Hiramatsu
2023-05-22 2:07 ` Re: Ze Gao
1 sibling, 0 replies; 15+ messages in thread
From: Masami Hiramatsu @ 2023-05-22 1:36 UTC (permalink / raw)
To: Jiri Olsa
Cc: Ze Gao, Yonghong Song, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Hao Luo, John Fastabend, KP Singh,
Martin KaFai Lau, Masami Hiramatsu, Song Liu, Stanislav Fomichev,
Steven Rostedt, Yonghong Song, bpf, linux-kernel,
linux-trace-kernel, kafai, kpsingh, netdev, paulmck,
songliubraving, Ze Gao
On Sun, 21 May 2023 22:26:37 +0200
Jiri Olsa <olsajiri@gmail.com> wrote:
> On Sun, May 21, 2023 at 11:10:16PM +0800, Ze Gao wrote:
> > > kprobe_multi/fprobe share the same set of attachments with fentry.
> > > Currently, fentry does not filter with !rcu_is_watching, maybe
> > > because this is an extreme corner case. Not sure whether it is
> > > worthwhile or not.
> >
> > Agreed, it's rare, especially after Peter's patches which push narrow
> > down rcu eqs regions
> > in the idle path and reduce the chance of any traceable functions
> > happening in between.
> >
> > However, from RCU's perspective, we ought to check if rcu_is_watching
> > theoretically
> > when there's a chance our code will run in the idle path and also we
> > need rcu to be alive,
> > And also we cannot simply make assumptions for any future changes in
> > the idle path.
> > You know, just like what was hit in the thread.
> >
> > > Maybe if you can give a concrete example (e.g., attachment point)
> > > with current code base to show what the issue you encountered and
> > > it will make it easier to judge whether adding !rcu_is_watching()
> > > is necessary or not.
> >
> > I can reproduce likely warnings on v6.1.18 where arch_cpu_idle is
> > traceable but not on the latest version
> > so far. But as I state above, in theory we need it. So here is a
> > gentle ping :) .
>
> hum, this change [1] added rcu_is_watching check to ftrace_test_recursion_trylock,
> which we use in fprobe_handler and is coming to fprobe_exit_handler in [2]
>
> I might be missing something, but it seems like we don't need another
> rcu_is_watching call on kprobe_multi level
Good point! OK, then it seems we don't need it. The rethook continues to
use the rcu_is_watching() because it is also used from kprobes, but the
kprobe_multi doesn't need it.
Thank you,
>
> jirka
>
>
> [1] d099dbfd3306 cpuidle: tracing: Warn about !rcu_is_watching()
> [2] https://lore.kernel.org/bpf/20230517034510.15639-4-zegao@tencent.com/
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re:
2023-05-21 20:26 ` Re: Jiri Olsa
2023-05-22 1:36 ` Re: Masami Hiramatsu
@ 2023-05-22 2:07 ` Ze Gao
2023-05-23 4:38 ` Re: Yonghong Song
` (2 more replies)
1 sibling, 3 replies; 15+ messages in thread
From: Ze Gao @ 2023-05-22 2:07 UTC (permalink / raw)
To: Jiri Olsa
Cc: Yonghong Song, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Hao Luo, John Fastabend, KP Singh,
Martin KaFai Lau, Masami Hiramatsu, Song Liu, Stanislav Fomichev,
Steven Rostedt, Yonghong Song, bpf, linux-kernel,
linux-trace-kernel, kafai, kpsingh, netdev, paulmck,
songliubraving, Ze Gao
Oops, I missed that. Thanks for pointing that out, which I thought is
conditional use of rcu_is_watching before.
One last point, I think we should double check on this
"fentry does not filter with !rcu_is_watching"
as quoted from Yonghong and argue whether it needs
the same check for fentry as well.
Regards,
Ze
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re:
2023-05-22 2:07 ` Re: Ze Gao
@ 2023-05-23 4:38 ` Yonghong Song
2023-05-23 5:30 ` Re: Masami Hiramatsu
2023-05-23 14:10 ` kprobes and rcu_is_watching() Steven Rostedt
2 siblings, 0 replies; 15+ messages in thread
From: Yonghong Song @ 2023-05-23 4:38 UTC (permalink / raw)
To: Ze Gao, Jiri Olsa
Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, Hao Luo,
John Fastabend, KP Singh, Martin KaFai Lau, Masami Hiramatsu,
Song Liu, Stanislav Fomichev, Steven Rostedt, Yonghong Song, bpf,
linux-kernel, linux-trace-kernel, kafai, kpsingh, netdev, paulmck,
songliubraving, Ze Gao
On 5/21/23 7:07 PM, Ze Gao wrote:
> Oops, I missed that. Thanks for pointing that out, which I thought is
> conditional use of rcu_is_watching before.
>
> One last point, I think we should double check on this
> "fentry does not filter with !rcu_is_watching"
> as quoted from Yonghong and argue whether it needs
> the same check for fentry as well.
I would suggest that we address rcu_is_watching issue for fentry
only if we do have a reproducible case to show something goes wrong...
>
> Regards,
> Ze
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re:
2023-05-22 2:07 ` Re: Ze Gao
2023-05-23 4:38 ` Re: Yonghong Song
@ 2023-05-23 5:30 ` Masami Hiramatsu
2023-05-23 6:59 ` Re: Paul E. McKenney
2023-05-23 14:10 ` kprobes and rcu_is_watching() Steven Rostedt
2 siblings, 1 reply; 15+ messages in thread
From: Masami Hiramatsu @ 2023-05-23 5:30 UTC (permalink / raw)
To: Ze Gao
Cc: Jiri Olsa, Yonghong Song, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Hao Luo, John Fastabend, KP Singh,
Martin KaFai Lau, Masami Hiramatsu, Song Liu, Stanislav Fomichev,
Steven Rostedt, Yonghong Song, bpf, linux-kernel,
linux-trace-kernel, kafai, kpsingh, netdev, paulmck,
songliubraving, Ze Gao
On Mon, 22 May 2023 10:07:42 +0800
Ze Gao <zegao2021@gmail.com> wrote:
> Oops, I missed that. Thanks for pointing that out, which I thought is
> conditional use of rcu_is_watching before.
>
> One last point, I think we should double check on this
> "fentry does not filter with !rcu_is_watching"
> as quoted from Yonghong and argue whether it needs
> the same check for fentry as well.
rcu_is_watching() comment says;
* if the current CPU is not in its idle loop or is in an interrupt or
* NMI handler, return true.
Thus it returns *fault* if the current CPU is in the idle loop and not
any interrupt(including NMI) context. This means if any tracable function
is called from idle loop, it can be !rcu_is_watching(). I meant, this is
'context' based check, thus fentry can not filter out that some commonly
used functions is called from that context but it can be detected.
Thank you,
>
> Regards,
> Ze
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re:
2023-05-23 5:30 ` Re: Masami Hiramatsu
@ 2023-05-23 6:59 ` Paul E. McKenney
2023-05-25 0:13 ` Re: Masami Hiramatsu
0 siblings, 1 reply; 15+ messages in thread
From: Paul E. McKenney @ 2023-05-23 6:59 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Ze Gao, Jiri Olsa, Yonghong Song, Alexei Starovoitov,
Andrii Nakryiko, Daniel Borkmann, Hao Luo, John Fastabend,
KP Singh, Martin KaFai Lau, Song Liu, Stanislav Fomichev,
Steven Rostedt, Yonghong Song, bpf, linux-kernel,
linux-trace-kernel, kafai, kpsingh, netdev, songliubraving,
Ze Gao
On Tue, May 23, 2023 at 01:30:19PM +0800, Masami Hiramatsu wrote:
> On Mon, 22 May 2023 10:07:42 +0800
> Ze Gao <zegao2021@gmail.com> wrote:
>
> > Oops, I missed that. Thanks for pointing that out, which I thought is
> > conditional use of rcu_is_watching before.
> >
> > One last point, I think we should double check on this
> > "fentry does not filter with !rcu_is_watching"
> > as quoted from Yonghong and argue whether it needs
> > the same check for fentry as well.
>
> rcu_is_watching() comment says;
>
> * if the current CPU is not in its idle loop or is in an interrupt or
> * NMI handler, return true.
>
> Thus it returns *fault* if the current CPU is in the idle loop and not
> any interrupt(including NMI) context. This means if any tracable function
> is called from idle loop, it can be !rcu_is_watching(). I meant, this is
> 'context' based check, thus fentry can not filter out that some commonly
> used functions is called from that context but it can be detected.
It really does return false (rather than faulting?) if the current CPU
is deep within the idle loop.
In addition, the recent x86/entry rework (thank you Peter and
Thomas!) mean that the "idle loop" is quite restricted, as can be
seen by the invocations of ct_cpuidle_enter() and ct_cpuidle_exit().
For example, in default_idle_call(), these are immediately before and
after the call to arch_cpu_idle().
Would the following help? Or am I missing your point?
Thanx, Paul
------------------------------------------------------------------------
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 1449cb69a0e0..fae9b4e29c93 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -679,10 +679,14 @@ static void rcu_disable_urgency_upon_qs(struct rcu_data *rdp)
/**
* rcu_is_watching - see if RCU thinks that the current CPU is not idle
*
- * Return true if RCU is watching the running CPU, which means that this
- * CPU can safely enter RCU read-side critical sections. In other words,
- * if the current CPU is not in its idle loop or is in an interrupt or
- * NMI handler, return true.
+ * Return @true if RCU is watching the running CPU and @false otherwise.
+ * An @true return means that this CPU can safely enter RCU read-side
+ * critical sections.
+ *
+ * More specifically, if the current CPU is not deep within its idle
+ * loop, return @true. Note that rcu_is_watching() will return @true if
+ * invoked from an interrupt or NMI handler, even if that interrupt or
+ * NMI interrupted the CPU while it was deep within its idle loop.
*
* Make notrace because it can be called by the internal functions of
* ftrace, and making this notrace removes unnecessary recursion calls.
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: kprobes and rcu_is_watching()
2023-05-22 2:07 ` Re: Ze Gao
2023-05-23 4:38 ` Re: Yonghong Song
2023-05-23 5:30 ` Re: Masami Hiramatsu
@ 2023-05-23 14:10 ` Steven Rostedt
2023-05-24 3:51 ` Ze Gao
2 siblings, 1 reply; 15+ messages in thread
From: Steven Rostedt @ 2023-05-23 14:10 UTC (permalink / raw)
To: Ze Gao
Cc: Jiri Olsa, Yonghong Song, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Hao Luo, John Fastabend, KP Singh,
Martin KaFai Lau, Masami Hiramatsu, Song Liu, Stanislav Fomichev,
Yonghong Song, bpf, linux-kernel, linux-trace-kernel, kafai,
kpsingh, netdev, paulmck, songliubraving, Ze Gao
[ Added a subject, as I always want to delete these emails as spam! ]
On Mon, 22 May 2023 10:07:42 +0800
Ze Gao <zegao2021@gmail.com> wrote:
> Oops, I missed that. Thanks for pointing that out, which I thought is
> conditional use of rcu_is_watching before.
>
> One last point, I think we should double check on this
> "fentry does not filter with !rcu_is_watching"
> as quoted from Yonghong and argue whether it needs
> the same check for fentry as well.
>
Note that trace_test_and_set_recursion() (which is used by
ftrace_test_recursion_trylock()) checks for rcu_is_watching() and
returns false if it isn't (and the trylock will fail).
-- Steve
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: kprobes and rcu_is_watching()
2023-05-23 14:10 ` kprobes and rcu_is_watching() Steven Rostedt
@ 2023-05-24 3:51 ` Ze Gao
0 siblings, 0 replies; 15+ messages in thread
From: Ze Gao @ 2023-05-24 3:51 UTC (permalink / raw)
To: Steven Rostedt
Cc: Jiri Olsa, Yonghong Song, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Hao Luo, John Fastabend, KP Singh,
Martin KaFai Lau, Masami Hiramatsu, Song Liu, Stanislav Fomichev,
Yonghong Song, bpf, linux-kernel, linux-trace-kernel, kafai,
kpsingh, netdev, paulmck, songliubraving, Ze Gao
Thanks Steven, I think we've come to a consensus on this.
The question here is whether bpf tracing fentry i.e.,
__bpf_prog_enter{_sleepable}
needs to check rcu_is_watching as well before using rcu related
calls. And Yonghong suggested making a change when there is
indeed some bad case occurring since it's rare the tracee is in the idle path.
Regards,
Ze
On Tue, May 23, 2023 at 10:10 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> [ Added a subject, as I always want to delete these emails as spam! ]
>
> On Mon, 22 May 2023 10:07:42 +0800
> Ze Gao <zegao2021@gmail.com> wrote:
>
> > Oops, I missed that. Thanks for pointing that out, which I thought is
> > conditional use of rcu_is_watching before.
> >
> > One last point, I think we should double check on this
> > "fentry does not filter with !rcu_is_watching"
> > as quoted from Yonghong and argue whether it needs
> > the same check for fentry as well.
> >
>
> Note that trace_test_and_set_recursion() (which is used by
> ftrace_test_recursion_trylock()) checks for rcu_is_watching() and
> returns false if it isn't (and the trylock will fail).
>
> -- Steve
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re:
2023-05-23 6:59 ` Re: Paul E. McKenney
@ 2023-05-25 0:13 ` Masami Hiramatsu
0 siblings, 0 replies; 15+ messages in thread
From: Masami Hiramatsu @ 2023-05-25 0:13 UTC (permalink / raw)
To: paulmck
Cc: Ze Gao, Jiri Olsa, Yonghong Song, Alexei Starovoitov,
Andrii Nakryiko, Daniel Borkmann, Hao Luo, John Fastabend,
KP Singh, Martin KaFai Lau, Song Liu, Stanislav Fomichev,
Steven Rostedt, Yonghong Song, bpf, linux-kernel,
linux-trace-kernel, kafai, kpsingh, netdev, songliubraving,
Ze Gao
On Mon, 22 May 2023 23:59:28 -0700
"Paul E. McKenney" <paulmck@kernel.org> wrote:
> On Tue, May 23, 2023 at 01:30:19PM +0800, Masami Hiramatsu wrote:
> > On Mon, 22 May 2023 10:07:42 +0800
> > Ze Gao <zegao2021@gmail.com> wrote:
> >
> > > Oops, I missed that. Thanks for pointing that out, which I thought is
> > > conditional use of rcu_is_watching before.
> > >
> > > One last point, I think we should double check on this
> > > "fentry does not filter with !rcu_is_watching"
> > > as quoted from Yonghong and argue whether it needs
> > > the same check for fentry as well.
> >
> > rcu_is_watching() comment says;
> >
> > * if the current CPU is not in its idle loop or is in an interrupt or
> > * NMI handler, return true.
> >
> > Thus it returns *fault* if the current CPU is in the idle loop and not
> > any interrupt(including NMI) context. This means if any tracable function
> > is called from idle loop, it can be !rcu_is_watching(). I meant, this is
> > 'context' based check, thus fentry can not filter out that some commonly
> > used functions is called from that context but it can be detected.
>
> It really does return false (rather than faulting?) if the current CPU
> is deep within the idle loop.
>
> In addition, the recent x86/entry rework (thank you Peter and
> Thomas!) mean that the "idle loop" is quite restricted, as can be
> seen by the invocations of ct_cpuidle_enter() and ct_cpuidle_exit().
> For example, in default_idle_call(), these are immediately before and
> after the call to arch_cpu_idle().
Thanks! I also found that the default_idle_call() is enough small and
it seems not happening on fentry because there are no commonly used
functions on that path.
>
> Would the following help? Or am I missing your point?
Yes, thank you for the update!
>
> Thanx, Paul
>
> ------------------------------------------------------------------------
>
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 1449cb69a0e0..fae9b4e29c93 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -679,10 +679,14 @@ static void rcu_disable_urgency_upon_qs(struct rcu_data *rdp)
> /**
> * rcu_is_watching - see if RCU thinks that the current CPU is not idle
> *
> - * Return true if RCU is watching the running CPU, which means that this
> - * CPU can safely enter RCU read-side critical sections. In other words,
> - * if the current CPU is not in its idle loop or is in an interrupt or
> - * NMI handler, return true.
> + * Return @true if RCU is watching the running CPU and @false otherwise.
> + * An @true return means that this CPU can safely enter RCU read-side
> + * critical sections.
> + *
> + * More specifically, if the current CPU is not deep within its idle
> + * loop, return @true. Note that rcu_is_watching() will return @true if
> + * invoked from an interrupt or NMI handler, even if that interrupt or
> + * NMI interrupted the CPU while it was deep within its idle loop.
> *
> * Make notrace because it can be called by the internal functions of
> * ftrace, and making this notrace removes unnecessary recursion calls.
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2023-05-25 0:13 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20220515203653.4039075-1-jolsa@kernel.org>
2023-05-20 9:47 ` Ze Gao
2023-05-21 3:58 ` Yonghong Song
2023-05-21 15:10 ` Re: Ze Gao
2023-05-21 20:26 ` Re: Jiri Olsa
2023-05-22 1:36 ` Re: Masami Hiramatsu
2023-05-22 2:07 ` Re: Ze Gao
2023-05-23 4:38 ` Re: Yonghong Song
2023-05-23 5:30 ` Re: Masami Hiramatsu
2023-05-23 6:59 ` Re: Paul E. McKenney
2023-05-25 0:13 ` Re: Masami Hiramatsu
2023-05-23 14:10 ` kprobes and rcu_is_watching() Steven Rostedt
2023-05-24 3:51 ` Ze Gao
2023-05-21 8:08 ` Jiri Olsa
2023-05-21 10:09 ` Re: Masami Hiramatsu
2023-05-21 14:19 ` Re: Ze Gao
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).