Re: [PATCH bpf-next 7/8] selftests/bpf: add hashmap test for bpf_for_each_map_elem() helper

[Yonghong Song <yhs@fb.com>]

On 2/8/21 10:34 AM, Andrii Nakryiko wrote:
> On Thu, Feb 4, 2021 at 5:53 PM Yonghong Song <yhs@fb.com> wrote:
>>
>> A test case is added for hashmap and percpu hashmap. The test
>> also exercises nested bpf_for_each_map_elem() calls like
>>      bpf_prog:
>>        bpf_for_each_map_elem(func1)
>>      func1:
>>        bpf_for_each_map_elem(func2)
>>      func2:
>>
>>    $ ./test_progs -n 44
>>    #44/1 hash_map:OK
>>    #44 for_each:OK
>>    Summary: 1/1 PASSED, 0 SKIPPED, 0 FAILED
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>>   .../selftests/bpf/prog_tests/for_each.c       |  91 ++++++++++++++++
>>   .../bpf/progs/for_each_hash_map_elem.c        | 103 ++++++++++++++++++
>>   2 files changed, 194 insertions(+)
>>   create mode 100644 tools/testing/selftests/bpf/prog_tests/for_each.c
>>   create mode 100644 tools/testing/selftests/bpf/progs/for_each_hash_map_elem.c
>>
> 
> [...]
> 
>> +       num_cpus = bpf_num_possible_cpus();
>> +       percpu_map_fd = bpf_map__fd(skel->maps.percpu_map);
>> +       percpu_valbuf = malloc(sizeof(__u64) * num_cpus);
>> +       if (CHECK_FAIL(!percpu_valbuf))
>> +               goto out;
>> +
>> +       key = 1;
>> +       for (i = 0; i < num_cpus; i++)
>> +               percpu_valbuf[i] = i + 1;
>> +       err = bpf_map_update_elem(percpu_map_fd, &key, percpu_valbuf, BPF_ANY);
>> +       if (CHECK(err, "percpu_map_update", "map_update failed\n"))
>> +               goto out;
>> +
>> +       do_dummy_read(skel->progs.dump_task);
> 
> why use iter/task programs to trigger your test BPF code? This test
> doesn't seem to rely on anything iter-specific, so it's much simpler
> (and less code) to just use the typical sys_enter approach with
> usleep(1) as a trigger function, no?

I am aware of this. I did not change this in v1 mainly wanting to
get some comments on API and verifier change etc. for v1.
I will use bpf_prog_test_run() to call the program in v2.

> 
>> +
>> +       ASSERT_EQ(skel->bss->called, 1, "called");
>> +       ASSERT_EQ(skel->bss->hashmap_output, 4, "output_val");
>> +
>> +       key = 1;
>> +       err = bpf_map_lookup_elem(hashmap_fd, &key, &val);
>> +       ASSERT_ERR(err, "hashmap_lookup");
>> +
>> +       ASSERT_EQ(skel->bss->percpu_called, 1, "percpu_called");
>> +       CHECK_FAIL(skel->bss->cpu >= num_cpus);
> 
> please don't use CHECK_FAIL: use CHECK or one of ASSERT_xxx variants

We do not have ASSERT_GE, I may invent one.

> 
>> +       ASSERT_EQ(skel->bss->percpu_key, 1, "percpu_key");
>> +       ASSERT_EQ(skel->bss->percpu_val, skel->bss->cpu + 1, "percpu_val");
>> +       ASSERT_EQ(skel->bss->percpu_output, 100, "percpu_output");
>> +out:
>> +       free(percpu_valbuf);
>> +       for_each_hash_map_elem__destroy(skel);
>> +}
>> +
>> +void test_for_each(void)
>> +{
>> +       if (test__start_subtest("hash_map"))
>> +               test_hash_map();
>> +}
> 
> [...]
> 
>> +
>> +__u32 cpu = 0;
>> +__u32 percpu_called = 0;
>> +__u32 percpu_key = 0;
>> +__u64 percpu_val = 0;
>> +
>> +static __u64
>> +check_percpu_elem(struct bpf_map *map, __u32 *key, __u64 *val,
>> +                 struct callback_ctx *data)
>> +{
>> +       percpu_called++;
>> +       cpu = bpf_get_smp_processor_id();
> 
> It's a bit counter-intuitive (at least I was confused initially) that
> for a per-cpu array for_each() will iterate only current CPU's
> elements. I think it's worthwhile to emphasize this in
> bpf_for_each_map_elem() documentation (at least), and call out in
> corresponding patches adding per-cpu array/hash iteration support.

Right. Will emphasize this in uapi bpf.h and also comments in the code.

> 
>> +       percpu_key = *key;
>> +       percpu_val = *val;
>> +
>> +       bpf_for_each_map_elem(&hashmap, check_hash_elem, data, 0);
>> +       return 0;
>> +}
>> +
> 
> [...]
>