Re: [PATCH bpf-next v12 05/10] libbpf: Add support for global percpu data

On 14/8/26 00:26, [email protected] wrote:
>> diff --git a/tools/lib/bpf/bpf_gen_internal.h b/tools/lib/bpf/bpf_gen_internal.h
>> index 0425691877521..6c5ad6c55e8a6 100644
>> --- a/tools/lib/bpf/bpf_gen_internal.h
>> +++ b/tools/lib/bpf/bpf_gen_internal.h
> 
> [ ... ]
> 
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 514e4e9daa825..e574870fb7169 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
> 
> [ ... ]
> 
>> @@ -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;
> 
> This isn't a bug, but bpf_map_mmap_sz() is documented as returning the
> length to pass to mmap()/munmap()/mprotect(), and both other arms return
> page-granular lengths (array_map_mmap_sz() rounds up to page_sz, ARENA
> multiplies by page_sz).
> 
> The new PERCPU_ARRAY arm returns the raw, unrounded value_size. This
> creates an asymmetry visible in bpf_map__set_value_size(), which compares
> mmap_old_sz = bpf_map_mmap_sz(map) (unrounded for percpu) against
> mmap_new_sz = array_map_mmap_sz(size, max_entries) (page-rounded), so even
> a no-op resize performs a redundant mmap() + memcpy() + munmap() cycle.
> 
> Would using array_map_mmap_sz(map->def.value_size, map->def.max_entries)
> for the PERCPU_ARRAY case be more consistent with the ARRAY case?

Correct.

Will move 'case BPF_MAP_TYPE_PERCPU_ARRAY:' upward one line, and drop
'return map->def.value_size;' line.

> 
> [ ... ]
> 
>> @@ -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)
>>  {
>>  	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)
>>  		pfx_len = 0;
>>  	else
>>  		pfx_len = min((size_t)BPF_OBJ_NAME_LEN - sfx_len - 1, strlen(obj->name));
> 
> Emil Tsalapatis raised a design question in v10: Why does .percpu get
> special treatment in internal_map_name() instead of following the same
> prefixing logic as .data and .rodata?
> 
> He noted "it seems like we're singling out the .percpu section for highly
> nonbvious reasons" and asked "@Andrii Wdyt?" You responded that you prefer
> excluding PERCPU "to avoid the legacy naming convention for new internal
> maps" but said "Let us wait for Andrii's comment."
> 
> Andrii didn't publicly respond to this design question in the lore threads,
> and v11/v12 proceeded with the same special treatment. The related
> map_uses_real_name() function also treats PERCPU differently (always
> returns true for PERCPU, while DATA/RODATA check against their base section
> names).
> 
> Should this naming convention uniformity concern be addressed, or is there
> a technical reason for .percpu's special handling that could be documented
> in the comment?

See Andrii's comment.

> 
> [ ... ]
> 
>> @@ -1941,6 +1950,13 @@ static bool map_is_mmapable(struct bpf_object *obj, struct bpf_map *map)
>>  	if (!map->btf_value_type_id)
>>  		return false;
>>
>> +	/*
>> +	 * The internal PERCPU maps are not mmapble because the underlying
>> +	 * percpu_array maps do not have mmap support.
>> +	 */
>> +	if (map->libbpf_type == LIBBPF_MAP_PERCPU)
>> +		return false;
>> +
> 
> This isn't a bug, but there's a typo: "mmapble" should be "mmappable".

Yes, it is a typo. "mmapble" should be "mmapable".

> 
> [ ... ]
> 
>> @@ -5293,18 +5326,20 @@ 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;
>> +	const __u64 update_flags = is_percpu ? BPF_F_ALL_CPUS : 0;
>>  	int err, zero = 0;
>>  	size_t mmap_sz;
>>
>>  	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 +5384,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;
> 
> This isn't a bug, but unlike every other internal map, the percpu
> initialization image is deliberately kept mapped and turned read-only with
> mprotect() instead of being munmap()ed.
> 
> The skeleton pointer that bpftool generates for this map is a plain,
> non-const 'struct <obj>__percpu *', so a perfectly ordinary post-load
> assignment such as 'skel->percpu->var = 1;' now faults with SIGSEGV in the
> user process rather than failing in a diagnosable way.
> 
> The behaviour is intentional and described in the commit message, but would
> a const-qualified generated pointer (as is done for .rodata) express the
> restriction at compile time instead of at runtime?

See Andrii's comment.

Thanks,
Leon

> 
> [ ... ]
> 
> 
> ---
> AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
> 
> CI run summary: https://github.com/kernel-patches/bpf/actions/runs/31716447858