Re: [PATCH bpf-next v8 03/10] bpf: add struct_ops_tab to btf.

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

On 10/30/23 18:09, Martin KaFai Lau wrote:
> On 10/30/23 12:28 PM, thinker.li@gmail.com wrote:
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> Maintain a registry of registered struct_ops types in the per-btf 
>> (module)
>> struct_ops_tab. This registry allows for easy lookup of struct_ops types
>> that are registered by a specific module.
>>
>> Every struct_ops type should have an associated module BTF to provide 
>> type
>> information since we are going to allow modules to define and register 
>> new
>> struct_ops types. Once this change is made, the bpf_struct_ops subsystem
>> knows where to look up type info with just a bpf_struct_ops.
> 
> I think this part needs better description. I found it hard to parse. In 
> particular:
> 
> ...
>    the "bpf_struct_ops" subsystem
>         knows where to look up type info with just
>      a "bpf_struct_ops"
> ...
> 
> May be something like:
> 
> It is a preparation work for supporting kernel module struct_ops in a 
> latter patch. Each struct_ops will be registered under its own kernel 
> module btf and will be stored in the newly added btf->struct_ops_tab. 
> The bpf verifier and bpf syscall (e.g. prog and map cmd) can find the 
> struct_ops and its btf type/size/id... information from 
> btf->struct_ops_tab.


Got it!

> 
>>
>> The subsystem looks up struct_ops types from a given module BTF 
>> although it
>> is always btf_vmlinux now. Once start using struct_ops_tab, btfs other 
>> than
>> btf_vmlinux can be used as well.
> 
> I think this describes about the "struct btf *btf" argument change in 
> this patch. This seems unrelated to the "add struct_ops_tab to btf" 
> change. Can it be in its own preparation patch?
> 

Sure!

> [ ... ]
> 
>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>> index e35d6321a2f8..0bc21a39257d 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -186,6 +186,7 @@ static void bpf_struct_ops_init_one(struct 
>> bpf_struct_ops_desc *st_ops_desc,
>>               pr_warn("Error in init bpf_struct_ops %s\n",
>>                   st_ops->name);
>>           } else {
>> +            st_ops_desc->btf = btf;
>>               st_ops_desc->type_id = type_id;
>>               st_ops_desc->type = t;
>>               st_ops_desc->value_id = value_id;
>> @@ -222,7 +223,7 @@ void bpf_struct_ops_init(struct btf *btf, struct 
>> bpf_verifier_log *log)
>>   extern struct btf *btf_vmlinux;
>>   static const struct bpf_struct_ops_desc *
>> -bpf_struct_ops_find_value(u32 value_id)
>> +bpf_struct_ops_find_value(struct btf *btf, u32 value_id)
> 
> The "!btf_vmlinux" check a few lines below should also be changed to 
> "!btf". I think I had commented on a similar point in v5.

At this patch, btf is still always btf_vmlinux until the patch 6 and 7.
I will move these changes from the patch 6 and 7 to here or
the new patch mentioned above.

> 
>>   {
>>       unsigned int i;
>> @@ -237,7 +238,8 @@ bpf_struct_ops_find_value(u32 value_id)
>>       return NULL;
>>   }
>> -const struct bpf_struct_ops_desc *bpf_struct_ops_find(u32 type_id)
>> +const struct bpf_struct_ops_desc *
>> +bpf_struct_ops_find(struct btf *btf, u32 type_id)
> 
> same here.
>

Got it!

>>   {
>>       unsigned int i;
> 
> [ ... ]
> 
>> +static struct bpf_struct_ops_desc *
>> +btf_add_struct_ops(struct btf *btf, struct bpf_struct_ops *st_ops)
>> +{
>> +    struct btf_struct_ops_tab *tab, *new_tab;
>> +    int i;
>> +
>> +    if (!btf)
>> +        return ERR_PTR(-ENOENT);
>> +
>> +    /* Assume this function is called for a module when the module is
>> +     * loading.
>> +     */
>> +
>> +    tab = btf->struct_ops_tab;
>> +    if (!tab) {
>> +        tab = kzalloc(offsetof(struct btf_struct_ops_tab, ops[4]),
>> +                  GFP_KERNEL);
>> +        if (!tab)
>> +            return ERR_PTR(-ENOMEM);
>> +        tab->capacity = 4;
>> +        btf->struct_ops_tab = tab;
>> +    }
>> +
>> +    for (i = 0; i < tab->cnt; i++)
>> +        if (tab->ops[i].st_ops == st_ops)
>> +            return ERR_PTR(-EEXIST);
>> +
>> +    if (tab->cnt == tab->capacity) {
>> +        new_tab = krealloc(tab, sizeof(*tab) +
>> +                   sizeof(struct bpf_struct_ops *) *
>> +                   tab->capacity * 2, GFP_KERNEL);
> 
> nit. Use a similar offsetof() like a few lines above.
Sure!
> 
>> +        if (!new_tab)
>> +            return ERR_PTR(-ENOMEM);
>> +        tab = new_tab;
>> +        tab->capacity *= 2;
>> +        btf->struct_ops_tab = tab;
>> +    }
>> +
>> +    btf->struct_ops_tab->ops[btf->struct_ops_tab->cnt].st_ops = st_ops;
> 
> nit. s/btf->struct_ops_tab/tab/
> 

Sure!

>> +
>> +    return &btf->struct_ops_tab->ops[btf->struct_ops_tab->cnt++];
>> +}
>