On Sun, Jul 19, 2026 at 10:00 PM Leon Hwang <[email protected]> wrote:
>
> On 18/7/26 05:39, Emil Tsalapatis wrote:
> > On Wed Jul 15, 2026 at 11:32 AM EDT, Leon Hwang wrote:
>
> [...]
>
> >>
> >> struct elf_sec_desc {
> >> @@ -1839,6 +1842,8 @@ static size_t bpf_map_mmap_sz(const struct bpf_map
> >> *map)
> >> switch (map->def.type) {
> >> case BPF_MAP_TYPE_ARRAY:
> >> return array_map_mmap_sz(map->def.value_size,
> >> map->def.max_entries);
> >> + case BPF_MAP_TYPE_PERCPU_ARRAY:
> >> + return map->def.value_size;
> >> case BPF_MAP_TYPE_ARENA:
> >> return page_sz * map->def.max_entries;
> >> default:
> >> @@ -1866,7 +1871,8 @@ static int bpf_map_mmap_resize(struct bpf_map *map,
> >> size_t old_sz, size_t new_sz
> >> return 0;
> >> }
> >>
> >> -static char *internal_map_name(struct bpf_object *obj, const char
> >> *real_name)
> >> +static char *internal_map_name(struct bpf_object *obj, const char
> >> *real_name,
> >> + enum libbpf_map_type type)
> >
> > We can avoid passing the type here by testing against ".percpu" since
> > that's how we are deriving the map type in the first place. But more
> > importantly:
> >
I think type is cleaner, because it's not just .percpu, but also
.percpu.whateveryouwant, so having type is cleaner, IMO.
> >> {
> >> char map_name[BPF_OBJ_NAME_LEN], *p;
> >> int pfx_len, sfx_len = max((size_t)7, strlen(real_name));
> >> @@ -1907,8 +1913,11 @@ static char *internal_map_name(struct bpf_object
> >> *obj, const char *real_name)
> >> if (sfx_len >= BPF_OBJ_NAME_LEN)
> >> sfx_len = BPF_OBJ_NAME_LEN - 1;
> >>
> >> - /* if there are two or more dots in map name, it's a custom dot map */
> >> - if (strchr(real_name + 1, '.') != NULL)
> >> + /*
> >> + * Don't prefix the bpf_object name if this is a custom dot map
> >> + * (containing two or more dots) or a percpu data map.
> >> + */
> >> + if (strchr(real_name + 1, '.') != NULL || type == LIBBPF_MAP_PERCPU)
> >
> > Is there a reason we don't use the exact same logic as the other
> > internal maps here? I understand that a lot of the conventions around
> > the naming are there for legacy reasons, but it seems like we're
> > singling out the .percpu section for highly nonbvious reasons. Imo we
> > should consider doing the same prefixing for a bare ".percpu" section
> > that we do for the other internal ones. At the very least, there needs
> > to be some explanation as to why .percpu gets special treatment.
>
>
> I prefer passing 'type'. Excluding _PERCPU here is to avoid the legacy
> naming convention for new internal maps.
+1
and it's not that .percpu gets special treatment, it's all the legacy
maps that have special treatment
>
> >
> > @Andrii Wdyt?
> >
> >> pfx_len = 0;
> >> else
> >> pfx_len = min((size_t)BPF_OBJ_NAME_LEN - sfx_len - 1,
> >> strlen(obj->name));
> >> @@ -1938,7 +1947,7 @@ static bool map_is_mmapable(struct bpf_object *obj,
> >> struct bpf_map *map)
> >> struct btf_var_secinfo *vsi;
> >> int i, n;
> >>
> >> - if (!map->btf_value_type_id)
> >> + if (!map->btf_value_type_id || map->libbpf_type == LIBBPF_MAP_PERCPU)
> >> return false;
> >
> > Nit: These are two separate checks rolled into one, and each one checks
> > a different thing. THe type check against MAP_PERCPU merits a comment as
> > well: It's the only internal section that is not really mappable because
> > there's no way to represent it as userspace state.
>
>
> Ack.
>
> Will add a new iff for libbpf_type check with a comment.
yep, but don't overdo comments, it's not that hard to understand why
per-cpu map is not mmapable
>
> >
> >>
> >> t = btf__type_by_id(obj->btf, map->btf_value_type_id);
> >> @@ -1962,6 +1971,7 @@ static int
> >> bpf_object__init_internal_map(struct bpf_object *obj, enum
> >> libbpf_map_type type,
> >> const char *real_name, int sec_idx, void *data,
> >> size_t data_sz)
> >> {
> >> + bool is_percpu = type == LIBBPF_MAP_PERCPU;
> >> struct bpf_map_def *def;
> >> struct bpf_map *map;
> >> size_t mmap_sz;
> [...]
>
> >> @@ -4944,7 +4970,7 @@ static int map_fill_btf_type_info(struct bpf_object
> >> *obj, struct bpf_map *map)
> >>
> >> /*
> >> * LLVM annotates global data differently in BTF, that is,
> >> - * only as '.data', '.bss' or '.rodata'.
> >> + * only as '.data', '.bss', '.percpu' or '.rodata'.
> >> */
> >> if (!bpf_map__is_internal(map))
> >> return -ENOENT;
> >> @@ -5293,18 +5319,30 @@ static int
> >> bpf_object__populate_internal_map(struct bpf_object *obj, struct bpf_map
> >> *map)
> >> {
> >> enum libbpf_map_type map_type = map->libbpf_type;
> >> + bool is_percpu = map_type == LIBBPF_MAP_PERCPU;
> >
> > Nit: If we do
> > __u64 update_flags = is_percpu ? BPF_F_ALL_CPUS : 0;
> >
> > we can declare the variable as const and ...
> >
> >> + __u64 update_flags = 0;
> >> int err, zero = 0;
> >> size_t mmap_sz;
> >>
> >> + if (is_percpu) {
> >> + if (!obj->gen_loader && !kernel_supports(obj,
> >> FEAT_PERCPU_DATA)) {
> >> + pr_warn("map '%s': kernel does not support percpu
> >> data.\n",
> >> + bpf_map__name(map));
> >> + return -EOPNOTSUPP;
> >> + }
> >> +
> >> + update_flags = BPF_F_ALL_CPUS;
> >> + }
> >
> > ... we can collapse the above into a single nested level:
> >
> > if (is_percpu && !obj->gen_loader && !kernel_supports(obj,
> > FEAT_PERCPU_DATA)) {
> > ...
> > }
>
> Good point.
>
> >
> >> +
> >> if (obj->gen_loader) {
> >> bpf_gen__map_update_elem(obj->gen_loader, map - obj->maps,
> >> - map->mmaped, map->def.value_size);
> >> + map->mmaped, map->def.value_size,
> >> update_flags);
> >> if (map_type == LIBBPF_MAP_RODATA || map_type ==
> >> LIBBPF_MAP_KCONFIG)
> >> bpf_gen__map_freeze(obj->gen_loader, map - obj->maps);
> >> return 0;
> >> }
> >>
> >> - err = bpf_map_update_elem(map->fd, &zero, map->mmaped, 0);
> >> + err = bpf_map_update_elem(map->fd, &zero, map->mmaped, update_flags);
> >> if (err) {
> >> err = -errno;
> >> pr_warn("map '%s': failed to set initial contents: %s\n",
> >> @@ -5349,6 +5387,13 @@ bpf_object__populate_internal_map(struct bpf_object
> >> *obj, struct bpf_map *map)
> >> return err;
> >> }
> >> map->mmaped = mmaped;
> >> + } else if (is_percpu) {
> >> + if (mprotect(map->mmaped, mmap_sz, PROT_READ)) {
> >> + err = -errno;
> >> + pr_warn("map '%s': failed to mprotect() contents:
> >> %s\n",
> >> + bpf_map__name(map), errstr(err));
> >> + return err;
> >> + }
> >> } else if (map->mmaped) {
> >> munmap(map->mmaped, mmap_sz);
> >> map->mmaped = NULL;
> >> @@ -10807,11 +10852,16 @@ static bool map_uses_real_name(const struct
> >> bpf_map *map)
> >> * such map's corresponding ELF section name as a map name.
> >> * This check distinguishes .data/.rodata from .data.* and .rodata.*
> >> * maps to know which name has to be returned to the user.
> >> + * Map name of the custom .percpu.* maps might be truncated to
> >> + * BPF_OBJ_NAME_LEN-1 chars in internal_map_name(). Hence, percpu data
> >> + * maps must use real name for their user-visible name.
> >> */
> >> if (map->libbpf_type == LIBBPF_MAP_DATA && strcmp(map->real_name,
> >> DATA_SEC) != 0)
> >> return true;
> >> if (map->libbpf_type == LIBBPF_MAP_RODATA && strcmp(map->real_name,
> >> RODATA_SEC) != 0)
> >> return true;
> >> + if (map->libbpf_type == LIBBPF_MAP_PERCPU)
> >> + return true;
> >
> > Same comment as above here wrt uniformity. This is the part that
> > requires us to check agianst the map type in __init_internal_map().
>
>
> Let us wait for Andrii's comment.
>
I think it's fine and basically inevitable
> Thanks,
> Leon
>
> >
> >> return false;
> >> }
> >>
> >
>
