Re: [PATCH v4 2/2] rethook: honor CONFIG_FTRACE_VALIDATE_RCU_IS_WATCHING in rethook_try_get()

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Fri, 19 Apr 2024 10:59:09 -0700
Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:

> On Thu, Apr 18, 2024 at 6:00 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >
> > On Thu, 18 Apr 2024 12:09:09 -0700
> > Andrii Nakryiko <andrii@kernel.org> wrote:
> >
> > > Take into account CONFIG_FTRACE_VALIDATE_RCU_IS_WATCHING when validating
> > > that RCU is watching when trying to setup rethooko on a function entry.
> > >
> > > One notable exception when we force rcu_is_watching() check is
> > > CONFIG_KPROBE_EVENTS_ON_NOTRACE=y case, in which case kretprobes will use
> > > old-style int3-based workflow instead of relying on ftrace, making RCU
> > > watching check important to validate.
> > >
> > > This further (in addition to improvements in the previous patch)
> > > improves BPF multi-kretprobe (which rely on rethook) runtime throughput
> > > by 2.3%, according to BPF benchmarks ([0]).
> > >
> > >   [0] https://lore.kernel.org/bpf/CAEf4BzauQ2WKMjZdc9s0rBWa01BYbgwHN6aNDXQSHYia47pQ-w@mail.gmail.com/
> > >
> > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> >
> >
> > Thanks for update! This looks good to me.
> 
> Thanks, Masami! Will you take it through your tree, or you'd like to
> route it through bpf-next?

OK, let me take it through linux-trace tree.

Thank you!

> 
> >
> > Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> >
> > Thanks,
> >
> > > ---
> > >  kernel/trace/rethook.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/kernel/trace/rethook.c b/kernel/trace/rethook.c
> > > index fa03094e9e69..a974605ad7a5 100644
> > > --- a/kernel/trace/rethook.c
> > > +++ b/kernel/trace/rethook.c
> > > @@ -166,6 +166,7 @@ struct rethook_node *rethook_try_get(struct rethook *rh)
> > >       if (unlikely(!handler))
> > >               return NULL;
> > >
> > > +#if defined(CONFIG_FTRACE_VALIDATE_RCU_IS_WATCHING) || defined(CONFIG_KPROBE_EVENTS_ON_NOTRACE)
> > >       /*
> > >        * This expects the caller will set up a rethook on a function entry.
> > >        * When the function returns, the rethook will eventually be reclaimed
> > > @@ -174,6 +175,7 @@ struct rethook_node *rethook_try_get(struct rethook *rh)
> > >        */
> > >       if (unlikely(!rcu_is_watching()))
> > >               return NULL;
> > > +#endif
> > >
> > >       return (struct rethook_node *)objpool_pop(&rh->pool);
> > >  }
> > > --
> > > 2.43.0
> > >
> >
> >
> > --
> > Masami Hiramatsu (Google) <mhiramat@kernel.org>


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>