Hi,
On 7/6/2023 7:54 PM, Anton Protopopov wrote:
> 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.
Great.
SNIP
> +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;
No. I think changing it to "if (max_count > total) ret = -ENOENT;" will
be more appropriate, because it means the requested count couldn't been
fulfilled and it is also consistent with the comments in
include/uapi/linux/bpf.h
>
> 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
I think these use cases will be fine. Because when the last element has
been successfully iterated and returned, the out_batch is also updated,
so if the batch op is called again, -ENOENT will be returned.
>
> Can we update comments in include/uapi/linux/bpf.h?
I think the comments are correct.
>
>
> .
