From: Jiri Olsa <olsajiri@gmail.com>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: Steven Rostedt <rostedt@kernel.org>,
Florent Revest <revest@google.com>,
Mark Rutland <mark.rutland@arm.com>,
bpf@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-trace-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Andrii Nakryiko <andrii@kernel.org>,
Menglong Dong <menglong8.dong@gmail.com>,
Song Liu <song@kernel.org>
Subject: Re: [PATCHv5 bpf-next 9/9] bpf,x86: Use single ftrace_ops for direct calls
Date: Fri, 19 Dec 2025 10:27:56 +0100 [thread overview]
Message-ID: <aUUanPijlWsDlS0X@krava> (raw)
In-Reply-To: <20251218112608.11a14a4a@gandalf.local.home>
On Thu, Dec 18, 2025 at 11:26:08AM -0500, Steven Rostedt wrote:
> On Mon, 15 Dec 2025 22:14:02 +0100
> Jiri Olsa <jolsa@kernel.org> wrote:
>
> > Using single ftrace_ops for direct calls update instead of allocating
> > ftrace_ops object for each trampoline.
> >
> > With single ftrace_ops object we can use update_ftrace_direct_* api
> > that allows multiple ip sites updates on single ftrace_ops object.
> >
> > Adding HAVE_SINGLE_FTRACE_DIRECT_OPS config option to be enabled on
> > each arch that supports this.
> >
> > At the moment we can enable this only on x86 arch, because arm relies
> > on ftrace_ops object representing just single trampoline image (stored
> > in ftrace_ops::direct_call). Ach that do not support this will continue
>
> My back "Ach" and doesn't support me well. ;-)
heh, should have been 'Archs' ;-)
>
> > to use *_ftrace_direct api.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> > arch/x86/Kconfig | 1 +
> > kernel/bpf/trampoline.c | 195 ++++++++++++++++++++++++++++++++++------
> > kernel/trace/Kconfig | 3 +
> > kernel/trace/ftrace.c | 7 +-
> > 4 files changed, 177 insertions(+), 29 deletions(-)
> >
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index 17a107cc5244..d0c36e49e66e 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -335,6 +335,7 @@ config X86
> > select SCHED_SMT if SMP
> > select ARCH_SUPPORTS_SCHED_CLUSTER if SMP
> > select ARCH_SUPPORTS_SCHED_MC if SMP
> > + select HAVE_SINGLE_FTRACE_DIRECT_OPS if X86_64 && DYNAMIC_FTRACE_WITH_DIRECT_CALLS
>
> You can remove the "&& DYNAMIC_FTRACE_WITH_DIRECT_CALLS" part by having the
> config depend on it (see below).
...
>
> >
> > config INSTRUCTION_DECODER
> > def_bool y
> > diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> > index 17af2aad8382..02371db3db3e 100644
> > --- a/kernel/bpf/trampoline.c
> > +++ b/kernel/bpf/trampoline.c
> > @@ -33,12 +33,40 @@ static DEFINE_MUTEX(trampoline_mutex);
> > #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> > static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mutex);
> >
> > +#ifdef CONFIG_HAVE_SINGLE_FTRACE_DIRECT_OPS
>
> Make this:
>
> #ifdef CONFIG_SINGLE_FTRACE_DIRECT_OPS
>
> for the suggested modification in the Kconfig below.
>
> > +static struct bpf_trampoline *direct_ops_ip_lookup(struct ftrace_ops *ops, unsigned long ip)
> > +{
> > + struct hlist_head *head_ip;
> > + struct bpf_trampoline *tr;
> > +
> > + mutex_lock(&trampoline_mutex);
>
> guard(mutex)(&trampoline_mutex);
right, will change
>
> > + head_ip = &trampoline_ip_table[hash_64(ip, TRAMPOLINE_HASH_BITS)];
> > + hlist_for_each_entry(tr, head_ip, hlist_ip) {
> > + if (tr->ip == ip)
>
> return NULL;
>
> > + goto out;
> > + }
>
>
> > + tr = NULL;
> > +out:
> > + mutex_unlock(&trampoline_mutex);
>
> No need for the above
yep
>
> > + return tr;
> > +}
> > +#else
> > +static struct bpf_trampoline *direct_ops_ip_lookup(struct ftrace_ops *ops, unsigned long ip)
> > +{
> > + return ops->private;
> > +}
> > +#endif /* CONFIG_HAVE_SINGLE_FTRACE_DIRECT_OPS */
> > +
> > static int bpf_tramp_ftrace_ops_func(struct ftrace_ops *ops, unsigned long ip,
> > enum ftrace_ops_cmd cmd)
> > {
> > - struct bpf_trampoline *tr = ops->private;
> > + struct bpf_trampoline *tr;
> > int ret = 0;
> >
> > + tr = direct_ops_ip_lookup(ops, ip);
> > + if (!tr)
> > + return -EINVAL;
> > +
> > if (cmd == FTRACE_OPS_CMD_ENABLE_SHARE_IPMODIFY_SELF) {
> > /* This is called inside register_ftrace_direct_multi(), so
> > * tr->mutex is already locked.
> > @@ -137,6 +165,139 @@ void bpf_image_ksym_del(struct bpf_ksym *ksym)
> > PAGE_SIZE, true, ksym->name);
> > }
> >
> > +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> > +#ifdef CONFIG_HAVE_SINGLE_FTRACE_DIRECT_OPS
>
> Replace the above two with:
>
> #ifdef CONFIG_SINGLE_FTRACE_DIRECT_OPS
...
>
> > +/*
> > + * We have only single direct_ops which contains all the direct call
> > + * sites and is the only global ftrace_ops for all trampolines.
> > + *
> > + * We use 'update_ftrace_direct_*' api for attachment.
> > + */
> > +struct ftrace_ops direct_ops = {
> > + .ops_func = bpf_tramp_ftrace_ops_func,
> > +};
> > +
> > +static int direct_ops_alloc(struct bpf_trampoline *tr)
> > +{
> > + tr->fops = &direct_ops;
> > + return 0;
> > +}
> > +
> > +static void direct_ops_free(struct bpf_trampoline *tr) { }
> > +
> > +static struct ftrace_hash *hash_from_ip(struct bpf_trampoline *tr, void *ptr)
> > +{
> > + unsigned long ip, addr = (unsigned long) ptr;
> > + struct ftrace_hash *hash;
> > +
> > + ip = ftrace_location(tr->ip);
> > + if (!ip)
> > + return NULL;
> > + hash = alloc_ftrace_hash(FTRACE_HASH_DEFAULT_BITS);
> > + if (!hash)
> > + return NULL;
> > + if (bpf_trampoline_use_jmp(tr->flags))
> > + addr = ftrace_jmp_set(addr);
> > + if (!add_hash_entry_direct(hash, ip, addr)) {
> > + free_ftrace_hash(hash);
> > + return NULL;
> > + }
> > + return hash;
> > +}
> > +
> > +static int direct_ops_add(struct bpf_trampoline *tr, void *addr)
> > +{
> > + struct ftrace_hash *hash = hash_from_ip(tr, addr);
> > + int err = -ENOMEM;
> > +
> > + if (hash)
> > + err = update_ftrace_direct_add(tr->fops, hash);
> > + free_ftrace_hash(hash);
> > + return err;
> > +}
>
> I think these functions would be cleaner as:
>
> {
> struct ftrace_hash *hash = hash_from_ip(tr, addr);
> int err;
>
> if (!hash)
> return -ENOMEM;
>
> err = update_ftrace_direct_*(tr->fops, hash);
> free_ftrace_hash(hash);
> return err;
> }
np, will change
>
>
> > +
> > +static int direct_ops_del(struct bpf_trampoline *tr, void *addr)
> > +{
> > + struct ftrace_hash *hash = hash_from_ip(tr, addr);
> > + int err = -ENOMEM;
> > +
> > + if (hash)
> > + err = update_ftrace_direct_del(tr->fops, hash);
> > + free_ftrace_hash(hash);
> > + return err;
> > +}
> > +
> > +static int direct_ops_mod(struct bpf_trampoline *tr, void *addr, bool lock_direct_mutex)
> > +{
> > + struct ftrace_hash *hash = hash_from_ip(tr, addr);
> > + int err = -ENOMEM;
> > +
> > + if (hash)
> > + err = update_ftrace_direct_mod(tr->fops, hash, lock_direct_mutex);
> > + free_ftrace_hash(hash);
> > + return err;
> > +}
> > +#else
> > +/*
> > + * We allocate ftrace_ops object for each trampoline and it contains
> > + * call site specific for that trampoline.
> > + *
> > + * We use *_ftrace_direct api for attachment.
> > + */
> > +static int direct_ops_alloc(struct bpf_trampoline *tr)
> > +{
> > + tr->fops = kzalloc(sizeof(struct ftrace_ops), GFP_KERNEL);
> > + if (!tr->fops)
> > + return -ENOMEM;
> > + tr->fops->private = tr;
> > + tr->fops->ops_func = bpf_tramp_ftrace_ops_func;
> > + return 0;
> > +}
> > +
> > +static void direct_ops_free(struct bpf_trampoline *tr)
> > +{
> > + if (tr->fops) {
> > + ftrace_free_filter(tr->fops);
> > + kfree(tr->fops);
> > + }
> > +}
>
> Why not:
>
> static void direct_ops_free(struct bpf_trampoline *tr)
> {
> if (!tr->fops)
> return;
>
> ftrace_free_filter(tr->fops);
> kfree(tr->fops);
> }
same pattern like above, ok
>
> ?
>
> > +
> > +static int direct_ops_add(struct bpf_trampoline *tr, void *ptr)
> > +{
> > + unsigned long addr = (unsigned long) ptr;
> > + struct ftrace_ops *ops = tr->fops;
> > + int ret;
> > +
> > + if (bpf_trampoline_use_jmp(tr->flags))
> > + addr = ftrace_jmp_set(addr);
> > +
> > + ret = ftrace_set_filter_ip(ops, tr->ip, 0, 1);
> > + if (ret)
> > + return ret;
> > + return register_ftrace_direct(ops, addr);
> > +}
> > +
> > +static int direct_ops_del(struct bpf_trampoline *tr, void *addr)
> > +{
> > + return unregister_ftrace_direct(tr->fops, (long)addr, false);
> > +}
> > +
> > +static int direct_ops_mod(struct bpf_trampoline *tr, void *ptr, bool lock_direct_mutex)
> > +{
> > + unsigned long addr = (unsigned long) ptr;
> > + struct ftrace_ops *ops = tr->fops;
> > +
> > + if (bpf_trampoline_use_jmp(tr->flags))
> > + addr = ftrace_jmp_set(addr);
> > + if (lock_direct_mutex)
> > + return modify_ftrace_direct(ops, addr);
> > + return modify_ftrace_direct_nolock(ops, addr);
> > +}
> > +#endif /* CONFIG_HAVE_SINGLE_FTRACE_DIRECT_OPS */
> > +#else
> > +static void direct_ops_free(struct bpf_trampoline *tr) { }
>
> This is somewhat inconsistent with direct_ops_alloc() that has:
>
> #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> if (direct_ops_alloc(tr)) {
> kfree(tr);
> tr = NULL;
> goto out;
> }
> #endif
>
> Now, if you wrap the direct_ops_free() too, we can remove the
> #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> part with my kconfig suggestion. Otherwise keep the kconfig as is, but I
> would add a stub function for direct_ops_alloc() too.
ah right.. I think let's do the kconfig change you suggest to make
this simpler
>
> > +#endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
> > +
> > static struct bpf_trampoline *bpf_trampoline_lookup(u64 key, unsigned long ip)
> > {
> > struct bpf_trampoline *tr;
> > @@ -155,14 +316,11 @@ static struct bpf_trampoline *bpf_trampoline_lookup(u64 key, unsigned long ip)
> > if (!tr)
> > goto out;
> > #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> > - tr->fops = kzalloc(sizeof(struct ftrace_ops), GFP_KERNEL);
> > - if (!tr->fops) {
> > + if (direct_ops_alloc(tr)) {
> > kfree(tr);
> > tr = NULL;
> > goto out;
> > }
> > - tr->fops->private = tr;
> > - tr->fops->ops_func = bpf_tramp_ftrace_ops_func;
> > #endif
> >
> > tr->key = key;
> > @@ -206,7 +364,7 @@ static int unregister_fentry(struct bpf_trampoline *tr, u32 orig_flags,
> > int ret;
> >
> > if (tr->func.ftrace_managed)
> > - ret = unregister_ftrace_direct(tr->fops, (long)old_addr, false);
> > + ret = direct_ops_del(tr, old_addr);
>
> Doesn't this need a wrapper too?
yep
>
> > else
> > ret = bpf_trampoline_update_fentry(tr, orig_flags, old_addr, NULL);
> >
> > @@ -220,15 +378,7 @@ 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);
> > -
> > - if (lock_direct_mutex)
> > - ret = modify_ftrace_direct(tr->fops, addr);
> > - else
> > - ret = modify_ftrace_direct_nolock(tr->fops, addr);
> > + ret = direct_ops_mod(tr, new_addr, lock_direct_mutex);
>
> and this.
>
> > } else {
> > ret = bpf_trampoline_update_fentry(tr, orig_flags, old_addr,
> > new_addr);
> > @@ -251,15 +401,7 @@ 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);
> > -
> > - ret = ftrace_set_filter_ip(tr->fops, (unsigned long)ip, 0, 1);
> > - if (ret)
> > - return ret;
> > - ret = register_ftrace_direct(tr->fops, addr);
> > + ret = direct_ops_add(tr, new_addr);
>
> Ditto.
yes
>
> > } else {
> > ret = bpf_trampoline_update_fentry(tr, 0, NULL, new_addr);
> > }
> > @@ -910,10 +1052,7 @@ void bpf_trampoline_put(struct bpf_trampoline *tr)
> > */
> > hlist_del(&tr->hlist_key);
> > hlist_del(&tr->hlist_ip);
> > - if (tr->fops) {
> > - ftrace_free_filter(tr->fops);
> > - kfree(tr->fops);
> > - }
> > + direct_ops_free(tr);
> > kfree(tr);
> > out:
> > mutex_unlock(&trampoline_mutex);
> > diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
> > index 4661b9e606e0..1ad2e307c834 100644
> > --- a/kernel/trace/Kconfig
> > +++ b/kernel/trace/Kconfig
> > @@ -50,6 +50,9 @@ config HAVE_DYNAMIC_FTRACE_WITH_REGS
> > config HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> > bool
> >
> > +config HAVE_SINGLE_FTRACE_DIRECT_OPS
> > + bool
> > +
>
> Now you could add:
>
> config SINGLE_FTRACE_DIRECT_OPS
> bool
> default y
> depends on HAVE_SINGLE_FTRACE_DIRECT_OPS && DYNAMIC_FTRACE_WITH_DIRECT_CALLS
ok, the dependency is more ovbvious, will change
thanks,
jirka
next prev parent reply other threads:[~2025-12-19 9:28 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-12-15 21:13 [PATCHv5 bpf-next 0/9] ftrace,bpf: Use single direct ops for bpf trampolines Jiri Olsa
2025-12-15 21:13 ` [PATCHv5 bpf-next 1/9] ftrace,bpf: Remove FTRACE_OPS_FL_JMP ftrace_ops flag Jiri Olsa
2025-12-15 21:31 ` bot+bpf-ci
2025-12-16 1:27 ` Menglong Dong
2025-12-17 8:40 ` Jiri Olsa
2025-12-15 21:13 ` [PATCHv5 bpf-next 2/9] ftrace: Make alloc_and_copy_ftrace_hash direct friendly Jiri Olsa
2025-12-15 21:13 ` [PATCHv5 bpf-next 3/9] ftrace: Export some of hash related functions Jiri Olsa
2025-12-18 1:07 ` Steven Rostedt
2025-12-19 9:27 ` Jiri Olsa
2025-12-15 21:13 ` [PATCHv5 bpf-next 4/9] ftrace: Add update_ftrace_direct_add function Jiri Olsa
2025-12-16 14:32 ` kernel test robot
2025-12-18 1:39 ` Steven Rostedt
2025-12-19 9:27 ` Jiri Olsa
2025-12-15 21:13 ` [PATCHv5 bpf-next 5/9] ftrace: Add update_ftrace_direct_del function Jiri Olsa
2025-12-18 1:48 ` Steven Rostedt
2025-12-19 9:27 ` Jiri Olsa
2025-12-15 21:13 ` [PATCHv5 bpf-next 6/9] ftrace: Add update_ftrace_direct_mod function Jiri Olsa
2025-12-18 15:19 ` Steven Rostedt
2025-12-18 15:41 ` Steven Rostedt
2025-12-19 9:27 ` Jiri Olsa
2025-12-15 21:14 ` [PATCHv5 bpf-next 7/9] bpf: Add trampoline ip hash table Jiri Olsa
2025-12-15 21:14 ` [PATCHv5 bpf-next 8/9] ftrace: Factor ftrace_ops ops_func interface Jiri Olsa
2025-12-18 16:06 ` Steven Rostedt
2025-12-15 21:14 ` [PATCHv5 bpf-next 9/9] bpf,x86: Use single ftrace_ops for direct calls Jiri Olsa
2025-12-18 16:26 ` Steven Rostedt
2025-12-19 9:27 ` Jiri Olsa [this message]
2025-12-28 15:22 ` Jiri Olsa
2025-12-29 16:03 ` Steven Rostedt
2025-12-20 19:38 ` kernel test robot
2025-12-21 11:09 ` kernel test robot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aUUanPijlWsDlS0X@krava \
--to=olsajiri@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-trace-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=menglong8.dong@gmail.com \
--cc=revest@google.com \
--cc=rostedt@goodmis.org \
--cc=rostedt@kernel.org \
--cc=song@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.