Re: [PATCH bpf-next v12 06/10] bpftool: Generate skeleton for global percpu data

On 14/8/26 00:26, [email protected] wrote:
>> diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
>> index a01d06d22d1a3..a50540ef6521c 100644
>> --- a/tools/bpf/bpftool/gen.c
>> +++ b/tools/bpf/bpftool/gen.c
> 
> [ ... ]
> 
>> @@ -254,7 +260,7 @@ static const struct btf_type *find_type_for_map(struct btf *btf, const char *map
>>  	return NULL;
>>  }
>>
>> -static bool is_mmapable_map(const struct bpf_map *map, char *buf, size_t sz)
>> +static bool is_skel_data(const struct bpf_map *map, char *buf, size_t sz)
>>  {
>>  	size_t tmp_sz;
>>
>> @@ -263,13 +269,24 @@ static bool is_mmapable_map(const struct bpf_map *map, char *buf, size_t sz)
>>  		return true;
>>  	}
>>
>> -	if (!bpf_map__is_internal(map) || !(bpf_map__map_flags(map) & BPF_F_MMAPABLE))
>> +	if (!bpf_map__is_internal(map))
>>  		return false;
>>
>>  	if (!get_map_ident(map, buf, sz))
>>  		return false;
>>
>> -	return true;
>> +	if (bpf_map__map_flags(map) & BPF_F_MMAPABLE)
>> +		return true;
>> +
>> +	if (bpf_map__type(map) == BPF_MAP_TYPE_PERCPU_ARRAY)
>> +		return bpf_map__btf_value_type_id(map) != 0;
>> +
>> +	return false;
>> +}
>> +
>> +static bool is_mmapable_map(const struct bpf_map *map, char *buf, size_t sz)
>> +{
>> +	return is_skel_data(map, buf, sz) && bpf_map__type(map) != BPF_MAP_TYPE_PERCPU_ARRAY;
>>  }
>>
>>  static int codegen_datasecs(struct bpf_object *obj, const char *obj_name)
>> @@ -287,7 +304,7 @@ static int codegen_datasecs(struct bpf_object *obj, const char *obj_name)
>>
>>  	bpf_object__for_each_map(map, obj) {
>>  		/* only generate definitions for memory-mapped internal maps */
>> -		if (!is_mmapable_map(map, map_ident, sizeof(map_ident)))
>> +		if (!is_skel_data(map, map_ident, sizeof(map_ident)))
>>  			continue;
> 
> The comment here says "only generate definitions for memory-mapped
> internal maps," but is_skel_data() now also returns true for
> BPF_MAP_TYPE_PERCPU_ARRAY maps. Those percpu maps are never
> memory-mapped: map_is_mmapable() in tools/lib/bpf/libbpf.c returns false
> for LIBBPF_MAP_PERCPU, which is why this patch had to add mprotect() for
> the init buffer rather than relying on shared mmap.
> 
> When you removed the identical comment from codegen_maps_skeleton()
> (line 988), should this one have been updated to say "skeleton data maps"
> or "internal maps exposed as skeleton data" to match what is_skel_data()
> actually checks?
> 
> The same wording also appears at line 534 in codegen_asserts():

Will update the comments.

> 
>>  	bpf_object__for_each_map(map, obj) {
>> -		if (!is_mmapable_map(map, map_ident, sizeof(map_ident)))
>> +		if (!is_skel_data(map, map_ident, sizeof(map_ident)))
>>  			continue;
> 
> where the comment above (line 513) says "Emit type size asserts for all
> top-level fields in memory-mapped internal maps." That comment now covers
> percpu maps too.

Will update the comment to "Emit type size asserts for all top-level
fields in skel data maps.".

Thanks,
Leon

>