Re: [PATCH bpf-next 3/4] bpf, bpftool: Generate skeleton for global percpu data

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

On 6/2/25 08:09, Andrii Nakryiko wrote:
> On Mon, Jan 27, 2025 at 8:22 AM Leon Hwang <leon.hwang@linux.dev> 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@linux.dev>
>> ---
>>  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