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

On Mon, Feb 8, 2021 at 10:46 PM Yonghong Song [off-list ref] wrote:

On 2/8/21 10:34 AM, Andrii Nakryiko wrote:

quoted

On Thu, Feb 4, 2021 at 5:53 PM Yonghong Song [off-list ref] wrote:

quoted

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 <redacted>
---
  .../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

[...]

quoted

+       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.

quoted

quoted

+
+       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.

Yeah, it has come up multiple times now. It would be great to have all
the inequality variants so that we can stop adding new CHECK()s which
has notoriously confusing semantics. Thanks!

quoted

quoted

+       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();
+}

[...]

quoted

+
+__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.

quoted

quoted

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

[...]