Message-ID: <CAMB2axOFqAUZooWNe2euQ-N3A0P83=PGj1p6_LtngN6PW9uQcw@mail.gmail.com>
Date: Fri, 9 Jan 2026 10:49:06 -0800
From: Amery Hung <ameryhung@...il.com>
To: Martin KaFai Lau <martin.lau@...ux.dev>
Cc: netdev@...r.kernel.org, alexei.starovoitov@...il.com, andrii@...nel.org, 
	daniel@...earbox.net, memxor@...il.com, martin.lau@...nel.org, 
	kpsingh@...nel.org, yonghong.song@...ux.dev, song@...nel.org, 
	haoluo@...gle.com, kernel-team@...a.com, bpf@...r.kernel.org
Subject: Re: [PATCH bpf-next v3 04/16] bpf: Convert bpf_selem_unlink to failable

On Fri, Jan 9, 2026 at 10:16 AM Martin KaFai Lau <martin.lau@...ux.dev> wrote:
>
>
>
> On 12/18/25 9:56 AM, Amery Hung wrote:
> > To prepare changing both bpf_local_storage_map_bucket::lock and
> > bpf_local_storage::lock to rqspinlock, convert bpf_selem_unlink() to
> > failable. It still always succeeds and returns 0 until the change
> > happens. No functional change.
> >
> > For bpf_local_storage_map_free(), WARN_ON() for now as no real error
> > will happen until we switch to rqspinlock.
> >
> > __must_check is added to the function declaration locally to make sure
> > all callers are accounted for during the conversion.
>
> I don't see __must_check. The same for patch 2.

I only added it locally. I will follow your suggestion to include it
in the patchset.

Per your suggestion:
Ignore the warning instead of WARN_ON for now. Add __must_check to
functions when everything is ready.

>
> >
> > Signed-off-by: Amery Hung <ameryhung@...il.com>
> > ---
> >   include/linux/bpf_local_storage.h | 2 +-
> >   kernel/bpf/bpf_cgrp_storage.c     | 3 +--
> >   kernel/bpf/bpf_inode_storage.c    | 4 +---
> >   kernel/bpf/bpf_local_storage.c    | 8 +++++---
> >   kernel/bpf/bpf_task_storage.c     | 4 +---
> >   net/core/bpf_sk_storage.c         | 4 +---
> >   6 files changed, 10 insertions(+), 15 deletions(-)
> >
> > diff --git a/include/linux/bpf_local_storage.h b/include/linux/bpf_local_storage.h
> > index 6cabf5154cf6..a94e12ddd83d 100644
> > --- a/include/linux/bpf_local_storage.h
> > +++ b/include/linux/bpf_local_storage.h
> > @@ -176,7 +176,7 @@ int bpf_local_storage_map_check_btf(const struct bpf_map *map,
> >   void bpf_selem_link_storage_nolock(struct bpf_local_storage *local_storage,
> >                                  struct bpf_local_storage_elem *selem);
> >
> > -void bpf_selem_unlink(struct bpf_local_storage_elem *selem, bool reuse_now);
> > +int bpf_selem_unlink(struct bpf_local_storage_elem *selem, bool reuse_now);
> >
> >   int bpf_selem_link_map(struct bpf_local_storage_map *smap,
> >                      struct bpf_local_storage_elem *selem);
> > diff --git a/kernel/bpf/bpf_cgrp_storage.c b/kernel/bpf/bpf_cgrp_storage.c
> > index 0687a760974a..8fef24fcac68 100644
> > --- a/kernel/bpf/bpf_cgrp_storage.c
> > +++ b/kernel/bpf/bpf_cgrp_storage.c
> > @@ -118,8 +118,7 @@ static int cgroup_storage_delete(struct cgroup *cgroup, struct bpf_map *map)
> >       if (!sdata)
> >               return -ENOENT;
> >
> > -     bpf_selem_unlink(SELEM(sdata), false);
> > -     return 0;
> > +     return bpf_selem_unlink(SELEM(sdata), false);
> >   }
> >
> >   static long bpf_cgrp_storage_delete_elem(struct bpf_map *map, void *key)
> > diff --git a/kernel/bpf/bpf_inode_storage.c b/kernel/bpf/bpf_inode_storage.c
> > index e54cce2b9175..cedc99184dad 100644
> > --- a/kernel/bpf/bpf_inode_storage.c
> > +++ b/kernel/bpf/bpf_inode_storage.c
> > @@ -110,9 +110,7 @@ static int inode_storage_delete(struct inode *inode, struct bpf_map *map)
> >       if (!sdata)
> >               return -ENOENT;
> >
> > -     bpf_selem_unlink(SELEM(sdata), false);
> > -
> > -     return 0;
> > +     return bpf_selem_unlink(SELEM(sdata), false);
> >   }
> >
> >   static long bpf_fd_inode_storage_delete_elem(struct bpf_map *map, void *key)
> > diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c
> > index 0e3fa5fbaaf3..fa629a180e9e 100644
> > --- a/kernel/bpf/bpf_local_storage.c
> > +++ b/kernel/bpf/bpf_local_storage.c
> > @@ -367,7 +367,7 @@ static void bpf_selem_link_map_nolock(struct bpf_local_storage_map *smap,
> >       hlist_add_head_rcu(&selem->map_node, &b->list);
> >   }
> >
> > -void bpf_selem_unlink(struct bpf_local_storage_elem *selem, bool reuse_now)
> > +int bpf_selem_unlink(struct bpf_local_storage_elem *selem, bool reuse_now)
> >   {
> >       struct bpf_local_storage *local_storage;
> >       bool free_local_storage = false;
> > @@ -377,7 +377,7 @@ void bpf_selem_unlink(struct bpf_local_storage_elem *selem, bool reuse_now)
> >
> >       if (unlikely(!selem_linked_to_storage_lockless(selem)))
> >               /* selem has already been unlinked from sk */
> > -             return;
> > +             return 0;
> >
> >       local_storage = rcu_dereference_check(selem->local_storage,
> >                                             bpf_rcu_lock_held());
> > @@ -402,6 +402,8 @@ void bpf_selem_unlink(struct bpf_local_storage_elem *selem, bool reuse_now)
> >
> >       if (free_local_storage)
> >               bpf_local_storage_free(local_storage, reuse_now);
> > +
> > +     return err;
>
> err is not used in patch 3 and then becomes useful in patch 4. The
> ai-review discovered issue on err also. Squash patch 4 into patch 3. It
> will be easier to read.

Got it. Will squash patch 3 and 4.

>
> >   }
> >
> >   void __bpf_local_storage_insert_cache(struct bpf_local_storage *local_storage,
> > @@ -837,7 +839,7 @@ void bpf_local_storage_map_free(struct bpf_map *map,
> >                               struct bpf_local_storage_elem, map_node))) {
> >                       if (busy_counter)
> >                               this_cpu_inc(*busy_counter);
> > -                     bpf_selem_unlink(selem, true);
> > +                     WARN_ON(bpf_selem_unlink(selem, true));
>
> nit. I would add __must_check to the needed functions in a single patch
> when everything is ready instead of having an intermediate WARN_ON here
> and then removed it later.
>

Powered by blists - more mailing lists