On Fri, Apr 5, 2024 at 10:41 AM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> 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;
>
yep, this is simpler, no need to go for assembly
Though not sure why assembly would be sad, we do call subprograms by
their name (it's just symbols after all), so passing a pointer to
subprog should work just as well.
> bpf_for_each_map_elem(map, ...);
>
> but optional. imo.
