On 6/2/25 08:09, Andrii Nakryiko wrote:
> On Mon, Jan 27, 2025 at 8:22 AM Leon Hwang <leon.hwang@xxxxxxxxx> wrote:
>>
>> This patch enhances bpftool to generate skeletons for global percpu
>> variables. The generated skeleton includes a dedicated structure for
>> percpu data, allowing users to initialize and access percpu variables
>> efficiently.
>>
>> Changes:
>>
>> 1. skeleton structure:
>>
>> For global percpu variables, the skeleton now includes a nested
>> structure, e.g.:
>>
>> struct test_global_percpu_data {
>> struct bpf_object_skeleton *skeleton;
>> struct bpf_object *obj;
>> struct {
>> struct bpf_map *bss;
>> struct bpf_map *percpu;
>> } maps;
>> // ...
>> struct test_global_percpu_data__percpu {
>> int percpu_data;
>> } *percpu;
>>
>> // ...
>> };
>>
>> * The "struct test_global_percpu_data__percpu" points to initialized
>> data, which is actually "maps.percpu->data".
>> * Before loading the skeleton, updating the
>> "struct test_global_percpu_data__percpu" modifies the initial value
>> of the corresponding global percpu variables.
>> * After loading the skeleton, accessing or updating this struct is no
>> longer meaningful. Instead, users must interact with the global
>> percpu variables via the "maps.percpu" map.
>
> can we set this pointer to NULL after load to avoid accidental
> (successful) reads/writes to it?
>
Good idea. I'll try to achieve it.
>>
>> 2. code changes:
>>
>> * Added support for ".percpu" sections in bpftool's map identification
>> logic.
>> * Modified skeleton generation to handle percpu data maps
>> appropriately.
>> * Updated libbpf to make "percpu" pointing to "maps.percpu->data".
>>
>> Signed-off-by: Leon Hwang <leon.hwang@xxxxxxxxx>
>> ---
>> tools/bpf/bpftool/gen.c | 13 +++++++++----
>> tools/lib/bpf/libbpf.c | 3 +++
>> tools/lib/bpf/libbpf.h | 1 +
>> 3 files changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
>> index 5a4d3240689ed..975775683ca12 100644
>> --- a/tools/bpf/bpftool/gen.c
>> +++ b/tools/bpf/bpftool/gen.c
>> @@ -92,7 +92,7 @@ static void get_header_guard(char *guard, const char *obj_name, const char *suff
>>
>> static bool get_map_ident(const struct bpf_map *map, char *buf, size_t buf_sz)
>> {
>> - static const char *sfxs[] = { ".data", ".rodata", ".bss", ".kconfig" };
>> + static const char *sfxs[] = { ".data", ".rodata", ".percpu", ".bss", ".kconfig" };
>> const char *name = bpf_map__name(map);
>> int i, n;
>>
>> @@ -117,7 +117,7 @@ static bool get_map_ident(const struct bpf_map *map, char *buf, size_t buf_sz)
>>
>> static bool get_datasec_ident(const char *sec_name, char *buf, size_t buf_sz)
>> {
>> - static const char *pfxs[] = { ".data", ".rodata", ".bss", ".kconfig" };
>> + static const char *pfxs[] = { ".data", ".rodata", ".percpu", ".bss", ".kconfig" };
>> int i, n;
>>
>> /* recognize hard coded LLVM section name */
>> @@ -263,7 +263,9 @@ static bool is_mmapable_map(const struct bpf_map *map, char *buf, size_t sz)
>> return true;
>> }
>>
>> - if (!bpf_map__is_internal(map) || !(bpf_map__map_flags(map) & BPF_F_MMAPABLE))
>> + if (!bpf_map__is_internal(map) ||
>> + (!(bpf_map__map_flags(map) & BPF_F_MMAPABLE) &&
>> + bpf_map__type(map) != BPF_MAP_TYPE_PERCPU_ARRAY))
>
> there will be no BPF_F_MMAPABLE set for PERCPU_ARRAY, why adding these
> unnecessary extra conditionals?
>
Ack. It's unnecessary.
>> return false;
>>
>> if (!get_map_ident(map, buf, sz))
>> @@ -903,7 +905,10 @@ codegen_maps_skeleton(struct bpf_object *obj, size_t map_cnt, bool mmaped, bool
>> i, bpf_map__name(map), ident);
>> /* memory-mapped internal maps */
>> if (mmaped && is_mmapable_map(map, ident, sizeof(ident))) {
>> - printf("\tmap->mmaped = (void **)&obj->%s;\n", ident);
>> + if (bpf_map__type(map) == BPF_MAP_TYPE_PERCPU_ARRAY)
>> + printf("\tmap->data = (void **)&obj->%s;\n", ident);
>> + else
>> + printf("\tmap->mmaped = (void **)&obj->%s;\n", ident);
>> }
>>
>> if (populate_links && bpf_map__type(map) == BPF_MAP_TYPE_STRUCT_OPS) {
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 6da6004c5c84d..dafb419bc5b86 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -13915,6 +13915,7 @@ static int populate_skeleton_maps(const struct bpf_object *obj,
>> struct bpf_map **map = map_skel->map;
>> const char *name = map_skel->name;
>> void **mmaped = map_skel->mmaped;
>> + void **data = map_skel->data;
>>
>> *map = bpf_object__find_map_by_name(obj, name);
>> if (!*map) {
>> @@ -13925,6 +13926,8 @@ static int populate_skeleton_maps(const struct bpf_object *obj,
>> /* externs shouldn't be pre-setup from user code */
>> if (mmaped && (*map)->libbpf_type != LIBBPF_MAP_KCONFIG)
>> *mmaped = (*map)->mmaped;
>> + if (data && (*map)->libbpf_type == LIBBPF_MAP_PERCPU_DATA)
>> + *data = (*map)->data;
>> }
>> return 0;
>> }
>> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
>> index 3020ee45303a0..c49d6e44b5630 100644
>> --- a/tools/lib/bpf/libbpf.h
>> +++ b/tools/lib/bpf/libbpf.h
>> @@ -1701,6 +1701,7 @@ struct bpf_map_skeleton {
>> const char *name;
>> struct bpf_map **map;
>> void **mmaped;
>> + void **data;
>
> this is breaking backwards/forward compatibility. let's try to reuse mmaped
>
Indeed. If reuse map->mmaped for global percpu variables, it's
unnecessary to add this "data".
>> struct bpf_link **link;
>> };
>>
>> --
>> 2.47.1
>>
Thanks,
Leon
