On Thu, Jul 06, 2023 at 06:49:02PM +0800, Hou Tao wrote:
> Hi,
>
> On 7/6/2023 12:01 AM, Anton Protopopov wrote:
> > Add a new map test, map_percpu_stats.c, which is checking the correctness of
> > map's percpu elements counters. For supported maps the test upserts a number
> > of elements, checks the correctness of the counters, then deletes all the
> > elements and checks again that the counters sum drops down to zero.
> >
> > The following map types are tested:
> >
> > * BPF_MAP_TYPE_HASH, BPF_F_NO_PREALLOC
> > * BPF_MAP_TYPE_PERCPU_HASH, BPF_F_NO_PREALLOC
> > * BPF_MAP_TYPE_HASH,
> > * BPF_MAP_TYPE_PERCPU_HASH,
> > * BPF_MAP_TYPE_LRU_HASH
> > * BPF_MAP_TYPE_LRU_PERCPU_HASH
> > * BPF_MAP_TYPE_LRU_HASH, BPF_F_NO_COMMON_LRU
> > * BPF_MAP_TYPE_LRU_PERCPU_HASH, BPF_F_NO_COMMON_LRU
> > * BPF_MAP_TYPE_HASH_OF_MAPS
> >
> > Signed-off-by: Anton Protopopov <aspsk@xxxxxxxxxxxxx>
>
> Acked-by: Hou Tao <houtao1@xxxxxxxxxx>
>
> With two nits below.
Thanks, fixed both for v5.
> > +
> > +static const char *map_type_to_s(__u32 type)
> > +{
> > + switch (type) {
> > + case BPF_MAP_TYPE_HASH:
> > + return "HASH";
> > + case BPF_MAP_TYPE_PERCPU_HASH:
> > + return "PERCPU_HASH";
> > + case BPF_MAP_TYPE_LRU_HASH:
> > + return "LRU_HASH";
> > + case BPF_MAP_TYPE_LRU_PERCPU_HASH:
> > + return "LRU_PERCPU_HASH";
> > + default:
> > + return "<define-me>";
> > + }
> Missing BPF_MAP_TYPE_HASH_OF_MAPS ?
> > +}
> > +
> > +static __u32 map_count_elements(__u32 type, int map_fd)
> > +{
> > + __u32 key = -1;
> > + int n = 0;
> > +
> > + while (!bpf_map_get_next_key(map_fd, &key, &key))
> > + n++;
> > + return n;
> > +}
> > +
> > +#define BATCH true
> > +
> > +static void delete_and_lookup_batch(int map_fd, void *keys, __u32 count)
> > +{
> > + static __u8 values[(8 << 10) * MAX_ENTRIES];
> > + void *in_batch = NULL, *out_batch;
> > + __u32 save_count = count;
> > + int ret;
> > +
> > + ret = bpf_map_lookup_and_delete_batch(map_fd,
> > + &in_batch, &out_batch,
> > + keys, values, &count,
> > + NULL);
> > +
> > + /*
> > + * Despite what uapi header says, lookup_and_delete_batch will return
> > + * -ENOENT in case we successfully have deleted all elements, so check
> > + * this separately
> > + */
>
> It seems it is a bug in __htab_map_lookup_and_delete_batch(). I could
> post a patch to fix it if you don't plan to do that by yourself.
This should be as simple as
@@ -1876,7 +1876,8 @@ __htab_map_lookup_and_delete_batch(struct bpf_map *map,
total += bucket_cnt;
batch++;
if (batch >= htab->n_buckets) {
- ret = -ENOENT;
+ if (!total)
+ ret = -ENOENT;
goto after_loop;
}
goto again;
However, this might be already utilized by some apps to check that they've read
all entries. Two local examples are map_tests/map_in_map_batch_ops.c and
map_tests/htab_map_batch_ops.c. Another example I know is from BCC tools:
https://github.com/iovisor/bcc/blob/master/libbpf-tools/map_helpers.c#L58
Can we update comments in include/uapi/linux/bpf.h?
> > + CHECK(ret < 0 && (errno != ENOENT || !count), "bpf_map_lookup_and_delete_batch",
> > + "error: %s\n", strerror(errno));
> > +
> > + CHECK(count != save_count,
> > + "bpf_map_lookup_and_delete_batch",
> > + "deleted not all elements: removed=%u expected=%u\n",
> > + count, save_count);
> > +}
> > +
> SNIP
> > +static __u32 get_cur_elements(int map_id)
> > +{
> > + LIBBPF_OPTS(bpf_iter_attach_opts, opts);
> > + union bpf_iter_link_info linfo;
> > + struct map_percpu_stats *skel;
> > + struct bpf_link *link;
> > + __u32 n_elements;
> > + int iter_fd;
> > + int ret;
> > +
> > + opts.link_info = &linfo;
> > + opts.link_info_len = sizeof(linfo);
> > +
> > + skel = map_percpu_stats__open();
> > + CHECK(skel == NULL, "map_percpu_stats__open", "error: %s", strerror(errno));
> > +
> > + skel->bss->target_id = map_id;
> > +
> > + ret = map_percpu_stats__load(skel);
> > + CHECK(ret != 0, "map_percpu_stats__load", "error: %s", strerror(errno));
> > +
> > + link = bpf_program__attach_iter(skel->progs.dump_bpf_map, &opts);
>
> Instead of passing a uninitialized opts, I think using NULL will be fine
> here because there is no option for bpf map iterator now.
>
