Re: [PATCH bpf-next 02/17] bpf: Use mutex lock pool for bpf trampolines

[Jiri Olsa <olsajiri@gmail.com>]

On Mon, Feb 23, 2026 at 11:35:29AM -0800, Alexei Starovoitov wrote:
> On Sun, Feb 22, 2026 at 6:34 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Fri, Feb 20, 2026 at 11:58:13AM -0800, Alexei Starovoitov wrote:
> > > On Fri, Feb 20, 2026 at 2:07 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > > >
> > > > Adding mutex lock pool that replaces bpf trampolines mutex.
> > > >
> > > > For tracing_multi link coming in following changes we need to lock all
> > > > the involved trampolines during the attachment. This could mean thousands
> > > > of mutex locks, which is not convenient.
> > > >
> > > > As suggested by Andrii we can replace bpf trampolines mutex with mutex
> > > > pool, where each trampoline is hash-ed to one of the locks from the pool.
> > > >
> > > > It's better to lock all the pool mutexes (64 at the moment) than
> > > > thousands of them.
> > > >
> > > > Removing the mutex_is_locked in bpf_trampoline_put, because we removed
> > > > the mutex from bpf_trampoline.
> > > >
> > > > Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > ---
> > > >  include/linux/bpf.h     |  2 --
> > > >  kernel/bpf/trampoline.c | 74 +++++++++++++++++++++++++++++++----------
> > > >  2 files changed, 56 insertions(+), 20 deletions(-)
> > > >
> > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > > index cd9b96434904..46bf3d86bdb2 100644
> > > > --- a/include/linux/bpf.h
> > > > +++ b/include/linux/bpf.h
> > > > @@ -1335,8 +1335,6 @@ struct bpf_trampoline {
> > > >         /* hlist for trampoline_ip_table */
> > > >         struct hlist_node hlist_ip;
> > > >         struct ftrace_ops *fops;
> > > > -       /* serializes access to fields of this trampoline */
> > > > -       struct mutex mutex;
> > > >         refcount_t refcnt;
> > > >         u32 flags;
> > > >         u64 key;
> > > > diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> > > > index 952cd7932461..05dc0358654d 100644
> > > > --- a/kernel/bpf/trampoline.c
> > > > +++ b/kernel/bpf/trampoline.c
> > > > @@ -30,6 +30,45 @@ static struct hlist_head trampoline_ip_table[TRAMPOLINE_TABLE_SIZE];
> > > >  /* serializes access to trampoline tables */
> > > >  static DEFINE_MUTEX(trampoline_mutex);
> > > >
> > > > +#define TRAMPOLINE_LOCKS_BITS 6
> > > > +#define TRAMPOLINE_LOCKS_TABLE_SIZE (1 << TRAMPOLINE_LOCKS_BITS)
> > > > +
> > > > +static struct {
> > > > +       struct mutex mutex;
> > > > +       struct lock_class_key key;
> > > > +} *trampoline_locks;
> > > > +
> > > > +static struct mutex *trampoline_locks_lookup(struct bpf_trampoline *tr)
> > >
> > > select_trampoline_lock() ?
> >
> > ok
> >
> > >
> > > > +{
> > > > +       return &trampoline_locks[hash_64((u64) tr, TRAMPOLINE_LOCKS_BITS)].mutex;
> > > > +}
> > > > +
> > > > +static void trampoline_lock(struct bpf_trampoline *tr)
> > > > +{
> > > > +       mutex_lock(trampoline_locks_lookup(tr));
> > > > +}
> > > > +
> > > > +static void trampoline_unlock(struct bpf_trampoline *tr)
> > > > +{
> > > > +       mutex_unlock(trampoline_locks_lookup(tr));
> > > > +}
> > > > +
> > > > +static int __init trampoline_locks_init(void)
> > > > +{
> > > > +       int i;
> > > > +
> > > > +       trampoline_locks = kmalloc_array(TRAMPOLINE_LOCKS_TABLE_SIZE,
> > > > +                                        sizeof(trampoline_locks[0]), GFP_KERNEL);
> > >
> > > why bother with memory allocation? This is just 64 mutexes.
> >
> > ok, I could probably use __mutex_init directly for static key
> >
> > about 64.. not sure how I missed that but there's lockdep limit for
> > maximum locks depth and it's 48.. so we'll need to use 32 locks,
> > which is probably still ok
> >
> > >
> > > > +       if (!trampoline_locks)
> > > > +               return -ENOMEM;
> > > > +
> > > > +       for (i = 0; i < TRAMPOLINE_LOCKS_TABLE_SIZE; i++) {
> > > > +               lockdep_register_key(&trampoline_locks[i].key);
> > >
> > > why special key?
> >
> > if we keep single key we will get lockdep 'recursive locking' warning
> > during bpf_trampoline_multi_attach, because lockdep will think we lock
> > the same mutex
> >
> > there's support to annotate nested locking with mutex_lock_nested but
> > it allows maximum of 8 nested instances
> 
> yeah. subclass limit of 8 is there for a different use case.
> 
> 
> I guess you never validated your earlier approach of "let's take
> all trampoline mutexes" with lockdep ? ;)

nope, the rfc had workaround for lockdep ;-)

+#ifdef CONFIG_LOCKDEP
+	mutex_init_with_key(&tr->mutex, &__lockdep_no_track__);
+#else

but I overlooked lockdep config for this version

> MAX_LOCK_DEPTH is indeed 48.
> 
> See fs/configfs/inode.c and default_group_class.
> It does:
>                         lockdep_set_class(&inode->i_rwsem,
>                                           &default_group_class[depth - 1]);
> 
> the idea here is that the number of lockdep keys doesn't have
> to be equal to the actual number of mutexes.

I see, thanks for the pointer

> 
> I guess we can keep a total of 32 mutexes to avoid making it too fancy.
> Please add a comment explaining 32 and why it needs lockdep_key.

ok

> 
> I thought declaring all mutexes as static will avoid the need for the key,
> but DEFINE_MUTEX doesn't support an array.
> So since we need a loop anyway to init mutex and the key,
> let's keep kmalloc_array() above. Which is now renamed to kmalloc_objs()
> after 7.0-rc1.

I don't mind either way, meanwhile I used this version:

static struct {
       struct mutex mutex;
       struct lock_class_key key;
} trampoline_locks[TRAMPOLINE_LOCKS_TABLE_SIZE];

       for (i = 0; i < TRAMPOLINE_LOCKS_TABLE_SIZE; i++)
               __mutex_init(&trampoline_locks[i].mutex, "trampoline_lock", &trampoline_locks[i].key);

thanks,
jirka