Re: [PATCH bpf-next v3 2/4] bpf, libbpf: Support global percpu data

[Leon Hwang <leon.hwang@linux.dev>]

On 28/5/25 06:31, Andrii Nakryiko wrote:
> On Mon, May 26, 2025 at 9:22 AM Leon Hwang <leon.hwang@linux.dev> wrote:
>>
>> This patch introduces support for global percpu data in libbpf by adding a
>> new ".data..percpu" section, similar to ".data". It enables efficient
>> handling of percpu global variables in bpf programs.
>>
>> This enhancement improves performance for workloads that benefit from
>> percpu storage.
>>
>> Meanwhile, add bpf_map__is_internal_percpu() API to check whether the map
>> is an internal map used for global percpu variables.
> 
> I'm not a big fan of this super specific API. We do have
> bpf_map__is_internal() to let customer know that map is special in
> some way, but I'd like to avoid making this fine distinction between
> per-CPU internal map vs non-per-CPU (and then why stop there, why not
> have kconfig-specific API, ksym-specific check, etc)?
> 
> All this is mostly useful just for bpftool for skeleton codegen, and
> bpftool already has to know about .percpu prefix, so it doesn't need
> this API to make all these decisions. Let's try to drop this
> bpf_map__is_internal_percpu() API?
> 

To remove bpf_map__is_internal_percpu(), it can be replaced with:

static bool bpf_map_is_internal_percpu(const struct bpf_map *map)
{
	return bpf_map__is_internal(map) &&
	       bpf_map__type(map) == BPF_MAP_TYPE_PERCPU_ARRAY;
}

This should be functionally equivalent to checking:

map->libbpf_type == LIBBPF_MAP_PERCPU;

>>
>> Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
>> ---
>>  tools/lib/bpf/libbpf.c   | 102 +++++++++++++++++++++++++++++++--------
>>  tools/lib/bpf/libbpf.h   |   9 ++++
>>  tools/lib/bpf/libbpf.map |   1 +
>>  3 files changed, 91 insertions(+), 21 deletions(-)
>>

[...]

>>  struct elf_sec_desc {
>> @@ -1902,7 +1905,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_DATA)
> 
> Not sure this is correct. We should have btf_value_type_id for PERCPU
> global data array, no?
> 

Yes, a PERCPU global data array should indeed have a valid
btf_value_type_id, which is required for seq_show.

This is evident in percpu_array_map_seq_show_elem(), where
btf_value_type_id is used to display the per-CPU values:

static void percpu_array_map_seq_show_elem(struct bpf_map *map, void *key,
					   struct seq_file *m)
{
	// ...

	rcu_read_lock();

	seq_printf(m, "%u: {\n", *(u32 *)key);
	pptr = array->pptrs[index & array->index_mask];
	for_each_possible_cpu(cpu) {
		seq_printf(m, "\tcpu%d: ", cpu);
		btf_type_seq_show(map->btf, map->btf_value_type_id,
				  per_cpu_ptr(pptr, cpu), m);
		seq_putc(m, '\n');
	}
	seq_puts(m, "}\n");

	rcu_read_unlock();
}

So the check for !map->btf_value_type_id in combination with
LIBBPF_MAP_PERCPU_DATA needs to be considered.

>>                 return false;
>>
>>         t = btf__type_by_id(obj->btf, map->btf_value_type_id);
>> @@ -1926,6 +1929,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)

[...]

>>
>> +       /* .data..percpu DATASEC must have __aligned(8) size. */
> 
> please remind me why? similarly for def->value_size special casing?
> What will happen if we don't explicitly roundup() on libbpf side
> (kernel always does roundup(8) for ARRAY value_size anyways, which is
> why I am asking)
> 

t->size must match def->value_size.

That said, I believe it's acceptable to avoid using roundup() for both
values. I'll test this using seq_show to confirm.

>> +       if (strcmp(sec_name, PERCPU_DATA_SEC) == 0 || str_has_pfx(sec_name, PERCPU_DATA_SEC))
>> +               t->size = roundup(t->size, 8);
>> +
>>         for (i = 0, vsi = btf_var_secinfos(t); i < vars; i++, vsi++) {
>>                 const struct btf_type *t_var;
>>                 struct btf_var *var;
>> @@ -3923,6 +3939,11 @@ static int bpf_object__elf_collect(struct bpf_object *obj)

[...]

>> +       data_sz = map->def.value_size;
>> +       if (is_percpu) {
>> +               num_cpus = libbpf_num_possible_cpus();
>> +               if (num_cpus < 0) {
>> +                       err = num_cpus;
>> +                       return err;
> 
> hm... why not `return num_cpus;`?
> 

Ack.

>> +               }
>> +
>> +               data_sz = data_sz * num_cpus;
>> +               data = malloc(data_sz);
>> +               if (!data) {
>> +                       err = -ENOMEM;
>> +                       return err;
>> +               }
> 
> [...]
> 
>>  /**
>>   * @brief **bpf_map__set_pin_path()** sets the path attribute that tells where the
>>   * BPF map should be pinned. This does not actually create the 'pin'.
>> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
>> index 1205f9a4fe048..1c239ac88c699 100644
>> --- a/tools/lib/bpf/libbpf.map
>> +++ b/tools/lib/bpf/libbpf.map
>> @@ -443,4 +443,5 @@ LIBBPF_1.6.0 {
>>                 bpf_program__line_info_cnt;
>>                 btf__add_decl_attr;
>>                 btf__add_type_attr;
>> +               bpf_map__is_internal_percpu;
> 
> alphabetically sorted
> 

bpf_map__is_internal_percpu will be dropped.

Thanks,
Leon