Re: [PATCH v3 bpf-next 2/2] selftests/bpf: handle batch operations for map-in-map bpf-maps

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Mon, Apr 25, 2022 at 11:42 AM Takshak Chahande <ctakshak@fb.com> wrote:
>
> This patch adds up test cases that handles 4 combinations:
> a) outer map: BPF_MAP_TYPE_ARRAY_OF_MAPS
>    inner maps: BPF_MAP_TYPE_ARRAY and BPF_MAP_TYPE_HASH
> b) outer map: BPF_MAP_TYPE_HASH_OF_MAPS
>    inner maps: BPF_MAP_TYPE_ARRAY and BPF_MAP_TYPE_HASH
>
> v2->v3:
> - Handled transient ENOSPC correctly, bug was found in BPF CI (Daniel)
>
> v1->v2:
> - Fixed no format arguments error (Andrii)
>
> Signed-off-by: Takshak Chahande <ctakshak@fb.com>
> ---

Is there any extra benefit in putting these test under test_maps
instead of test_progs? test_progs has better "testing infra", it's
easier to isolate and debug tests, skip them or run just the ones you
want, better logging, better ASSERT_xxx() macros for testing, etc.

I see that you create a fixed amount of inner maps, etc. It's all
actually simpler to do in test_progs using BPF-side code. See other
examples under progs/ that show how to create and initialize
map-in-maps.


>  .../bpf/map_tests/map_in_map_batch_ops.c      | 239 ++++++++++++++++++
>  1 file changed, 239 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/map_tests/map_in_map_batch_ops.c
>

[...]

> +static int create_outer_map(enum bpf_map_type map_type, __u32 inner_map_fd)
> +{
> +       int outer_map_fd;
> +
> +       LIBBPF_OPTS(bpf_map_create_opts, attr);

LIBBPF_OPTS() is declaring a variable, it should go together with
other variables

> +       attr.inner_map_fd = inner_map_fd;
> +       outer_map_fd = bpf_map_create(map_type, "outer_map", sizeof(__u32),
> +                                     sizeof(__u32), OUTER_MAP_ENTRIES,
> +                                     &attr);
> +       CHECK(outer_map_fd < 0,
> +             "outer bpf_map_create()",
> +             "map_type=(%d), error:%s\n",
> +             map_type, strerror(errno));
> +
> +       return outer_map_fd;
> +}
> +

[...]

> +static void _map_in_map_batch_ops(enum bpf_map_type outer_map_type,
> +                                 enum bpf_map_type inner_map_type)
> +{
> +       __u32 *outer_map_keys, *inner_map_fds;
> +       __u32 max_entries = OUTER_MAP_ENTRIES;
> +       __u32 value_size = sizeof(__u32);
> +       int batch_size[2] = {5, 10};
> +       __u32 map_index, op_index;
> +       int outer_map_fd, ret;
> +       DECLARE_LIBBPF_OPTS(bpf_map_batch_opts, opts,

nit: prefere shorter LIBBPF_OPTS(). as for zero initialization of
elem_flags and flags, they are zero-initialized by default by
LIBBPF_OPTS, so you can just drop two lines below

> +                           .elem_flags = 0,
> +                           .flags = 0,
> +       );
> +
> +       outer_map_keys = calloc(max_entries, value_size);
> +       inner_map_fds = calloc(max_entries, value_size);
> +       create_inner_maps(inner_map_type, inner_map_fds);
> +

[...]