Re: [PATCH bpf-next 1/2] bpftool: Use batch lookups for bounded hash map dumps

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Multi-line comments do not follow the mandatory BPF subsystem style.
--

commit a1cb333fccdbef3bcdfaf55fc0e0a282326e27a3
Author: Tianyi Chen <hi@tychen.cc>

bpftool: Use batch lookups for bounded hash map dumps

This patch updates bpftool to use BPF_MAP_LOOKUP_BATCH when dumping bounded
hash maps to reduce BPF syscalls. It restricts this optimization to maps
that fit within a 4 MiB memory budget and handles fallbacks.

> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index 684a8fb724146..1eccdc0d1e963 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
[ ... ]
> @@ -762,10 +757,108 @@ static int dump_map_elem(int fd, void *key, void *value,
[ ... ]
> +/* Return 1 to use individual lookups, but only before batch traversal starts. */
> +static int dump_map_batch(int fd, void *key, void *value,
> +			  struct bpf_map_info *info, struct btf *btf,
> +			  json_writer_t *wtr, unsigned int *num_elems)
> +{
> +	__u32 capacity, count, batch = 0, next_batch = 0, i;
> +	void *keys = NULL, *values = NULL, *buf;
> +	bool first = true, can_fallback = true;
> +	int err;
> +
> +	/* Hash lookup batches must accommodate a whole bucket. Restrict the
> +	 * optimization to maps whose worst-case bucket fits the memory budget,
> +	 * so a later ENOSPC never forces a restart after printing some entries.
> +	 * Division also bounds the allocation multiplications on 32-bit hosts.
> +	 */

[Severity: Low]
Should the opening /* be on its own line to follow the BPF subsystem
multi-line comment style?

> +	if (info->type != BPF_MAP_TYPE_HASH || !info->max_entries ||
> +	    (__u64)info->key_size + info->value_size >
> +	    MAP_DUMP_BATCH_MAX_BYTES / info->max_entries)
> +		return 1;
[ ... ]
> +	while (true) {
> +		count = capacity;
> +		err = bpf_map_lookup_batch(fd, first ? NULL : &batch,
> +					   &next_batch, keys, values, &count, NULL);
> +		err = err ? errno : 0;
> +		/* Older kernels reject the command before updating count. Do not
> +		 * inspect the buffers on these errors, or fall back after progress.
> +		 */

[Severity: Low]
Does this comment also need the opening /* on its own line?

> +		if (can_fallback && (err == EINVAL || err == EOPNOTSUPP ||
> +				     err == 524 /* ENOTSUPP */)) {
> +			err = 1;
> +			goto out;
> +		}
[ ... ]
> +		for (i = 0; i < count; i++) {
> +			/* Keep the alignment provided by individual lookups, including
> +			 * for BTF types whose map key/value size is not aligned.
> +			 */

[Severity: Low]
Here is a third instance. Should this opening /* be on its own line as well?

> +			memcpy(key, keys + (size_t)i * info->key_size, info->key_size);
> +			memcpy(value, values + (size_t)i * info->value_size, info->value_size);
> +			print_map_elem(key, value, info, btf, wtr);
> +			(*num_elems)++;
> +		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260906170819.1212661-1-hi@tychen.cc?part=1