Re: [PATCH bpf-next v2 00/11] Enable BPF programs to declare arrays of kptr, bpf_rb_root, and bpf_list_head.

[Kui-Feng Lee <sinquersw@gmail.com>]

On 4/17/24 20:30, Alexei Starovoitov wrote:
> On Fri, Apr 12, 2024 at 2:08 PM Kui-Feng Lee <thinker.li@gmail.com> wrote:
>>
>> The arrays of kptr, bpf_rb_root, and bpf_list_head didn't work as
>> global variables. This was due to these types being initialized and
>> verified in a special manner in the kernel. This patchset allows BPF
>> programs to declare arrays of kptr, bpf_rb_root, and bpf_list_head in
>> the global namespace.
>>
>> The main change is to add "nelems" to btf_fields. The value of
>> "nelems" represents the number of elements in the array if a btf_field
>> represents an array. Otherwise, "nelem" will be 1. The verifier
>> verifies these types based on the information provided by the
>> btf_field.
>>
>> The value of "size" will be the size of the entire array if a
>> btf_field represents an array. Dividing "size" by "nelems" gives the
>> size of an element. The value of "offset" will be the offset of the
>> beginning for an array. By putting this together, we can determine the
>> offset of each element in an array. For example,
>>
>>      struct bpf_cpumask __kptr * global_mask_array[2];
> 
> Looks like this patch set enables arrays only.
> Meaning the following is supported already:
> 
> +private(C) struct bpf_spin_lock glock_c;
> +private(C) struct bpf_list_head ghead_array1 __contains(foo, node2);
> +private(C) struct bpf_list_head ghead_array2 __contains(foo, node2);
> 
> while this support is added:
> 
> +private(C) struct bpf_spin_lock glock_c;
> +private(C) struct bpf_list_head ghead_array1[3] __contains(foo, node2);
> +private(C) struct bpf_list_head ghead_array2[2] __contains(foo, node2);
> 
> Am I right?
> 
> What about the case when bpf_list_head is wrapped in a struct?
> private(C) struct foo {
>    struct bpf_list_head ghead;
> } ghead;
> 
> that's not enabled in this patch. I think.
> 
> And the following:
> private(C) struct foo {
>    struct bpf_list_head ghead;
> } ghead[2];
> 
> 
> or
> 
> private(C) struct foo {
>    struct bpf_list_head ghead[2];
> } ghead;
> 
> Won't work either.

No, they don't work.
We had a discussion about this in the other day.
I proposed to have another patch set to work on struct types.
Do you prefer to handle it in this patch set?

> 
> I think eventually we want to support all such combinations and
> the approach proposed in this patch with 'nelems'
> won't work for wrapper structs.
> 
> I think it's better to unroll/flatten all structs and arrays
> and represent them as individual elements in the flattened
> structure. Then there will be no need to special case array with 'nelems'.
> All special BTF types will be individual elements with unique offset.
> 
> Does this make sense?

That means it will creates 10 btf_field(s) for an array having 10
elements. The purpose of adding "nelems" is to avoid the repetition. Do
you prefer to expand them?


> 
> pw-bot: cr