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

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

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

quoted

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

quoted

A test is added for arraymap and percpu arraymap. The test also
exercises the early return for the helper which does not
traverse all elements.
     $ ./test_progs -n 44
     #44/1 hash_map:OK
     #44/2 array_map:OK
     #44 for_each:OK
     Summary: 1/2 PASSED, 0 SKIPPED, 0 FAILED

Signed-off-by: Yonghong Song <redacted>
---
  .../selftests/bpf/prog_tests/for_each.c       | 54 ++++++++++++++
  .../bpf/progs/for_each_array_map_elem.c       | 71 +++++++++++++++++++
  2 files changed, 125 insertions(+)
  create mode 100644 tools/testing/selftests/bpf/progs/for_each_array_map_elem.c

[...]

quoted

+
+       arraymap_fd = bpf_map__fd(skel->maps.arraymap);
+       expected_total = 0;
+       max_entries = bpf_map__max_entries(skel->maps.arraymap);
+       for (i = 0; i < max_entries; i++) {
+               key = i;
+               val = i + 1;
+               /* skip the last iteration for expected total */
+               if (i != max_entries - 1)
+                       expected_total += val;
+               err = bpf_map_update_elem(arraymap_fd, &key, &val, BPF_ANY);
+               if (CHECK(err, "map_update", "map_update failed\n"))
+                       goto out;
+       }
+
+       num_cpus = bpf_num_possible_cpus();
+        percpu_map_fd = bpf_map__fd(skel->maps.percpu_map);

formatting is off, please check with checkfile.pl

Sure. will do.

quoted

quoted

+        percpu_valbuf = malloc(sizeof(__u64) * num_cpus);
+        if (CHECK_FAIL(!percpu_valbuf))

please don't use CHECK_FAIL, ASSERT_PTR_OK would work nicely here

Okay.

quoted

quoted

+                goto out;
+
+       key = 0;
+        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);

see previous patch, iter/tasks seems like an overkill for these tests

Yes, will use bpf_prog_test_run() in v2.

quoted

quoted

+
+       ASSERT_EQ(skel->bss->called, 1, "called");
+       ASSERT_EQ(skel->bss->arraymap_output, expected_total, "array_output");
+       ASSERT_EQ(skel->bss->cpu + 1, skel->bss->percpu_val, "percpu_val");
+
+out:
+       free(percpu_valbuf);
+       for_each_array_map_elem__destroy(skel);
+}
+

[...]

quoted

+SEC("iter/task")
+int dump_task(struct bpf_iter__task *ctx)
+{
+       struct seq_file *seq =  ctx->meta->seq;
+       struct task_struct *task = ctx->task;
+       struct callback_ctx data;
+
+       /* only call once */
+       if (called > 0)
+               return 0;
+
+       called++;
+
+       data.output = 0;
+       bpf_for_each_map_elem(&arraymap, check_array_elem, &data, 0);
+       arraymap_output = data.output;
+
+       bpf_for_each_map_elem(&percpu_map, check_percpu_elem, 0, 0);

nit: NULL, 0 ?

We do not NULL defined in vmlinux.h or bpf_helpers.h. Hence I am using
0, I should at least use (void *)0 here, I think.

yeah, this NULL problem is annoying. Let's just add NULL to vmlinux.h,
you are not the first one to complain. We can do that separately from
this patch set, of course.

quoted

quoted

+
+       return 0;
+}
--
2.24.1