Re: [PATCH bpf-next v10 4/9] libbpf: Add support for global percpu data

[Leon Hwang <leon.hwang@linux.dev>]

On 16/7/26 00:00, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] BPF skeleton generation completely omits the `.percpu` map, breaking the promised ergonomic access.
> --
>
> --- Patch [4]: [PATCH bpf-next v10 4/9] libbpf: Add support for global percpu data ---
> (Note: The format of this review is altered to use a free-form summary rather than inline patch quoting due to automated recitation restrictions.)
>
> commit c26518c258a32645d11a26c0d0ce50242231d6a6
> Author: Leon Hwang <leon.hwang@linux.dev>
> libbpf: Add support for global percpu data
>
> This commit adds support for a new ".percpu" section in libbpf, enabling
> efficient handling of global percpu variables. It handles the section by
> creating a percpu_array map and populating it at load time.
>
> [Severity: Medium]
> The commit message mentions that users can read initialized percpu data
> after loading the program. Does the current implementation fully support
> BPF skeleton generation for this new section?
>
> In tools/lib/bpf/libbpf.c:map_is_mmapable(), the new code explicitly
> returns false when map->libbpf_type == LIBBPF_MAP_PERCPU.

map_is_mmapable() returns false for the internal percpu_array map to
avoid 'def->map_flags |= BPF_F_MMAPABLE'. But, it does not prevent
allocating mmaped memory for the internal percpu_array map.

In patch #6, the generated skeleton with this new section has been
tested well.

>
> Because PERCPU_ARRAY maps cannot be memory mapped, this prevents the
> skeleton generator from emitting the typed datasec pointer (such as
> skel->percpu).
>
> Additionally, does bpftool need a concurrent update to fully support this?
> Looking at get_map_ident() in tools/bpf/bpftool/gen.c, the sfxs[] array
> only contains ".data", ".rodata", ".bss", and ".kconfig".
>
> Without adding ".percpu" to the bpftool suffix list, it appears the skeleton
> generator will entirely omit the map from the generated header. If neither
> skel->percpu nor skel->maps.percpu is emitted, will users be forced to fall
> back to untyped manual lookups like bpf_object__find_map_by_name() instead
> of having the intended ergonomic skeleton access?
>

Hmm, Sashiko missed the map type check in get_map_ident() in patch #5.

Adding ".percpu" to the sfxs[] array is not robust, because
".percpu.data" could be translated to "data" identifier instead of
"percpu_data".

With the type check, ".percpu.data" will be translated to "percpu_data"
correctly.

Thanks,
Leon