Re: [PATCH v2 4/4] selftests/bpf: ringbuf, mmap: bump up page size to 64K

[Yauheni Kaliuta <yauheni.kaliuta@redhat.com>]

On Wed, Mar 31, 2021 at 9:26 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Mar 30, 2021 at 11:11 PM Yauheni Kaliuta
> <yauheni.kaliuta@redhat.com> wrote:
> >
> > Hi, Andrii,
> >
> > On Wed, Mar 31, 2021 at 8:49 AM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Mon, Mar 29, 2021 at 8:20 AM Yauheni Kaliuta
> > > <yauheni.kaliuta@redhat.com> wrote:
> > > >
> > > > On Sun, Mar 28, 2021 at 8:03 AM Andrii Nakryiko
> > > > <andrii.nakryiko@gmail.com> wrote:
> > > >
> > > > [...]
> > > >
> > > > > >
> > > > > >  struct {
> > > > > >         __uint(type, BPF_MAP_TYPE_ARRAY);
> > > > > > -       __uint(max_entries, 4096);
> > > > > > +       __uint(max_entries, PAGE_SIZE);
> > > > >
> > > > >
> > > > > so you can set map size at runtime before bpf_object__load (or
> > > > > skeleton's load) with bpf_map__set_max_entries. That way you don't
> > > > > have to do any assumptions. Just omit max_entries in BPF source code,
> > > > > and always set it in userspace.
> > > >
> > > > Will it work for ringbuf_multi? If I just set max_entries for ringbuf1
> > > > and ringbuf2 that way, it gives me
> > > >
> > > > libbpf: map 'ringbuf_arr': failed to create inner map: -22
> > > > libbpf: map 'ringbuf_arr': failed to create: Invalid argument(-22)
> > > > libbpf: failed to load object 'test_ringbuf_multi'
> > > > libbpf: failed to load BPF skeleton 'test_ringbuf_multi': -22
> > > > test_ringbuf_multi:FAIL:skel_load skeleton load failed
> > > >
> > >
> > > You are right, it won't work. We'd need to add something like
> > > bpf_map__inner_map() accessor to allow to adjust the inner map
> > > definition:
> > >
> > > bpf_map__set_max_entries(bpf_map__inner_map(skel->maps.ringbuf_arr), page_size);
> >
> > Thanks!
> >
> > On top on that, for some reason simple ringbuf_multi (converted to use
> > dynamic size) does not work on my 64K page configuration too, haven't
> > investigated why. Works on x86 4K page.
> >
> > >
> > > And some more fixes. Here's minimal diff that made it work, but
> > > probably needs a bit more testing:
> >
> > Thanks again.
> > I could send the patchset with mmap only converted and just increase
> > ringbuf size since it's not selftests only change, but requires libbpf
> > improvements.
> >
> > Or you would prefer to change them all together?
>
> Try to figure out why the 64K case doesn't work.

It was not related to the tests actually.

> And then just post
> all the patches together, it doesn't have to be strictly only
> selftests. bpf_map__inner_map() and bpf_map__set_inner_map_fd()
> resetting inner map are both useful for some cases that need more
> manual control.

I took you libbpf patch as is, will post current version.

>
> >
> >
> >
> > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > index 7aad78dbb4b4..ed5586cce227 100644
> > > --- a/tools/lib/bpf/libbpf.c
> > > +++ b/tools/lib/bpf/libbpf.c
> > > @@ -2194,6 +2194,7 @@ static int parse_btf_map_def(struct bpf_object *obj,
> > >              map->inner_map = calloc(1, sizeof(*map->inner_map));
> > >              if (!map->inner_map)
> > >                  return -ENOMEM;
> > > +            map->inner_map->fd = -1;
> > >              map->inner_map->sec_idx = obj->efile.btf_maps_shndx;
> > >              map->inner_map->name = malloc(strlen(map->name) +
> > >                                sizeof(".inner") + 1);
> > > @@ -3845,6 +3846,14 @@ __u32 bpf_map__max_entries(const struct bpf_map *map)
> > >      return map->def.max_entries;
> > >  }
> > >
> > > +struct bpf_map *bpf_map__inner_map(struct bpf_map *map)
> > > +{
> > > +    if (!bpf_map_type__is_map_in_map(map->def.type))
> > > +        return NULL;
> > > +
> > > +    return map->inner_map;
> > > +}
> > > +
> > >  int bpf_map__set_max_entries(struct bpf_map *map, __u32 max_entries)
> > >  {
> > >      if (map->fd >= 0)
> > > @@ -9476,6 +9485,7 @@ int bpf_map__set_inner_map_fd(struct bpf_map *map, int fd)
> > >          pr_warn("error: inner_map_fd already specified\n");
> > >          return -EINVAL;
> > >      }
> > > +    zfree(&map->inner_map);
> > >      map->inner_map_fd = fd;
> > >      return 0;
> > >  }
> > > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > > index f500621d28e5..bec4e6a6e31d 100644
> > > --- a/tools/lib/bpf/libbpf.h
> > > +++ b/tools/lib/bpf/libbpf.h
> > > @@ -480,6 +480,7 @@ LIBBPF_API int bpf_map__pin(struct bpf_map *map,
> > > const char *path);
> > >  LIBBPF_API int bpf_map__unpin(struct bpf_map *map, const char *path);
> > >
> > >  LIBBPF_API int bpf_map__set_inner_map_fd(struct bpf_map *map, int fd);
> > > +LIBBPF_API struct bpf_map *bpf_map__inner_map(struct bpf_map *map);
> > >
> > >  LIBBPF_API long libbpf_get_error(const void *ptr);
> > >
> > > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > > index f5990f7208ce..eeb6d5ebd1cc 100644
> > > --- a/tools/lib/bpf/libbpf.map
> > > +++ b/tools/lib/bpf/libbpf.map
> > > @@ -360,4 +360,5 @@ LIBBPF_0.4.0 {
> > >          bpf_linker__free;
> > >          bpf_linker__new;
> > >          bpf_object__set_kversion;
> > > +        bpf_map__inner_map;
> > >  } LIBBPF_0.3.0;
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c
> > > b/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c
> > > index d37161e59bb2..cdc9c9b1d0e1 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c
> > > @@ -41,13 +41,23 @@ static int process_sample(void *ctx, void *data, size_t len)
> > >  void test_ringbuf_multi(void)
> > >  {
> > >      struct test_ringbuf_multi *skel;
> > > -    struct ring_buffer *ringbuf;
> > > +    struct ring_buffer *ringbuf = NULL;
> > >      int err;
> > >
> > > -    skel = test_ringbuf_multi__open_and_load();
> > > +    skel = test_ringbuf_multi__open();
> > >      if (CHECK(!skel, "skel_open_load", "skeleton open&load failed\n"))
> > >          return;
> > >
> > > +    bpf_map__set_max_entries(skel->maps.ringbuf1, 4096);
> > > +    bpf_map__set_max_entries(skel->maps.ringbuf2, 4096);
> > > +    bpf_map__set_max_entries(bpf_map__inner_map(skel->maps.ringbuf_arr), 4096);
> > > +
> > > +    err = test_ringbuf_multi__load(skel);
> > > +    if (!ASSERT_OK(err, "skel_load"))
> > > +        goto cleanup;
> > > +
> > >      /* only trigger BPF program for current process */
> > >      skel->bss->pid = getpid();
> > >
> > > diff --git a/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c
> > > b/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c
> > > index edf3b6953533..055c10b2ff80 100644
> > > --- a/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c
> > > +++ b/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c
> > > @@ -15,7 +15,6 @@ struct sample {
> > >
> > >  struct ringbuf_map {
> > >      __uint(type, BPF_MAP_TYPE_RINGBUF);
> > > -    __uint(max_entries, 1 << 12);
> > >  } ringbuf1 SEC(".maps"),
> > >    ringbuf2 SEC(".maps");
> > >
> >
> >
> > --
> > WBR, Yauheni
> >
>


-- 
WBR, Yauheni