| Message ID | 20230705160139.19967-7-aspsk@isovalent.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: add percpu stats for bpf_map | expand |
On Wed, Jul 5, 2023 at 9:00 AM Anton Protopopov <aspsk@isovalent.com> wrote: > > Previous commits populated the ->elem_count per-cpu pointer for hash maps. > Check that this pointer is non-NULL in an existing map. > > Signed-off-by: Anton Protopopov <aspsk@isovalent.com> > --- > tools/testing/selftests/bpf/progs/map_ptr_kern.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/tools/testing/selftests/bpf/progs/map_ptr_kern.c b/tools/testing/selftests/bpf/progs/map_ptr_kern.c > index db388f593d0a..d6e234a37ccb 100644 > --- a/tools/testing/selftests/bpf/progs/map_ptr_kern.c > +++ b/tools/testing/selftests/bpf/progs/map_ptr_kern.c > @@ -33,6 +33,7 @@ struct bpf_map { > __u32 value_size; > __u32 max_entries; > __u32 id; > + __s64 *elem_count; > } __attribute__((preserve_access_index)); > > static inline int check_bpf_map_fields(struct bpf_map *map, __u32 key_size, > @@ -111,6 +112,8 @@ static inline int check_hash(void) > > VERIFY(check_default_noinline(&hash->map, map)); > > + VERIFY(map->elem_count != NULL); > + imo that's worse than no test. Just use kfunc here and get the real count?
On Wed, Jul 05, 2023 at 06:26:25PM -0700, Alexei Starovoitov wrote: > On Wed, Jul 5, 2023 at 9:00 AM Anton Protopopov <aspsk@isovalent.com> wrote: > > > > Previous commits populated the ->elem_count per-cpu pointer for hash maps. > > Check that this pointer is non-NULL in an existing map. > > > > Signed-off-by: Anton Protopopov <aspsk@isovalent.com> > > --- > > tools/testing/selftests/bpf/progs/map_ptr_kern.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/tools/testing/selftests/bpf/progs/map_ptr_kern.c b/tools/testing/selftests/bpf/progs/map_ptr_kern.c > > index db388f593d0a..d6e234a37ccb 100644 > > --- a/tools/testing/selftests/bpf/progs/map_ptr_kern.c > > +++ b/tools/testing/selftests/bpf/progs/map_ptr_kern.c > > @@ -33,6 +33,7 @@ struct bpf_map { > > __u32 value_size; > > __u32 max_entries; > > __u32 id; > > + __s64 *elem_count; > > } __attribute__((preserve_access_index)); > > > > static inline int check_bpf_map_fields(struct bpf_map *map, __u32 key_size, > > @@ -111,6 +112,8 @@ static inline int check_hash(void) > > > > VERIFY(check_default_noinline(&hash->map, map)); > > > > + VERIFY(map->elem_count != NULL); > > + > > imo that's worse than no test. > Just use kfunc here and get the real count? Then, as I mentioned in the previous version, I will have to teach kfuncs to recognize const_ptr_to_map args just for the sake of this selftest, while we already testing all functionality in the new selftest for test_maps. So I would just omit this one. Or am I missing something?
On Wed, Jul 5, 2023 at 10:42 PM Anton Protopopov <aspsk@isovalent.com> wrote: > > On Wed, Jul 05, 2023 at 06:26:25PM -0700, Alexei Starovoitov wrote: > > On Wed, Jul 5, 2023 at 9:00 AM Anton Protopopov <aspsk@isovalent.com> wrote: > > > > > > Previous commits populated the ->elem_count per-cpu pointer for hash maps. > > > Check that this pointer is non-NULL in an existing map. > > > > > > Signed-off-by: Anton Protopopov <aspsk@isovalent.com> > > > --- > > > tools/testing/selftests/bpf/progs/map_ptr_kern.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/tools/testing/selftests/bpf/progs/map_ptr_kern.c b/tools/testing/selftests/bpf/progs/map_ptr_kern.c > > > index db388f593d0a..d6e234a37ccb 100644 > > > --- a/tools/testing/selftests/bpf/progs/map_ptr_kern.c > > > +++ b/tools/testing/selftests/bpf/progs/map_ptr_kern.c > > > @@ -33,6 +33,7 @@ struct bpf_map { > > > __u32 value_size; > > > __u32 max_entries; > > > __u32 id; > > > + __s64 *elem_count; > > > } __attribute__((preserve_access_index)); > > > > > > static inline int check_bpf_map_fields(struct bpf_map *map, __u32 key_size, > > > @@ -111,6 +112,8 @@ static inline int check_hash(void) > > > > > > VERIFY(check_default_noinline(&hash->map, map)); > > > > > > + VERIFY(map->elem_count != NULL); > > > + > > > > imo that's worse than no test. > > Just use kfunc here and get the real count? > > Then, as I mentioned in the previous version, I will have to teach kfuncs to > recognize const_ptr_to_map args just for the sake of this selftest, while we > already testing all functionality in the new selftest for test_maps. So I would > just omit this one. Or am I missing something? Don't you want to do: val = bpf_map_lookup_elem(map, ...); cnt = bpf_map_sum_elem_count(map); and that's the main use case ? So teaching the verifier to understand that const_ptr_to_map matches BTF 'struct bpf_map *' is essential ?
On Thu, Jul 06, 2023 at 10:03:30AM -0700, Alexei Starovoitov wrote: > On Wed, Jul 5, 2023 at 10:42 PM Anton Protopopov <aspsk@isovalent.com> wrote: > > > > On Wed, Jul 05, 2023 at 06:26:25PM -0700, Alexei Starovoitov wrote: > > > On Wed, Jul 5, 2023 at 9:00 AM Anton Protopopov <aspsk@isovalent.com> wrote: > > > > > > > > Previous commits populated the ->elem_count per-cpu pointer for hash maps. > > > > Check that this pointer is non-NULL in an existing map. > > > > > > > > Signed-off-by: Anton Protopopov <aspsk@isovalent.com> > > > > --- > > > > tools/testing/selftests/bpf/progs/map_ptr_kern.c | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/tools/testing/selftests/bpf/progs/map_ptr_kern.c b/tools/testing/selftests/bpf/progs/map_ptr_kern.c > > > > index db388f593d0a..d6e234a37ccb 100644 > > > > --- a/tools/testing/selftests/bpf/progs/map_ptr_kern.c > > > > +++ b/tools/testing/selftests/bpf/progs/map_ptr_kern.c > > > > @@ -33,6 +33,7 @@ struct bpf_map { > > > > __u32 value_size; > > > > __u32 max_entries; > > > > __u32 id; > > > > + __s64 *elem_count; > > > > } __attribute__((preserve_access_index)); > > > > > > > > static inline int check_bpf_map_fields(struct bpf_map *map, __u32 key_size, > > > > @@ -111,6 +112,8 @@ static inline int check_hash(void) > > > > > > > > VERIFY(check_default_noinline(&hash->map, map)); > > > > > > > > + VERIFY(map->elem_count != NULL); > > > > + > > > > > > imo that's worse than no test. > > > Just use kfunc here and get the real count? > > > > Then, as I mentioned in the previous version, I will have to teach kfuncs to > > recognize const_ptr_to_map args just for the sake of this selftest, while we > > already testing all functionality in the new selftest for test_maps. So I would > > just omit this one. Or am I missing something? > > > Don't you want to do: > val = bpf_map_lookup_elem(map, ...); > cnt = bpf_map_sum_elem_count(map); > > and that's the main use case ? Not sure I understand what this ^ use case is... Our primary use case is to [periodically] get the number of elements from the user space. We can do this using an iterator as you've suggested and what is tested in the added selftest. > So teaching the verifier to understand that const_ptr_to_map matches > BTF 'struct bpf_map *' is essential ?
On Thu, Jul 6, 2023 at 10:42 AM Anton Protopopov <aspsk@isovalent.com> wrote: > > > > > Don't you want to do: > > val = bpf_map_lookup_elem(map, ...); > > cnt = bpf_map_sum_elem_count(map); > > > > and that's the main use case ? > > Not sure I understand what this ^ use case is... > > Our primary use case is to [periodically] get the number of elements from the > user space. We can do this using an iterator as you've suggested and what is > tested in the added selftest. Hmm. During the last office hours John explained that he needs to raise alarm to user space when the map is close to full capacity. Currently he's doing it with his own per-map counters implemented in a bpf prog. I'd expect the alarm to be done inline with updates to the counters. If you scan maps from user space every second or so there is a chance the spike in usage will be missed. If we're adding map counters they should be usable not only via iterators. John, did I describe your use case correctly?
On Thu, Jul 06, 2023 at 10:48:16AM -0700, Alexei Starovoitov wrote: > On Thu, Jul 6, 2023 at 10:42 AM Anton Protopopov <aspsk@isovalent.com> wrote: > > > > > > > > Don't you want to do: > > > val = bpf_map_lookup_elem(map, ...); > > > cnt = bpf_map_sum_elem_count(map); > > > > > > and that's the main use case ? > > > > Not sure I understand what this ^ use case is... > > > > Our primary use case is to [periodically] get the number of elements from the > > user space. We can do this using an iterator as you've suggested and what is > > tested in the added selftest. > > Hmm. During the last office hours John explained that he needs to raise > alarm to user space when the map is close to full capacity. > Currently he's doing it with his own per-map counters implemented > in a bpf prog. > I'd expect the alarm to be done inline with updates to the counters. > If you scan maps from user space every second or so there is a chance > the spike in usage will be missed. > > If we're adding map counters they should be usable not only via iterators. In some use cases this is ok to miss a spike in favour of not checking counters too often. But yes, for other use cases this makes sense to add support for const map ptr, so I will do this. > > John, > did I describe your use case correctly?
On Thu, Jul 6, 2023 at 11:34 AM Anton Protopopov <aspsk@isovalent.com> wrote: > > On Thu, Jul 06, 2023 at 10:48:16AM -0700, Alexei Starovoitov wrote: > > On Thu, Jul 6, 2023 at 10:42 AM Anton Protopopov <aspsk@isovalent.com> wrote: > > > > > > > > > > > Don't you want to do: > > > > val = bpf_map_lookup_elem(map, ...); > > > > cnt = bpf_map_sum_elem_count(map); > > > > > > > > and that's the main use case ? > > > > > > Not sure I understand what this ^ use case is... > > > > > > Our primary use case is to [periodically] get the number of elements from the > > > user space. We can do this using an iterator as you've suggested and what is > > > tested in the added selftest. > > > > Hmm. During the last office hours John explained that he needs to raise > > alarm to user space when the map is close to full capacity. > > Currently he's doing it with his own per-map counters implemented > > in a bpf prog. > > I'd expect the alarm to be done inline with updates to the counters. > > If you scan maps from user space every second or so there is a chance > > the spike in usage will be missed. > > > > If we're adding map counters they should be usable not only via iterators. > > In some use cases this is ok to miss a spike in favour of not checking counters > too often. But yes, for other use cases this makes sense to add support for > const map ptr, so I will do this. Great. Please send a follow up. I've applied the current set.
diff --git a/tools/testing/selftests/bpf/progs/map_ptr_kern.c b/tools/testing/selftests/bpf/progs/map_ptr_kern.c index db388f593d0a..d6e234a37ccb 100644 --- a/tools/testing/selftests/bpf/progs/map_ptr_kern.c +++ b/tools/testing/selftests/bpf/progs/map_ptr_kern.c @@ -33,6 +33,7 @@ struct bpf_map { __u32 value_size; __u32 max_entries; __u32 id; + __s64 *elem_count; } __attribute__((preserve_access_index)); static inline int check_bpf_map_fields(struct bpf_map *map, __u32 key_size, @@ -111,6 +112,8 @@ static inline int check_hash(void) VERIFY(check_default_noinline(&hash->map, map)); + VERIFY(map->elem_count != NULL); + VERIFY(hash->n_buckets == MAX_ENTRIES); VERIFY(hash->elem_size == 64);
Previous commits populated the ->elem_count per-cpu pointer for hash maps. Check that this pointer is non-NULL in an existing map. Signed-off-by: Anton Protopopov <aspsk@isovalent.com> --- tools/testing/selftests/bpf/progs/map_ptr_kern.c | 3 +++ 1 file changed, 3 insertions(+)