On Fri, Apr 5, 2024 at 10:36 AM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Thu, Apr 4, 2024 at 7:55 PM Philo Lu <lulie@xxxxxxxxxxxxxxxxx> wrote:
> >
> > A test is added for bpf_for_each_map_elem() with either an arraymap or a
> > hashmap.
> > $ tools/testing/selftests/bpf/test_progs -t for_each
> > #93/1 for_each/hash_map:OK
> > #93/2 for_each/array_map:OK
> > #93/3 for_each/write_map_key:OK
> > #93/4 for_each/multi_maps:OK
> > #93 for_each:OK
> > Summary: 1/4 PASSED, 0 SKIPPED, 0 FAILED
> >
> > Signed-off-by: Philo Lu <lulie@xxxxxxxxxxxxxxxxx>
> > ---
> > .../selftests/bpf/prog_tests/for_each.c | 62 +++++++++++++++++++
> > .../selftests/bpf/progs/for_each_multi_maps.c | 49 +++++++++++++++
> > 2 files changed, 111 insertions(+)
> > create mode 100644 tools/testing/selftests/bpf/progs/for_each_multi_maps.c
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/for_each.c b/tools/testing/selftests/bpf/prog_tests/for_each.c
> > index 8963f8a549f2..09f6487f58b9 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/for_each.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/for_each.c
> > @@ -5,6 +5,7 @@
> > #include "for_each_hash_map_elem.skel.h"
> > #include "for_each_array_map_elem.skel.h"
> > #include "for_each_map_elem_write_key.skel.h"
> > +#include "for_each_multi_maps.skel.h"
> >
> > static unsigned int duration;
> >
> > @@ -143,6 +144,65 @@ static void test_write_map_key(void)
> > for_each_map_elem_write_key__destroy(skel);
> > }
> >
> > +static void test_multi_maps(void)
> > +{
> > + struct for_each_multi_maps *skel;
> > + __u64 val, array_total, hash_total;
> > + __u32 key, max_entries;
> > + int i, err;
> > +
> > + LIBBPF_OPTS(bpf_test_run_opts, topts,
> > + .data_in = &pkt_v4,
> > + .data_size_in = sizeof(pkt_v4),
> > + .repeat = 1,
> > + );
> > +
> > + skel = for_each_multi_maps__open_and_load();
> > + if (!ASSERT_OK_PTR(skel, "for_each_multi_maps__open_and_load"))
> > + return;
> > +
> > + array_total = 0;
> > + max_entries = bpf_map__max_entries(skel->maps.arraymap);
> > + for (i = 0; i < max_entries; i++) {
> > + key = i;
> > + val = i + 1;
> > + array_total += val;
> > + err = bpf_map__update_elem(skel->maps.arraymap, &key, sizeof(key),
> > + &val, sizeof(val), BPF_ANY);
> > + if (!ASSERT_OK(err, "array_map_update"))
> > + goto out;
> > + }
> > +
> > + hash_total = 0;
> > + max_entries = bpf_map__max_entries(skel->maps.hashmap);
> > + for (i = 0; i < max_entries; i++) {
> > + key = i + 100;
> > + val = i + 1;
> > + hash_total += val;
> > + err = bpf_map__update_elem(skel->maps.hashmap, &key, sizeof(key),
> > + &val, sizeof(val), BPF_ANY);
> > + if (!ASSERT_OK(err, "hash_map_update"))
> > + goto out;
> > + }
> > +
> > + skel->bss->data_output = 0;
> > + skel->bss->use_array = 1;
> > + err = bpf_prog_test_run_opts(bpf_program__fd(skel->progs.test_pkt_access), &topts);
> > + ASSERT_OK(err, "bpf_prog_test_run_opts");
> > + ASSERT_OK(topts.retval, "retval");
> > + ASSERT_EQ(skel->bss->data_output, array_total, "array output");
> > +
> > + skel->bss->data_output = 0;
> > + skel->bss->use_array = 0;
> > + err = bpf_prog_test_run_opts(bpf_program__fd(skel->progs.test_pkt_access), &topts);
> > + ASSERT_OK(err, "bpf_prog_test_run_opts");
> > + ASSERT_OK(topts.retval, "retval");
> > + ASSERT_EQ(skel->bss->data_output, hash_total, "hash output");
> > +
> > +out:
> > + for_each_multi_maps__destroy(skel);
> > +}
> > +
> > void test_for_each(void)
> > {
> > if (test__start_subtest("hash_map"))
> > @@ -151,4 +211,6 @@ void test_for_each(void)
> > test_array_map();
> > if (test__start_subtest("write_map_key"))
> > test_write_map_key();
> > + if (test__start_subtest("multi_maps"))
> > + test_multi_maps();
> > }
> > diff --git a/tools/testing/selftests/bpf/progs/for_each_multi_maps.c b/tools/testing/selftests/bpf/progs/for_each_multi_maps.c
> > new file mode 100644
> > index 000000000000..ff0bed7d4459
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/for_each_multi_maps.c
> > @@ -0,0 +1,49 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include "vmlinux.h"
> > +#include <bpf/bpf_helpers.h>
> > +
> > +char _license[] SEC("license") = "GPL";
> > +
> > +struct {
> > + __uint(type, BPF_MAP_TYPE_ARRAY);
> > + __uint(max_entries, 3);
> > + __type(key, __u32);
> > + __type(value, __u64);
> > +} arraymap SEC(".maps");
> > +
> > +struct {
> > + __uint(type, BPF_MAP_TYPE_HASH);
> > + __uint(max_entries, 5);
> > + __type(key, __u32);
> > + __type(value, __u64);
> > +} hashmap SEC(".maps");
> > +
> > +struct callback_ctx {
> > + int output;
> > +};
> > +
> > +u32 data_output = 0;
> > +int use_array = 0;
> > +
> > +static __u64
> > +check_map_elem(struct bpf_map *map, __u32 *key, __u64 *val,
> > + struct callback_ctx *data)
> > +{
> > + data->output += *val;
> > + return 0;
> > +}
> > +
> > +SEC("tc")
> > +int test_pkt_access(struct __sk_buff *skb)
> > +{
> > + struct callback_ctx data;
> > +
> > + data.output = 0;
> > + if (use_array)
> > + bpf_for_each_map_elem(&arraymap, check_map_elem, &data, 0);
> > + else
> > + bpf_for_each_map_elem(&hashmap, check_map_elem, &data, 0);
>
> you are relying on the compiler to combine bpf_for_each_map_elem calls
> to one call instruction and just passing different map pointers,
> right? It would be best to code this in BPF assembly so that this
> doesn't rely on compiler decisions and optimizations.
It will look sad in asm. C is fine.
Maybe a follow up with
void *map;
if (..)
map = &array;
else
map = &hash;
bpf_for_each_map_elem(map, ...);
but optional. imo.
