Re: [PATCH bpf-next v4 2/2] selftests/bpf: Add stacktrace map lookup_and_delete_elem test case

[Tao Chen <chen.dylane@linux.dev>]

在 2025/9/23 04:58, Andrii Nakryiko 写道:
> On Mon, Sep 22, 2025 at 7:03 AM Tao Chen <chen.dylane@linux.dev> wrote:
>>
>> Add tests for stacktrace map lookup and delete:
>> 1. use bpf_map_lookup_and_delete_elem to lookup and delete the target
>>     stack_id,
>> 2. lookup the deleted stack_id again to double check.
>>
>> Signed-off-by: Tao Chen <chen.dylane@linux.dev>
>> ---
>>   .../selftests/bpf/prog_tests/stacktrace_map.c | 21 ++++++++++++++++++-
>>   .../selftests/bpf/progs/test_stacktrace_map.c |  8 +++++++
>>   2 files changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/stacktrace_map.c b/tools/testing/selftests/bpf/prog_tests/stacktrace_map.c
>> index 84a7e405e91..d50659fc25e 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/stacktrace_map.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/stacktrace_map.c
>> @@ -3,7 +3,7 @@
>>
>>   void test_stacktrace_map(void)
>>   {
>> -       int control_map_fd, stackid_hmap_fd, stackmap_fd, stack_amap_fd;
>> +       int control_map_fd, stackid_hmap_fd, stackmap_fd, stack_amap_fd, stack_key_map_fd;
>>          const char *prog_name = "oncpu";
>>          int err, prog_fd, stack_trace_len;
>>          const char *file = "./test_stacktrace_map.bpf.o";
>> @@ -11,6 +11,8 @@ void test_stacktrace_map(void)
>>          struct bpf_program *prog;
>>          struct bpf_object *obj;
>>          struct bpf_link *link;
>> +       __u32 stack_id;
>> +       char val_buf[PERF_MAX_STACK_DEPTH * sizeof(struct bpf_stack_build_id)];
>>
>>          err = bpf_prog_test_load(file, BPF_PROG_TYPE_TRACEPOINT, &obj, &prog_fd);
>>          if (CHECK(err, "prog_load", "err %d errno %d\n", err, errno))
>> @@ -41,6 +43,10 @@ void test_stacktrace_map(void)
>>          if (CHECK_FAIL(stack_amap_fd < 0))
>>                  goto disable_pmu;
>>
>> +       stack_key_map_fd = bpf_find_map(__func__, obj, "stack_key_map");
>> +       if (CHECK_FAIL(stack_key_map_fd < 0))
> 
> please don't use CHECK*() macros, they are superseded by more targeted
> ASSERT_xxx() ones
> 

got it, thanks.

> pw-bot: cr
> 
> 
>> +               goto disable_pmu;
>> +
>>          /* give some time for bpf program run */
>>          sleep(1);
>>
>> @@ -68,6 +74,19 @@ void test_stacktrace_map(void)
>>                    "err %d errno %d\n", err, errno))
>>                  goto disable_pmu;
>>
>> +       err = bpf_map_lookup_elem(stack_key_map_fd, &key, &stack_id);
>> +       if (CHECK(err, "stack_key_map lookup", "err %d errno %d\n", err, errno))
>> +               goto disable_pmu;
>> +
>> +       err = bpf_map_lookup_and_delete_elem(stackmap_fd, &stack_id, &val_buf);
>> +       if (CHECK(err, "stackmap lookup and delete",
>> +                 "err %d errno %d\n", err, errno))
>> +               goto disable_pmu;
>> +
>> +       err = bpf_map_lookup_elem(stackmap_fd, &stack_id, &val_buf);
>> +       CHECK((!err || errno != ENOENT), "stackmap lookup deleted stack_id",
>> +             "err %d errno %d\n", err, errno);
> 
> bpf_map_lookup_elem() returns error code directly, no need to use
> errno, just check that err == -ENOENT
> 

ok.

>> +
>>   disable_pmu:
>>          bpf_link__destroy(link);
>>   close_prog:
>> diff --git a/tools/testing/selftests/bpf/progs/test_stacktrace_map.c b/tools/testing/selftests/bpf/progs/test_stacktrace_map.c
>> index 47568007b66..3bede76c151 100644
>> --- a/tools/testing/selftests/bpf/progs/test_stacktrace_map.c
>> +++ b/tools/testing/selftests/bpf/progs/test_stacktrace_map.c
>> @@ -38,6 +38,13 @@ struct {
>>          __type(value, stack_trace_t);
>>   } stack_amap SEC(".maps");
>>
>> +struct {
>> +       __uint(type, BPF_MAP_TYPE_ARRAY);
>> +       __uint(max_entries, 1);
>> +       __type(key, __u32);
>> +       __type(value, __u32);
>> +} stack_key_map SEC(".maps");
>> +
>>   /* taken from /sys/kernel/tracing/events/sched/sched_switch/format */
>>   struct sched_switch_args {
>>          unsigned long long pad;
>> @@ -64,6 +71,7 @@ int oncpu(struct sched_switch_args *ctx)
>>          /* The size of stackmap and stackid_hmap should be the same */
>>          key = bpf_get_stackid(ctx, &stackmap, 0);
>>          if ((int)key >= 0) {
>> +               bpf_map_update_elem(&stack_key_map, &val, &key, 0);
> 
> ugh... you'd just use a global variable if this test was used through
> skeleton... maybe convert the test to skeleton and get rid of all
> those unnecessary bpf_find_map() calls as well?
> 

Yes, the test case seems too old, i will refactor it with skeleton.
>>                  bpf_map_update_elem(&stackid_hmap, &key, &val, 0);
>>                  stack_p = bpf_map_lookup_elem(&stack_amap, &key);
>>                  if (stack_p)
>> --
>> 2.48.1
>>


-- 
Best Regards
Tao Chen