On 20/7/26 12:59, Leon Hwang 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:
>>
>>> {
>>> 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.
>
>>
>> @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.
>
>>
>>>
>>> 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.
>
Hi Andrii,
Any comment?
Thanks,
Leon
