Message-ID: <4a7d22ed-e34f-4e75-a796-8d2744b6c62e@gmail.com>
Date: Wed, 1 Nov 2023 21:19:38 -0700
From: Kui-Feng Lee <sinquersw@...il.com>
To: Martin KaFai Lau <martin.lau@...ux.dev>
Cc: kuifeng@...a.com, netdev@...r.kernel.org, bpf@...r.kernel.org,
 ast@...nel.org, song@...nel.org, kernel-team@...a.com, andrii@...nel.org,
 thinker.li@...il.com, drosen@...gle.com
Subject: Re: [PATCH bpf-next v8 07/10] bpf, net: switch to dynamic
 registration



On 11/1/23 18:32, Martin KaFai Lau wrote:
> On 11/1/23 5:59 PM, Kui-Feng Lee wrote:
>>
>>
>> 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?
> 
> If it is not required to work, I prefer not adding it to avoid confusion 
> and avoid future cleanup temptation. Even the artificial bpf_dummy_ops 
> does not need it, so not enough reason to introduce this code redundancy.

Got it!

> 
> Switch topic.
> While we are on a new macro topic, I think a new macro will be useful to 
> emit the value type and register_bpf_struct_ops together. wdyt?

Like this?

#define REGISTER_STRUCT_OPS(st_type, st_ops) { \
         BTF_STRUCT_OPS_TYPE_EMIT(st_type);  \
         register_bpf_struct_ops(st_ops); } while(0)

static int bpf_testmod_init(void) {
     ....
     REGISTER_STRUCT_OPS(bpf_testmod_ops, &bpf_bpf_testmod_ops);
     ....
}

or you like something more aggressive

#define REGISTER_STRUCT_OPS(st_type) { \
         BTF_STRUCT_OPS_TYPE_EMIT(st_type);  \
         register_bpf_struct_ops(&bpf_##st_type); } while(0)




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

Powered by blists - more mailing lists