Re: [PATCH bpf v2 1/2] bpftool: Fix CPU IDs in per-CPU map output

[bot+bpf-ci]

> bpftool: Fix CPU IDs in per-CPU map output
>
> bpftool uses dense per-CPU value-buffer slots when printing per-CPU map
> values. It also uses the slot index as the CPU ID, which produces
> incorrect labels when the possible CPU mask is sparse, such as 0,2-3.
>
> Parse the possible CPU mask and use the corresponding logical CPU ID in
> plain, JSON, and BTF-formatted output. Keep the dense slot index for
> accessing the per-CPU value buffer, and propagate CPU-ID lookup and map
> output errors through the shared output path to its callers.
>
> Tested:
> - Built tools/bpf/bpftool successfully on the host.
> - Booted an arm64 QEMU guest with a patched virt device tree reporting
>   possible=0,2-3, present=0,2-3, and online=0,2-3.
> - Compared pre-fix and fixed plain and JSON map output in that guest:
>   labels changed from CPU 0,1,2 to CPU 0,2,3.
> - Loaded a BTF-described per-CPU array and dumped it with the fixed
>   bpftool in plain BTF and JSON formats; both formats reported CPU 0,2,3
>   while preserving the three dense per-CPU values.

This isn't a bug, but could the Tested: block be trimmed to the reproducer
(possible=0,2-3) or moved below the --- line, keeping the changelog focused
on the problem and the why? The first paragraph already mentions the sparse
mask, and the build-succeeded and format-by-format walkthrough lines restate
what the diff does.

> diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> index ef366ccc96501..ad10bfe8a0fc3 100644
> --- a/tools/bpf/bpftool/common.c
> +++ b/tools/bpf/bpftool/common.c

[ ... ]

> @@ -655,6 +656,45 @@ unsigned int get_possible_cpus(void)
>  	return cpus;
>  }
>
> +int get_possible_cpu_ids(int **cpu_ids)
> +{
> +	const char *possible_cpus_file = "/sys/devices/system/cpu/possible";
> +	bool *mask = NULL;
> +	int mask_sz, nr_cpus = 0;
> +	int *ids = NULL;
> +	int i, res;
> +
> +	*cpu_ids = NULL;
> +
> +	res = parse_cpu_mask_file(possible_cpus_file, &mask, &mask_sz);
> +	if (res) {
> +		p_err("failed to parse possible CPU mask: %s", strerror(-res));
> +		return res;
> +	}
> +
> +	for (i = 0; i < mask_sz; i++)
> +		nr_cpus += mask[i];
> +
> +	ids = calloc(nr_cpus, sizeof(*ids));
> +	if (!ids) {
> +		p_err("failed to allocate possible CPU IDs: %s", strerror(ENOMEM));
                                                                     ^^^^^^^^^^^^^^^^

This isn't a bug, but would p_err("mem alloc failed") (or strerror(errno))
fit the surrounding style better here, since strerror(ENOMEM) always prints
the same fixed string? This is the only strerror(ENOMEM) in common.c; every
other strerror() call passes errno, and the file's idiom for a failed
allocation is the terse p_err("mem alloc failed").

> +		res = -ENOMEM;
> +		goto out;
> +	}
> +
> +	for (i = 0, nr_cpus = 0; i < mask_sz; i++) {
> +		if (mask[i])
> +			ids[nr_cpus++] = i;
> +	}
> +	*cpu_ids = ids;
> +	ids = NULL;
> +	res = nr_cpus;
> +out:
> +	free(ids);
> +	free(mask);
> +	return res;
> +}
> +

[ ... ]

> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index 684a8fb724146..586c9e076a483 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c

[ ... ]

> @@ -833,6 +834,7 @@ map_dump(int fd, struct bpf_map_info *info, json_writer_t *wtr,
>  	void *key, *value, *prev_key;
>  	unsigned int num_elems = 0;
>  	struct btf *btf = NULL;
> +	int *cpu_ids = NULL;
>  	int err;
>
>  	key = malloc(info->key_size);
> @@ -845,6 +847,16 @@ map_dump(int fd, struct bpf_map_info *info, json_writer_t *wtr,
>
>  	prev_key = NULL;
>
> +	if (map_is_per_cpu(info->type)) {
> +		int cpu_cnt;
> +
> +		cpu_cnt = get_possible_cpu_ids(&cpu_ids);
> +		if (cpu_cnt < 0) {
> +			err = cpu_cnt;
> +			goto exit_free;
> +		}
> +	}
> +

This isn't a bug, but since both callers only look at the sign of
get_possible_cpu_ids(), would it read better to either pass cpu_cnt down
next to cpu_ids as the loop bound, or have the helper return plain 0/-errno
so there is one source of truth for the array length? Right now the
consumers derive the loop bound independently from get_possible_cpus().


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/35336124623