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

[Leon Hwang]

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.

Thanks,
Leon

> 
>>  	return false;
>>  }
>>  
>