On Mon, Mar 24, 2025 at 6:29 AM Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote: > > On Mon, Mar 24, 2025 at 7:36 AM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote: > > > > ping ? > > Sorry for the delay. Still thinking about it. > The mix of cleanups and features make it difficult to evaluate. > Most bpf folks attend lsfmmbpf this week, so expect more delays. I looked at the patches and didn't find anything obviously wrong with them. I'm a bit worried how we implicitly assume that if it's not per-cpu, then htab_map_update_elem_in_place() will be working with FD hashtable and map->ops->map_fd_put_ptr() will be defined. Seems a bit error prone. But overall everything looks correct, and some of the refactorings (e.g. patch #1) are a nice clean up. Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx> BTW, while I was reading all that code, I got a question. Why is it specifically per-CPU maps that allow in-place updates for arbitrary values? What makes it special? The assumption that we only have one BPF program using value on the current CPU or something? If yes, what do we say about bpf_map_lookup_percpu_elem() executed from another CPU? Weird. Hopefully I'm missing something and it's not really just broken. > > > On 3/8/2025 9:51 PM, Hou Tao wrote: > > > From: Hou Tao <houtao1@xxxxxxxxxx> > > > > > > Hi, > > > > > > The motivation for the patch set comes from the question raised by Cody > > > Haas [1]. When trying to concurrently lookup and update an existing > > > element in a htab of maps, the lookup procedure may return -ENOENT > > > unexpectedly. The first revision of the patch set tried to resolve the > > > problem by making the insertion of the new element and the deletion of > > > the old element being atomic from the perspective of the lookup process. > > > While the solution would benefit all hash maps, it does not fully > > > resolved the problem due to the immediate reuse issue. Therefore, in v2 > > > of the patch set, it only fixes the problem for fd htab. > > > > > > Please see individual patches for details. Comments are always welcome. > > > > > > v2: > > > * only support atomic update for fd htab > > > > > > v1: https://lore.kernel.org/bpf/20250204082848.13471-1-hotforest@xxxxxxxxx > > > > > > [1]: https://lore.kernel.org/xdp-newbies/CAH7f-ULFTwKdoH_t2SFc5rWCVYLEg-14d1fBYWH2eekudsnTRg@xxxxxxxxxxxxxx/ > > > > > > Hou Tao (6): > > > bpf: Factor out htab_elem_value helper() > > > bpf: Rename __htab_percpu_map_update_elem to > > > htab_map_update_elem_in_place > > > bpf: Support atomic update for htab of maps > > > bpf: Add is_fd_htab() helper > > > bpf: Don't allocate per-cpu extra_elems for fd htab > > > selftests/bpf: Add test case for atomic update of fd htab > > > > > > kernel/bpf/hashtab.c | 148 +++++++------- > > > .../selftests/bpf/prog_tests/fd_htab_lookup.c | 192 ++++++++++++++++++ > > > .../selftests/bpf/progs/fd_htab_lookup.c | 25 +++ > > > 3 files changed, 289 insertions(+), 76 deletions(-) > > > create mode 100644 tools/testing/selftests/bpf/prog_tests/fd_htab_lookup.c > > > create mode 100644 tools/testing/selftests/bpf/progs/fd_htab_lookup.c > > > > >
