Hi, On 3/29/2025 6:05 AM, Andrii Nakryiko wrote: > 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> Thanks for looking at the patch set and acking it. Since bpf CI had dropped the patch set, I will resend it next week. > > 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? I think part of the reason is that the update procedure will disable interruption on current CPU, therefore, there will be not concurrent lookup procedure on this CPU. > > 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. Er, considering that bpf_map_lookup_percpu_elem() can be used to lookup element from any CPU, there may be concurrent update on CPU A and lookup_percpu on CPU B, therefore, the lookup_percpu procedure may read a partial updated value. It seem there is no better way to handle it. We could do out-of-place update by allocation a new per-cpu pointer, however due to the immediate reuse, the lookup_percpu procedure may read a reused value in the per-cpu pointer. > > >>> 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 >>>>
