From: Leon Hwang <leon.hwang@linux.dev>
To: Emil Tsalapatis <emil@etsalapatis.com>, bpf@vger.kernel.org
Cc: Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Andrii Nakryiko <andrii@kernel.org>,
Martin KaFai Lau <martin.lau@linux.dev>,
Eduard Zingerman <eddyz87@gmail.com>,
Kumar Kartikeya Dwivedi <memxor@gmail.com>,
Song Liu <song@kernel.org>,
Yonghong Song <yonghong.song@linux.dev>,
Jiri Olsa <jolsa@kernel.org>,
John Fastabend <john.fastabend@gmail.com>,
Quentin Monnet <qmo@kernel.org>, Shuah Khan <shuah@kernel.org>,
linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org,
kernel-patches-bot@fb.com
Subject: Re: [PATCH bpf-next v10 5/9] bpftool: Generate skeleton for global percpu data
Date: Mon, 20 Jul 2026 13:00:57 +0800 [thread overview]
Message-ID: <149c6e74-fe9e-4b7e-a782-015cc44bec18@linux.dev> (raw)
In-Reply-To: <DK16GZ1CVRYZ.1ZKXOISH3846Z@etsalapatis.com>
On 18/7/26 05:52, Emil Tsalapatis wrote:
> On Wed Jul 15, 2026 at 11:32 AM EDT, Leon Hwang wrote:
[...]
>>
>> 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", ".bss", ".percpu", ".kconfig" };
>> int i, n;
>>
>> /* recognize hard coded LLVM section name */
>> @@ -254,7 +260,7 @@ static const struct btf_type *find_type_for_map(struct btf *btf, const char *map
>> return NULL;
>> }
>>
>> -static bool is_mmapable_map(const struct bpf_map *map, char *buf, size_t sz)
>> +static bool is_skel_data(const struct bpf_map *map, char *buf, size_t sz)
>
> Here we change the function to take PERCPU_ARRAY into account, but then
> immediately turn around and add checks of the form is_skel_data && type
> != BPF_MAP_TYPE_PERCPU_ARRAY. If we keep both the existing mmapable_map
> and add a separate is_skel_data map (possibly using is_mappable_map),
> we can remove those additional checks.
Agreed.
I'd like to keep this new is_skel_data(), then update the
is_mmapable_map() to:
static bool is_mmapable_map(const struct bpf_map *map, char *buf, size_t sz)
{
return is_skel_data(map, buf, sz) && bpf_map__type(map) !=
BPF_MAP_TYPE_PERCPU_ARRAY;
}
Yep, it is not a good choice to use '!=' here. But it can simplify the code.
>
>> {
>> size_t tmp_sz;
>>
>> @@ -263,13 +269,19 @@ 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))
>> return false;
>>
>> if (!get_map_ident(map, buf, sz))
>> return false;
>>
>> - return true;
>> + if (bpf_map__map_flags(map) & BPF_F_MMAPABLE)
>> + return true;
>> +
>> + if (bpf_map__type(map) == BPF_MAP_TYPE_PERCPU_ARRAY)
>> + return true;
>> +
>> + return false;
>> }
>>
>> static int codegen_datasecs(struct bpf_object *obj, const char *obj_name)
>> @@ -286,8 +298,11 @@ static int codegen_datasecs(struct bpf_object *obj, const char *obj_name)
>> return -errno;
>>
>> bpf_object__for_each_map(map, obj) {
>> - /* only generate definitions for memory-mapped internal maps */
>> - if (!is_mmapable_map(map, map_ident, sizeof(map_ident)))
>> + /*
>> + * Only generate definitions for internal maps that have
>> + * mmapped data.
>> + */
>> + if (!is_skel_data(map, map_ident, sizeof(map_ident)))
>> continue;
>>
>> sec = find_type_for_map(btf, map_ident);
>> @@ -339,8 +354,14 @@ static int codegen_subskel_datasecs(struct bpf_object *obj, const char *obj_name
>> return -errno;
>>
>> bpf_object__for_each_map(map, obj) {
>> - /* only generate definitions for memory-mapped internal maps */
>> - if (!is_mmapable_map(map, map_ident, sizeof(map_ident)))
>> + /*
>> + * Only generate definitions for internal maps that have
>> + * mmapped data.
>> + */
>> + if (!is_skel_data(map, map_ident, sizeof(map_ident)))
>> + continue;
>> +
>> + if (bpf_map__type(map) == BPF_MAP_TYPE_PERCPU_ARRAY)
>> continue;
>
> Case in point, the old is_mmapable_map would work fine here.
Yes.
>
>>
>> sec = find_type_for_map(btf, map_ident);
>> @@ -493,7 +514,10 @@ static size_t bpf_map_mmap_sz(const struct bpf_map *map)
>> return map_sz;
>> }
>>
>> -/* Emit type size asserts for all top-level fields in memory-mapped internal maps. */
>> +/*
>> + * Emit type size asserts for all top-level fields in internal maps that
>> + * have mmaped data.
>
> I do not think the new comment makes it any clearer, the difference is
> "memory-mapped internal maps" and "maps that are memory-mapped initially
> but actually inaccessible after loading because there's no way to
> represent them as a mapping of the skeleton" AFAICT which is not easy
> to infer currently.
Hmm, it is hard to explain the new case that the mmaped data is visible
in user-space, but is invisible in kernel-space.
Will drop this change.
>
>> + */
>> static void codegen_asserts(struct bpf_object *obj, const char *obj_name)
>> {
>> struct btf *btf = bpf_object__btf(obj);
>> @@ -517,7 +541,7 @@ static void codegen_asserts(struct bpf_object *obj, const char *obj_name)
>> ", obj_name);
>>
>> bpf_object__for_each_map(map, obj) {
>> - if (!is_mmapable_map(map, map_ident, sizeof(map_ident)))
>> + if (!is_skel_data(map, map_ident, sizeof(map_ident)))
>> continue;
>>
>> sec = find_type_for_map(btf, map_ident);
>> @@ -669,7 +693,8 @@ static void codegen_destroy(struct bpf_object *obj, const char *obj_name)
>> if (!get_map_ident(map, ident, sizeof(ident)))
>> continue;
>> if (bpf_map__is_internal(map) &&
>> - (bpf_map__map_flags(map) & BPF_F_MMAPABLE))
>> + ((bpf_map__map_flags(map) & BPF_F_MMAPABLE) ||
>> + bpf_map__type(map) == BPF_MAP_TYPE_PERCPU_ARRAY))
>
> I think this gives us a hint on how we could structure is_skel_data
> above, we now have a special calss that is "mappable maps + PERCPU_ARRAY
> whose initial representation in userspace is mappable, but the loaded
> map isn't".
Will use is_skel_data() here.
Thanks,
Leon
>
>> printf("\tskel_free_map_data(skel->%1$s, skel->maps.%1$s.initial_value, %2$zu);\n",
>> ident, bpf_map_mmap_sz(map));
>> codegen("\
>> @@ -741,7 +766,7 @@ static int gen_trace(struct bpf_object *obj, const char *obj_name, const char *h
>> const void *mmap_data = NULL;
>> size_t mmap_size = 0;
>>
>> - if (!is_mmapable_map(map, ident, sizeof(ident)))
>> + if (!is_skel_data(map, ident, sizeof(ident)))
>> continue;
>>
>> codegen("\
>> @@ -849,8 +874,22 @@ static int gen_trace(struct bpf_object *obj, const char *obj_name, const char *h
>> bpf_object__for_each_map(map, obj) {
>> const char *mmap_flags;
>>
>> - if (!is_mmapable_map(map, ident, sizeof(ident)))
>> + if (!is_skel_data(map, ident, sizeof(ident)))
>> + continue;
>> +
>> + if (bpf_map__type(map) == BPF_MAP_TYPE_PERCPU_ARRAY) {
>> + codegen("\
>> + \n\
>> + err = skel_protect_map_data(skel->%1$s, &skel->maps.%1$s.initial_value, %2$zd);\n\
>> + if (err) \n\
>> + return err; \n\
>> + #ifdef __KERNEL__ \n\
>> + skel->%1$s = NULL; \n\
>> + #endif \n\
>> + ",
>> + ident, bpf_map_mmap_sz(map));
>> continue;
>> + }
>>
>> if (bpf_map__map_flags(map) & BPF_F_RDONLY_PROG)
>> mmap_flags = "PROT_READ";
>> @@ -955,8 +994,7 @@ codegen_maps_skeleton(struct bpf_object *obj, size_t map_cnt, bool mmaped, bool
>> map->map = &obj->maps.%s; \n\
>> ",
>> i, bpf_map__name(map), ident);
>> - /* memory-mapped internal maps */
>> - if (mmaped && is_mmapable_map(map, ident, sizeof(ident))) {
>> + if (mmaped && is_skel_data(map, ident, sizeof(ident))) {
>> printf("\tmap->mmaped = (void **)&obj->%s;\n", ident);
>> }
>>
>> @@ -1740,7 +1778,9 @@ static int do_subskeleton(int argc, char **argv)
>> /* Also count all maps that have a name */
>> map_cnt++;
>>
>> - if (!is_mmapable_map(map, ident, sizeof(ident)))
>> + if (!is_skel_data(map, ident, sizeof(ident)))
>> + continue;
>> + if (bpf_map__type(map) == BPF_MAP_TYPE_PERCPU_ARRAY)
>> continue;
>
> Another instance of what I was talking about.
>
>>
>> map_type_id = bpf_map__btf_value_type_id(map);
>> @@ -1863,7 +1903,9 @@ static int do_subskeleton(int argc, char **argv)
>>
>> /* walk through each symbol and emit the runtime representation */
>> bpf_object__for_each_map(map, obj) {
>> - if (!is_mmapable_map(map, ident, sizeof(ident)))
>> + if (!is_skel_data(map, ident, sizeof(ident)))
>> + continue;
>> + if (bpf_map__type(map) == BPF_MAP_TYPE_PERCPU_ARRAY)
>> continue;
>>
>> map_type_id = bpf_map__btf_value_type_id(map);
>> diff --git a/tools/lib/bpf/skel_internal.h b/tools/lib/bpf/skel_internal.h
>> index 53fee53d36d5..1f3f332dffbe 100644
>> --- a/tools/lib/bpf/skel_internal.h
>> +++ b/tools/lib/bpf/skel_internal.h
>> @@ -131,8 +131,10 @@ static inline void skel_free_map_data(void *p, __u64 addr, size_t sz)
>> {
>> if (addr != ~0ULL)
>> kvfree(p);
>> - /* When addr == ~0ULL the 'p' points to
>> - * ((struct bpf_array *)map)->value. See skel_finalize_map_data.
>> + /*
>> + * When addr == ~0ULL the init buffer has already been released.
>> + * For skel_finalize_map_data(), 'p' points to
>> + * ((struct bpf_array *)map)->value.
>> */
>> }
>>
>> @@ -170,6 +172,15 @@ static inline void *skel_finalize_map_data(__u64 *init_val, size_t mmap_sz, int
>> return addr;
>> }
>>
>> +static inline int skel_protect_map_data(void *p, __u64 *init_val, size_t sz)
>> +{
>> + (void)sz;
>> +
>> + kvfree(p);
>> + *init_val = ~0ULL;
>> + return 0;
>> +}
>> +
>> #else
>>
>> static inline void *skel_alloc(size_t size)
>> @@ -208,6 +219,15 @@ static inline void *skel_finalize_map_data(__u64 *init_val, size_t mmap_sz, int
>> return NULL;
>> return addr;
>> }
>> +
>> +static inline int skel_protect_map_data(void *p, __u64 *init_val, size_t sz)
>> +{
>> + (void)init_val;
>> +
>> + if (mprotect(p, sz, PROT_READ))
>> + return -errno;
>> + return 0;
>> +}
>> #endif
>>
>> static inline int skel_closenz(int fd)
>
next prev parent reply other threads:[~2026-07-20 5:01 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-07-15 15:32 [PATCH bpf-next v10 0/9] bpf: Introduce global percpu data Leon Hwang
2026-07-15 15:32 ` [PATCH bpf-next v10 1/9] bpf: Drop duplicate blank lines in verifier Leon Hwang
2026-07-17 5:08 ` Emil Tsalapatis
2026-07-17 5:28 ` Leon Hwang
2026-07-21 13:48 ` Leon Hwang
2026-07-15 15:32 ` [PATCH bpf-next v10 2/9] bpf: Introduce global percpu data Leon Hwang
2026-07-17 6:25 ` Emil Tsalapatis
2026-07-20 4:59 ` Leon Hwang
2026-07-20 22:55 ` Emil Tsalapatis
2026-07-15 15:32 ` [PATCH bpf-next v10 3/9] libbpf: Probe percpu data feature Leon Hwang
2026-07-17 21:52 ` Emil Tsalapatis
2026-07-15 15:32 ` [PATCH bpf-next v10 4/9] libbpf: Add support for global percpu data Leon Hwang
2026-07-15 16:00 ` sashiko-bot
2026-07-16 5:21 ` Leon Hwang
2026-07-17 21:39 ` Emil Tsalapatis
2026-07-20 4:59 ` Leon Hwang
2026-07-15 15:32 ` [PATCH bpf-next v10 5/9] bpftool: Generate skeleton " Leon Hwang
2026-07-17 21:52 ` Emil Tsalapatis
2026-07-20 5:00 ` Leon Hwang [this message]
2026-07-15 15:32 ` [PATCH bpf-next v10 6/9] selftests/bpf: Add tests to verify " Leon Hwang
2026-07-15 15:55 ` sashiko-bot
2026-07-16 5:25 ` Leon Hwang
2026-07-15 16:11 ` bot+bpf-ci
2026-07-16 5:30 ` Leon Hwang
2026-07-17 22:47 ` Emil Tsalapatis
2026-07-20 5:02 ` Leon Hwang
2026-07-15 15:32 ` [PATCH bpf-next v10 7/9] selftests/bpf: Test direct reading/writing read-only percpu_array map Leon Hwang
2026-07-17 22:49 ` Emil Tsalapatis
2026-07-20 5:03 ` Leon Hwang
2026-07-15 15:32 ` [PATCH bpf-next v10 8/9] selftests/bpf: Test verifier log for global percpu data Leon Hwang
2026-07-17 22:50 ` Emil Tsalapatis
2026-07-15 15:32 ` [PATCH bpf-next v10 9/9] selftests/bpf: Verify bpf_iter " Leon Hwang
2026-07-17 23:11 ` Emil Tsalapatis
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=149c6e74-fe9e-4b7e-a782-015cc44bec18@linux.dev \
--to=leon.hwang@linux.dev \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=eddyz87@gmail.com \
--cc=emil@etsalapatis.com \
--cc=john.fastabend@gmail.com \
--cc=jolsa@kernel.org \
--cc=kernel-patches-bot@fb.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=martin.lau@linux.dev \
--cc=memxor@gmail.com \
--cc=qmo@kernel.org \
--cc=shuah@kernel.org \
--cc=song@kernel.org \
--cc=yonghong.song@linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox