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

[Menglong Dong <menglong8.dong@gmail.com>]

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);

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

> +
[...]
>