Re: [PATCH bpf-next v10 4/9] libbpf: Add support for global percpu data

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;
> >>  }
> >>
> >
>