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

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