Re: [PATCH bpf-next v8 07/10] bpf, net: switch to dynamic registration

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

On 11/1/23 17:17, Martin KaFai Lau wrote:
> On 10/31/23 5:19 PM, Kui-Feng Lee wrote:
>>
>>
>> On 10/31/23 17:02, Martin KaFai Lau wrote:
>>> On 10/31/23 4:34 PM, Kui-Feng Lee wrote:
>>>>>> diff --git a/include/linux/btf.h b/include/linux/btf.h
>>>>>> index a8813605f2f6..954536431e0b 100644
>>>>>> --- a/include/linux/btf.h
>>>>>> +++ b/include/linux/btf.h
>>>>>> @@ -12,6 +12,8 @@
>>>>>>   #include <uapi/linux/bpf.h>
>>>>>>   #define BTF_TYPE_EMIT(type) ((void)(type *)0)
>>>>>> +#define BTF_STRUCT_OPS_TYPE_EMIT(type) {((void)(struct type 
>>>>>> *)0);    \
>>>>>
>>>>> ((void)(struct type *)0); is new. Why is it needed?
>>>>
>>>> This is a trick of BTF to force compiler generate type info for
>>>> the given type. Without trick, compiler may skip these types if these
>>>> type are not used at all in the module.  For example, modules usually
>>>> don't use value types of struct_ops directly.
>>> It is not the value type and value type emit is understood. It is the 
>>> struct_ops type itself and it is new addition in this patchset 
>>> afaict. The value type emit is in the next line which was cut out 
>>> from the context here.
>>>
>> I mean both of them are required.
>> In the case of a dummy implementation, struct_ops type itself properly 
>> never being used, only being declared by the module. Without this line,
> 
> Other than bpf_dummy_ops, after reg(), the struct_ops->func() must be 
> used somewhere in the kernel or module. Like tcp must be using the 
> tcp_congestion_ops after reg(). bpf_dummy_ops is very special and 
> probably should be moved out to bpf_testmod somehow but this is for 
> later. Even bpf_dummy_ops does not have an issue now. Why it is needed 
> after the kmod support change?
> 
> or it is a preemptive addition to be future proof only?
> 
> Addition is fine if it is required to work. I am trying to understand 
> why this new addition is needed after the kmod support change. The 
> reason why this is needed after the kmod support change is not obvious 
> from looking at the code. The commit message didn't mention why and what 
> broke after this kmod change. If someone wants to clean it up a few 
> months later, we will need to figure out why it was added in the first 
> place.


It is a future proof.
What do you think if I add a comment in the code?

> 
> 
>> the module developer will fail to load a struct_ops map of the dummy
>> type. This line is added to avoid this awful situation.
>>
>