| Message ID | 20210326122438.211242-4-yauheni.kaliuta@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [v2,1/4] selftests/bpf: test_progs/sockopt_sk: Convert to use BPF skeleton | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Fri, Mar 26, 2021 at 5:24 AM Yauheni Kaliuta <yauheni.kaliuta@redhat.com> wrote: > > Both ringbuf and mmap need PAGE_SIZE, but it's not available during > bpf program compile time. 4K size was hardcoded (page shift 12 bits) > which makes the tests fail on systems, configured for larger pages. > > Bump it up to 64K which at the first glance look reasonable at the > moment for most of the systems. > > Use define to make it clear. > > Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com> > --- > tools/testing/selftests/bpf/prog_tests/ringbuf.c | 9 +++++++-- > tools/testing/selftests/bpf/progs/map_ptr_kern.c | 9 +++++++-- > tools/testing/selftests/bpf/progs/test_mmap.c | 10 ++++++++-- > tools/testing/selftests/bpf/progs/test_ringbuf.c | 8 +++++++- > tools/testing/selftests/bpf/progs/test_ringbuf_multi.c | 7 ++++++- > 5 files changed, 35 insertions(+), 8 deletions(-) > > diff --git a/tools/testing/selftests/bpf/prog_tests/ringbuf.c b/tools/testing/selftests/bpf/prog_tests/ringbuf.c > index fddbc5db5d6a..9057654da957 100644 > --- a/tools/testing/selftests/bpf/prog_tests/ringbuf.c > +++ b/tools/testing/selftests/bpf/prog_tests/ringbuf.c > @@ -15,6 +15,11 @@ > #include "test_ringbuf.skel.h" > > #define EDONE 7777 > +#ifdef PAGE_SIZE > +#undef PAGE_SIZE > +#endif > +/* this is not actual page size, but the value used for ringbuf */ > +#define PAGE_SIZE 65536 > > static int duration = 0; > > @@ -110,9 +115,9 @@ void test_ringbuf(void) > CHECK(skel->bss->avail_data != 3 * rec_sz, > "err_avail_size", "exp %ld, got %ld\n", > 3L * rec_sz, skel->bss->avail_data); > - CHECK(skel->bss->ring_size != 4096, > + CHECK(skel->bss->ring_size != PAGE_SIZE, > "err_ring_size", "exp %ld, got %ld\n", > - 4096L, skel->bss->ring_size); > + (long)PAGE_SIZE, skel->bss->ring_size); > CHECK(skel->bss->cons_pos != 0, > "err_cons_pos", "exp %ld, got %ld\n", > 0L, skel->bss->cons_pos); > diff --git a/tools/testing/selftests/bpf/progs/map_ptr_kern.c b/tools/testing/selftests/bpf/progs/map_ptr_kern.c > index d8850bc6a9f1..c1460f27af78 100644 > --- a/tools/testing/selftests/bpf/progs/map_ptr_kern.c > +++ b/tools/testing/selftests/bpf/progs/map_ptr_kern.c > @@ -8,6 +8,11 @@ > #define MAX_ENTRIES 8 > #define HALF_ENTRIES (MAX_ENTRIES >> 1) > > +#ifndef PAGE_SIZE > +/* use reasonable value for various configurations */ > +#define PAGE_SIZE 65536 > +#endif > + > _Static_assert(MAX_ENTRIES < LOOP_BOUND, "MAX_ENTRIES must be < LOOP_BOUND"); > > enum bpf_map_type g_map_type = BPF_MAP_TYPE_UNSPEC; > @@ -635,7 +640,7 @@ struct bpf_ringbuf_map { > > struct { > __uint(type, BPF_MAP_TYPE_RINGBUF); > - __uint(max_entries, 1 << 12); > + __uint(max_entries, PAGE_SIZE); > } m_ringbuf SEC(".maps"); > > static inline int check_ringbuf(void) > @@ -643,7 +648,7 @@ static inline int check_ringbuf(void) > struct bpf_ringbuf_map *ringbuf = (struct bpf_ringbuf_map *)&m_ringbuf; > struct bpf_map *map = (struct bpf_map *)&m_ringbuf; > > - VERIFY(check(&ringbuf->map, map, 0, 0, 1 << 12)); > + VERIFY(check(&ringbuf->map, map, 0, 0, PAGE_SIZE)); > > return 1; > } > diff --git a/tools/testing/selftests/bpf/progs/test_mmap.c b/tools/testing/selftests/bpf/progs/test_mmap.c > index 4eb42cff5fe9..c22fcfea0767 100644 > --- a/tools/testing/selftests/bpf/progs/test_mmap.c > +++ b/tools/testing/selftests/bpf/progs/test_mmap.c > @@ -5,11 +5,16 @@ > #include <stdint.h> > #include <bpf/bpf_helpers.h> > > +#ifndef PAGE_SIZE > +/* use reasonable value for various configurations */ > +#define PAGE_SIZE 65536 > +#endif > + > char _license[] SEC("license") = "GPL"; > > 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. > __uint(map_flags, BPF_F_MMAPABLE | BPF_F_RDONLY_PROG); > __type(key, __u32); > __type(value, char); > @@ -17,7 +22,8 @@ struct { > > struct { > __uint(type, BPF_MAP_TYPE_ARRAY); > - __uint(max_entries, 512 * 4); /* at least 4 pages of data */ > + /* at least 4 pages of data */ > + __uint(max_entries, 4 * (PAGE_SIZE / sizeof (__u64))); > __uint(map_flags, BPF_F_MMAPABLE); > __type(key, __u32); > __type(value, __u64); > diff --git a/tools/testing/selftests/bpf/progs/test_ringbuf.c b/tools/testing/selftests/bpf/progs/test_ringbuf.c > index 8ba9959b036b..6e645babdc18 100644 > --- a/tools/testing/selftests/bpf/progs/test_ringbuf.c > +++ b/tools/testing/selftests/bpf/progs/test_ringbuf.c > @@ -4,6 +4,12 @@ > #include <linux/bpf.h> > #include <bpf/bpf_helpers.h> > > +#ifndef PAGE_SIZE > +/* use reasonable value for various configurations */ > +#define PAGE_SIZE 65536 > +#endif > + > + > char _license[] SEC("license") = "GPL"; > > struct sample { > @@ -15,7 +21,7 @@ struct sample { > > struct { > __uint(type, BPF_MAP_TYPE_RINGBUF); > - __uint(max_entries, 1 << 12); > + __uint(max_entries, PAGE_SIZE); > } ringbuf SEC(".maps"); > > /* inputs */ > diff --git a/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c b/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c > index edf3b6953533..13bcf095e06c 100644 > --- a/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c > +++ b/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c > @@ -4,6 +4,11 @@ > #include <linux/bpf.h> > #include <bpf/bpf_helpers.h> > > +#ifndef PAGE_SIZE > +/* use reasonable value for various configurations */ > +#define PAGE_SIZE 65536 > +#endif > + > char _license[] SEC("license") = "GPL"; > > struct sample { > @@ -15,7 +20,7 @@ struct sample { > > struct ringbuf_map { > __uint(type, BPF_MAP_TYPE_RINGBUF); > - __uint(max_entries, 1 << 12); > + __uint(max_entries, PAGE_SIZE); > } ringbuf1 SEC(".maps"), > ringbuf2 SEC(".maps"); > > -- > 2.29.2 >
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
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); And some more fixes. Here's minimal diff that made it work, but probably needs a bit more testing: 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");
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? > 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"); >
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. 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. > > > > > 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 >
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 > > >
diff --git a/tools/testing/selftests/bpf/prog_tests/ringbuf.c b/tools/testing/selftests/bpf/prog_tests/ringbuf.c index fddbc5db5d6a..9057654da957 100644 --- a/tools/testing/selftests/bpf/prog_tests/ringbuf.c +++ b/tools/testing/selftests/bpf/prog_tests/ringbuf.c @@ -15,6 +15,11 @@ #include "test_ringbuf.skel.h" #define EDONE 7777 +#ifdef PAGE_SIZE +#undef PAGE_SIZE +#endif +/* this is not actual page size, but the value used for ringbuf */ +#define PAGE_SIZE 65536 static int duration = 0; @@ -110,9 +115,9 @@ void test_ringbuf(void) CHECK(skel->bss->avail_data != 3 * rec_sz, "err_avail_size", "exp %ld, got %ld\n", 3L * rec_sz, skel->bss->avail_data); - CHECK(skel->bss->ring_size != 4096, + CHECK(skel->bss->ring_size != PAGE_SIZE, "err_ring_size", "exp %ld, got %ld\n", - 4096L, skel->bss->ring_size); + (long)PAGE_SIZE, skel->bss->ring_size); CHECK(skel->bss->cons_pos != 0, "err_cons_pos", "exp %ld, got %ld\n", 0L, skel->bss->cons_pos); diff --git a/tools/testing/selftests/bpf/progs/map_ptr_kern.c b/tools/testing/selftests/bpf/progs/map_ptr_kern.c index d8850bc6a9f1..c1460f27af78 100644 --- a/tools/testing/selftests/bpf/progs/map_ptr_kern.c +++ b/tools/testing/selftests/bpf/progs/map_ptr_kern.c @@ -8,6 +8,11 @@ #define MAX_ENTRIES 8 #define HALF_ENTRIES (MAX_ENTRIES >> 1) +#ifndef PAGE_SIZE +/* use reasonable value for various configurations */ +#define PAGE_SIZE 65536 +#endif + _Static_assert(MAX_ENTRIES < LOOP_BOUND, "MAX_ENTRIES must be < LOOP_BOUND"); enum bpf_map_type g_map_type = BPF_MAP_TYPE_UNSPEC; @@ -635,7 +640,7 @@ struct bpf_ringbuf_map { struct { __uint(type, BPF_MAP_TYPE_RINGBUF); - __uint(max_entries, 1 << 12); + __uint(max_entries, PAGE_SIZE); } m_ringbuf SEC(".maps"); static inline int check_ringbuf(void) @@ -643,7 +648,7 @@ static inline int check_ringbuf(void) struct bpf_ringbuf_map *ringbuf = (struct bpf_ringbuf_map *)&m_ringbuf; struct bpf_map *map = (struct bpf_map *)&m_ringbuf; - VERIFY(check(&ringbuf->map, map, 0, 0, 1 << 12)); + VERIFY(check(&ringbuf->map, map, 0, 0, PAGE_SIZE)); return 1; } diff --git a/tools/testing/selftests/bpf/progs/test_mmap.c b/tools/testing/selftests/bpf/progs/test_mmap.c index 4eb42cff5fe9..c22fcfea0767 100644 --- a/tools/testing/selftests/bpf/progs/test_mmap.c +++ b/tools/testing/selftests/bpf/progs/test_mmap.c @@ -5,11 +5,16 @@ #include <stdint.h> #include <bpf/bpf_helpers.h> +#ifndef PAGE_SIZE +/* use reasonable value for various configurations */ +#define PAGE_SIZE 65536 +#endif + char _license[] SEC("license") = "GPL"; struct { __uint(type, BPF_MAP_TYPE_ARRAY); - __uint(max_entries, 4096); + __uint(max_entries, PAGE_SIZE); __uint(map_flags, BPF_F_MMAPABLE | BPF_F_RDONLY_PROG); __type(key, __u32); __type(value, char); @@ -17,7 +22,8 @@ struct { struct { __uint(type, BPF_MAP_TYPE_ARRAY); - __uint(max_entries, 512 * 4); /* at least 4 pages of data */ + /* at least 4 pages of data */ + __uint(max_entries, 4 * (PAGE_SIZE / sizeof (__u64))); __uint(map_flags, BPF_F_MMAPABLE); __type(key, __u32); __type(value, __u64); diff --git a/tools/testing/selftests/bpf/progs/test_ringbuf.c b/tools/testing/selftests/bpf/progs/test_ringbuf.c index 8ba9959b036b..6e645babdc18 100644 --- a/tools/testing/selftests/bpf/progs/test_ringbuf.c +++ b/tools/testing/selftests/bpf/progs/test_ringbuf.c @@ -4,6 +4,12 @@ #include <linux/bpf.h> #include <bpf/bpf_helpers.h> +#ifndef PAGE_SIZE +/* use reasonable value for various configurations */ +#define PAGE_SIZE 65536 +#endif + + char _license[] SEC("license") = "GPL"; struct sample { @@ -15,7 +21,7 @@ struct sample { struct { __uint(type, BPF_MAP_TYPE_RINGBUF); - __uint(max_entries, 1 << 12); + __uint(max_entries, PAGE_SIZE); } ringbuf SEC(".maps"); /* inputs */ diff --git a/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c b/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c index edf3b6953533..13bcf095e06c 100644 --- a/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c +++ b/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c @@ -4,6 +4,11 @@ #include <linux/bpf.h> #include <bpf/bpf_helpers.h> +#ifndef PAGE_SIZE +/* use reasonable value for various configurations */ +#define PAGE_SIZE 65536 +#endif + char _license[] SEC("license") = "GPL"; struct sample { @@ -15,7 +20,7 @@ struct sample { struct ringbuf_map { __uint(type, BPF_MAP_TYPE_RINGBUF); - __uint(max_entries, 1 << 12); + __uint(max_entries, PAGE_SIZE); } ringbuf1 SEC(".maps"), ringbuf2 SEC(".maps");
Both ringbuf and mmap need PAGE_SIZE, but it's not available during bpf program compile time. 4K size was hardcoded (page shift 12 bits) which makes the tests fail on systems, configured for larger pages. Bump it up to 64K which at the first glance look reasonable at the moment for most of the systems. Use define to make it clear. Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com> --- tools/testing/selftests/bpf/prog_tests/ringbuf.c | 9 +++++++-- tools/testing/selftests/bpf/progs/map_ptr_kern.c | 9 +++++++-- tools/testing/selftests/bpf/progs/test_mmap.c | 10 ++++++++-- tools/testing/selftests/bpf/progs/test_ringbuf.c | 8 +++++++- tools/testing/selftests/bpf/progs/test_ringbuf_multi.c | 7 ++++++- 5 files changed, 35 insertions(+), 8 deletions(-)