On Mon, Feb 8, 2021 at 10:46 PM Yonghong Song <yhs@xxxxxx> wrote:
>
>
>
> On 2/8/21 10:34 AM, Andrii Nakryiko wrote:
> > On Thu, Feb 4, 2021 at 5:53 PM Yonghong Song <yhs@xxxxxx> 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@xxxxxx>
> >> ---
> >> .../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.
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!
>
> >
> >> + 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;
> >> +}
> >> +
> >
> > [...]
> >
