Re: [PATCH bpf-next v6 0/4] bpf: Add bpf_call_rcu() and bpf_call_rcu_tasks_trace()

["Kumar Kartikeya Dwivedi" <memxor@gmail.com>]

On Wed Sep 23, 2026 at 6:45 PM CEST, Andrii Nakryiko wrote:
> On Wed, Sep 23, 2026 at 4:29 AM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
>>
>> On Wed Sep 23, 2026 at 12:37 PM CEST, Puranjay Mohan wrote:
>> > On Wed, Sep 23, 2026 at 12:41 AM Andrii Nakryiko
>> > <andrii.nakryiko@gmail.com> wrote:
>> >>
>> >> On Tue, Sep 22, 2026 at 1:02 PM Puranjay Mohan <puranjay@kernel.org> wrote:
>> >> >
>> >> > Changelog:
>> >> > v5: https://lore.kernel.org/all/20260921191407.1742386-1-puranjay@kernel.org/
>> >> > Changes in v6:
>> >> > - Size struct bpf_rcu_head at 48 bytes rather than 64, matching what the
>> >> >   inline state uses (Alexei)
>> >> > - Trim patch 1's changelog: drop the reasoning about possible future
>> >> >   layouts and about what the other async kfuncs return
>> >> > - Rebase on bpf-next/master
>> >> >
>> >> > v4: https://lore.kernel.org/all/20260915154248.3612028-1-puranjay@kernel.org/
>> >> > Changes in v5:
>> >> > - Rename the "hash_map" subtest to "bad_map" so that it matches its
>> >> >   helper test_call_rcu_bad_map() (bpf-ci)
>> >> > - Bump the callback counter after chain_err in the selftest callback.
>> >> >   Userspace polls that counter and then reads chain_err, so it could
>> >> >   still see the initial value before the re-arm had stored one, which
>> >> >   let the chain subtest's assertion pass without checking anything
>> >> > - Say in patch 1 why embedding struct rcu_head in a uapi struct is
>> >> >   acceptable here (Mykyta, Paul, Alexei)
>> >> > - Rebase on bpf-next/master
>> >> > v3: https://lore.kernel.org/all/20260915143640.36292-1-puranjay@kernel.org/
>> >> > Changes in v4:
>> >> > - Drop an unrelated hunk in bpf_async_update_prog_callback() that turned
>> >> >   PTR_ERR(prog) into -EBADF.  That is the shared bpf_timer/bpf_wq path
>> >> >   and bpf_prog_inc_not_zero() returns -ENOENT, so it would have changed
>> >> >   the errno bpf_timer_set_callback() and bpf_wq_set_callback() report to
>> >> >   userspace (Sashiko).  No other changes from v3
>> >> > v2: https://lore.kernel.org/all/20260915114240.3269184-1-puranjay@kernel.org/
>> >> > Changes in v3:
>> >> > - Move the bpf_call_rcu_tasks_trace() verifier bits from patch 1 to
>> >> >   patch 3; patch 1 alone emitted "resolve_btfids: unresolved symbol
>> >> >   bpf_call_rcu_tasks_trace" (Sashiko)
>> >> > - Poll the callback counter with an acquire load (Sashiko)
>> >> > - teardown: v2 only checked that the program was eventually freed, which
>> >> >   passes even if nothing was ever armed.  Also assert that the chain ran,
>> >> >   and read the -EPERM back through an independent .bss fd
>> >> > - Use kern_sync_rcu() instead of open coding the grace-period wait
>> >> > - Return -EBADF rather than -ENOENT when the calling program is going
>> >> >   away, matching bpf_task_work_schedule()
>> >> > - mismatch_map now pins the bpf_rcu_head label the new code emits; it
>> >> >   passed with that branch removed.  Add a two_heads test, and wait for
>> >> >   each grace period separately in the chain test
>> >> > - Commit messages: correct the -EPERM parity claim, explain the inline
>> >> >   callback state and the struct size, motivate the tasks trace flavour
>> >> > v1: https://lore.kernel.org/all/20260907134552.1772405-1-puranjay@kernel.org/
>> >> > Changes in v2:
>> >> > - Rebase on bpf-next/master
>> >> > - Use rcu_read_lock_dont_migrate() over open coding (Alexei)
>> >> > - Improve re-arming selftest to detect failure (Sashiko)
>> >> >
>> >> > BPF programs that manage their own objects have no way to run their own
>> >> > logic once an RCU grace period has elapsed.  bpf_obj_drop() defers a
>> >> > free, but returning an index to an allocator or unpinning a resource
>> >> > once readers are done has no equivalent.  sched_ext's BPF library works
>> >> > around this today by pushing freed nodes onto a list and having a
>> >> > userspace thread call membarrier(MEMBARRIER_CMD_GLOBAL) and then run a
>> >> > BPF program to reclaim them; it is the first intended user.
>> >> >
>> >> > Add:
>> >> >
>> >> >         int bpf_call_rcu(struct bpf_rcu_head *rh, void *map,
>> >> >                          int (*callback)(struct bpf_map *map, void *key,
>> >> >                                          void *value));
>> >> >
>> >> > and bpf_call_rcu_tasks_trace(), same signature, which also waits for
>> >> > sleepable programs.
>> >> >
>> >> > @rh is a struct bpf_rcu_head embedded in a value of @map, so the
>> >> > callback runs as callback(map, key, value) for the element it lives in
>> >> > and needs no cookie.  The field is only accepted in BPF_MAP_TYPE_ARRAY,
>> >>
>> >> Isn't restricting this to BPF_MAP_TYPE_ARRAY unnecessarily restrictive
>> >> in practice? Do other APIs of similar kind have this limitation? E.g.,
>> >> bpf_task_work_schedule() or timer, I don't think they limit user to
>> >> just ARRAY maps, it's way too inflexible.
>> >
>> > Right, bpf_timer and bpf_task_work take HASH/LRU_HASH/ARRAY. They can
>> > because they can cancel: on element delete bpf_obj_free_fields() calls
>>
>> bpf_obj_cancel_fields(). bpf_obj_free_fields() is no longer called except when
>> the object is being freed finally.
>>
>> > bpf_task_work_cancel_and_free(), which reaches task_work_cancel(), so
>> > a pending callback never runs against a recycled element.
>> > A queued RCU callback cannot be cancelled. rcu_barrier() is the only
>> > thing that waits for one, and it sleeps, so it is not callable from an
>> > element delete. Array elements are never freed individually, which is
>> > what makes it safe.
>>
>> If cancel is a noop, can you not simply skip it on map update?
>>
>> It is a bit unfortunate we can't provide cancel semantics, it will be yet
>> another divergence from how all other async callbacks work...sigh.
>>
>> >
>> > I can think of a complicated way to support hash maps but that would
>> > need making the state dynamically allocated and finding a way to
>> > cancel the callbacks. But I would do it as a follow-up if there is a
>> > real use case.
>>
>> That would mean allocation and frees, it is already quite expensive as is for a
>> call_rcu() primitive (with the refcount bumps and atomics).
>
> maybe so, but as is ARRAY is a severe limitation and makes a lot of
> cases either unusable or requiring extra sequential ID allocation
> logic to remap some HASH map entry to index in an ARRAY map, just so
> you can call rcu callback for a given kernel object stored in HASH
> map.
>
> E.g., think about keep as set of sockets, or tasks, or whatnot in HASH
> or RHASHTABLE (especially the latter one with support for resizing).
> How would you do bpf_call_rcu() for them with unduly complications,
> limitations and a lot of waste (unlike HASH you can't have lazy memory
> allocation).

Yeah, I don't disagree. We would also need to support storing them in more map
types to make it woth with arenas (which was one of the motivations for the
feature).

Perhaps it is better to make cancel a noop / unsupported and skip it when a map
element is deleted. That will be the easiest path to enabling it, but it does
create (perhaps surprising) divergence from some of the other async callback
primitives.

That said, I do think performance should be a consideration for this API; maybe
unlike other ones, I would expect that this could be called very frequently. I
had concerns about prog refcount increment as well, but didn't really bring it
up since it is something that can be addressed after the fact (maybe using pcpu
refs). But once we promise supporting cancellation, it is hard to walk that
back. I think it's less of a concern for other async cb types. In practice,
except to support map semantics I don't know if anyone will use cancellation API
either, the kernel side never grew such support.

Anyway, overall the best path to me seems to be just enabling it as is and
declaring bpf_obj_cancel_fields() on this noop. Existing synchronization should
be enough to ensure callback stops getting issued once element is finally freed
back to kernel allocator.