[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <c4427a57-aea9-4acc-a6be-e30cfb1dbaad@gmail.com>
Date: Wed, 1 Nov 2023 17:59:30 -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 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.
>>
>
Powered by blists - more mailing lists