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?
>
> 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?
> 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
> struct bpf_link **link;
> };
>
> --
> 2.47.1
>
