Re: [PATCH v4 bpf-next 6/6] selftests/bpf: check that ->elem_count is non-zero for the hash map

[Anton Protopopov <aspsk@isovalent.com>]

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 ?