Re: [RFC bpf-next v2 2/9] bpf: add register and unregister functions for struct_ops.

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

On 9/18/23 11:47, Martin KaFai Lau wrote:
> On 9/15/23 6:14 PM, Kui-Feng Lee wrote:
>>
>>
>> On 9/15/23 17:05, Martin KaFai Lau wrote:
>>> On 9/12/23 11:14 PM, thinker.li@gmail.com wrote:
>>>> +int register_bpf_struct_ops(struct bpf_struct_ops_mod *mod)
>>>> +{
>>>> +    struct bpf_struct_ops *st_ops = mod->st_ops;
>>>> +    struct bpf_verifier_log *log;
>>>> +    struct btf *btf;
>>>> +    int err;
>>>> +
>>>> +    if (mod->st_ops == NULL ||
>>>> +        mod->owner == NULL)
>>>> +        return -EINVAL;
>>>> +
>>>> +    log = kzalloc(sizeof(*log), GFP_KERNEL | __GFP_NOWARN);
>>>> +    if (!log) {
>>>> +        err = -ENOMEM;
>>>> +        goto errout;
>>>> +    }
>>>> +
>>>> +    log->level = BPF_LOG_KERNEL;
>>>> +
>>>> +    btf = btf_get_module_btf(mod->owner);
>>>
>>> Where is btf_put called?
>>>
>>> It is not stored anywhere in patch 2, so a bit confusing. I quickly 
>>> looked at the following patches but also don't see the bpf_put.
>>
>> It is my fault to use it without calling btf_put().
>> I miss-understood the API, thought it doesn't increase refcount by
>> mistake.
>>
>>>
>>>> +    if (!btf) {
>>>> +        err = -EINVAL;
>>>> +        goto errout;
>>>> +    }
>>>> +
>>>> +    bpf_struct_ops_init_one(st_ops, btf, log);
>>>> +    err = add_struct_ops(st_ops);
>>>> +
>>>> +errout:
>>>> +    kfree(log);
>>>> +
>>>> +    return err;
>>>> +}
>>>> +EXPORT_SYMBOL(register_bpf_struct_ops);
>>>> +
>>>> +int unregister_bpf_struct_ops(struct bpf_struct_ops_mod *mod)
>>>
>>> It is not clear to me why the subsystem needs to explicitly call 
>>> unregister_bpf_struct_ops(). Can it be done similar to the module 
>>> kfunc support (the kfunc_set_tab goes away with the btf)?
>>
>> It could be. However, registering to module notifier
>> (register_module_notifier()) is more straight forward if we go
>> this way. What do you think?
> 
> Right, but not sure if struct_ops needs to create yet another notifier 
> considering there is already a btf_module_notify(). It is why the 
> earlier question on btf_put because I was trying to understand if the 
> struct_ops can go away together during btf_free. More on this next.

In short, it is not necessary to have another notifier.
The benefit with a separated notifier is loose coupling without touching
btf code. I don't have a strong opinion on this.


> 
>>
>>>
>>> Related to this, does it need to maintain a global struct_ops array 
>>> for all kernel module? Can the struct_ops be maintained under its 
>>> corresponding module btf itself?
>>
>> What is the purpose?
>> We have a global struct_ops array already, although it is not
>> per-module. For now, the number of struct_ops is pretty small.
>> We have only one so far, and it is unlikely to grow fast in
>> near future. It is probably a bit overkill to have
>> per-module ones if this is what you mean.
> 
> The array size is not the concern.
> 
> The global struct_ops array was created before btf supporting kernel 
> module. Since then, btf module and kfunc module support were added.
> 
> To maintain this global struct_ops array, it needs to register its own 
> module notifier, maintains its own mutex_lock (in patch 5), and also the 
> modified bpf_struct_ops_find*() is searching something under a specific 
> btf module.
> 
> afaict, the current btf kfunc support has the infrastructure to do all 
> these (for example, the global LIST_HEAD(btf_modules), btf_module_mutex, 
> btf_module_notify()...etc). Why struct_ops needs to be special and 
> reinvent something which looks very similar to btf kfunc? Did I missing 
> something that struct_ops needs special handling?


I don't think you missing anything.

> 
>>
>>>
>>>> +{
>>>> +    struct bpf_struct_ops *st_ops = mod->st_ops;
>>>> +    int err;
>>>> +
>>>> +    err = remove_struct_ops(st_ops);
>>>> +    if (!err && st_ops->uninit)
>>>> +        err = st_ops->uninit();
>>>> +
>>>> +    return err;
>>>> +}
>>>> +EXPORT_SYMBOL(unregister_bpf_struct_ops);
>>>
>>>
>