Re: [PATCHv4 bpf-next 1/9] ftrace,bpf: Remove FTRACE_OPS_FL_JMP ftrace_ops flag

[Jiri Olsa <olsajiri@gmail.com>]

On Wed, Dec 03, 2025 at 05:15:52PM +0800, Menglong Dong wrote:
> On Wed, Dec 3, 2025 at 4:24 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > At the moment the we allow the jmp attach only for ftrace_ops that
> > has FTRACE_OPS_FL_JMP set. This conflicts with following changes
> > where we use single ftrace_ops object for all direct call sites,
> > so all could be be attached via just call or jmp.
> >
> > We already limit the jmp attach support with config option and bit
> > (LSB) set on the trampoline address. It turns out that's actually
> > enough to limit the jmp attach for architecture and only for chosen
> > addresses (with LSB bit set).
> >
> > Each user of register_ftrace_direct or modify_ftrace_direct can set
> > the trampoline bit (LSB) to indicate it has to be attached by jmp.
> >
> > The bpf trampoline generation code uses trampoline flags to generate
> > jmp-attach specific code and ftrace inner code uses the trampoline
> > bit (LSB) to handle return from jmp attachment, so there's no harm
> > to remove the FTRACE_OPS_FL_JMP bit.
> >
> > The fexit/fmodret performance stays the same (did not drop),
> > current code:
> >
> >   fentry         :   77.904 ± 0.546M/s
> >   fexit          :   62.430 ± 0.554M/s
> >   fmodret        :   66.503 ± 0.902M/s
> >
> > with this change:
> >
> >   fentry         :   80.472 ± 0.061M/s
> >   fexit          :   63.995 ± 0.127M/s
> >   fmodret        :   67.362 ± 0.175M/s
> >
> > Fixes: 25e4e3565d45 ("ftrace: Introduce FTRACE_OPS_FL_JMP")
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  include/linux/ftrace.h  |  1 -
> >  kernel/bpf/trampoline.c | 32 ++++++++++++++------------------
> >  kernel/trace/ftrace.c   | 14 --------------
> >  3 files changed, 14 insertions(+), 33 deletions(-)
> >
> > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> > index 015dd1049bea..505b7d3f5641 100644
> > --- a/include/linux/ftrace.h
> > +++ b/include/linux/ftrace.h
> > @@ -359,7 +359,6 @@ enum {
> >         FTRACE_OPS_FL_DIRECT                    = BIT(17),
> >         FTRACE_OPS_FL_SUBOP                     = BIT(18),
> >         FTRACE_OPS_FL_GRAPH                     = BIT(19),
> > -       FTRACE_OPS_FL_JMP                       = BIT(20),
> 
> Yeah, the FTRACE_OPS_FL_JMP is not necessary. I added
> it in case that we maybe want to implement such "jmp" for
> ftrace trampoline in the feature. But it's OK to remove it now.
> 
> >  };
> >
> >  #ifndef CONFIG_DYNAMIC_FTRACE_WITH_ARGS
> > diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> > index 976d89011b15..b9a358d7a78f 100644
> > --- a/kernel/bpf/trampoline.c
> > +++ b/kernel/bpf/trampoline.c
> > @@ -214,10 +214,15 @@ static int modify_fentry(struct bpf_trampoline *tr, u32 orig_flags,
> >         int ret;
> >
> >         if (tr->func.ftrace_managed) {
> > +               unsigned long addr = (unsigned long) new_addr;
> > +
> > +               if (bpf_trampoline_use_jmp(tr->flags))
> > +                       addr = ftrace_jmp_set(addr);

I wanted to get rid of the  void * -> unsigned long casting in all the
places.. this way it has to be just on one place above, but maybe we
could have already direct_ops_add with unsigned long addr, will check

jirka

> 
> nit: It seems that we can remove the variable "addr" can use
> the "new_addr" directly?
> 
> > +
> >                 if (lock_direct_mutex)
> > -                       ret = modify_ftrace_direct(tr->fops, (long)new_addr);
> > +                       ret = modify_ftrace_direct(tr->fops, addr);
> >                 else
> > -                       ret = modify_ftrace_direct_nolock(tr->fops, (long)new_addr);
> > +                       ret = modify_ftrace_direct_nolock(tr->fops, addr);
> >         } else {
> >                 ret = bpf_trampoline_update_fentry(tr, orig_flags, old_addr,
> >                                                    new_addr);
> > @@ -240,10 +245,15 @@ static int register_fentry(struct bpf_trampoline *tr, void *new_addr)
> >         }
> >
> >         if (tr->func.ftrace_managed) {
> > +               unsigned long addr = (unsigned long) new_addr;
> > +
> > +               if (bpf_trampoline_use_jmp(tr->flags))
> > +                       addr = ftrace_jmp_set(addr);
> 
> And here.
> 
> Thanks!
> Menglong Dong
> 
> > +
> [...]
> >