Re: [PATCH bpf-next] bpf: net: emit anonymous enum with BPF_TCP_CLOSE value explicitly

[Yonghong Song <yhs@fb.com>]

On 3/15/21 5:18 PM, Yonghong Song wrote:
> 
> 
> On 3/15/21 4:33 PM, Daniel Borkmann wrote:
>> On 3/14/21 4:58 AM, Yonghong Song wrote:
>> [...]
>>> This patch explicited add an expression like
>>>    (void)BPF_TCP_ESTABLISHED
>>> to enable generation of debuginfo for the anonymous
>>> enum which also includes BPF_TCP_CLOSE. I put
>>> this explicit type generation in kernel/bpf/core.c
>>> to (1) avoid polute net/ipv4/tcp.c and more importantly
>>> (2) provide a central place to add other types (e.g. in
>>> bpf/btf uapi header) if they are not referenced in the kernel
>>> or generated in vmlinux dwarf.
>>>
>>> Signed-off-by: Yonghong Song <yhs@fb.com>
>>> ---
>>>   include/linux/btf.h |  1 +
>>>   kernel/bpf/core.c   | 19 +++++++++++++++++++
>>>   2 files changed, 20 insertions(+)
>>>
>>> diff --git a/include/linux/btf.h b/include/linux/btf.h
>>> index 7fabf1428093..9c1b52738bbe 100644
>>> --- a/include/linux/btf.h
>>> +++ b/include/linux/btf.h
>>> @@ -9,6 +9,7 @@
>>>   #include <uapi/linux/bpf.h>
>>>   #define BTF_TYPE_EMIT(type) ((void)(type *)0)
>>> +#define BTF_TYPE_EMIT_ENUM(enum_val) ((void)enum_val)
>>>   struct btf;
>>>   struct btf_member;
>>> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
>>> index 3a283bf97f2f..60551bf68ece 100644
>>> --- a/kernel/bpf/core.c
>>> +++ b/kernel/bpf/core.c
>>> @@ -2378,3 +2378,22 @@ EXPORT_SYMBOL(bpf_stats_enabled_key);
>>>   EXPORT_TRACEPOINT_SYMBOL_GPL(xdp_exception);
>>>   EXPORT_TRACEPOINT_SYMBOL_GPL(xdp_bulk_tx);
>>> +
>>> +static int __init bpf_emit_btf_type(void)
>>> +{
>>> +    /* bpf uapi header bpf.h defines an anonymous enum with values
>>> +     * BPF_TCP_* used by bpf programs. Currently gcc built vmlinux
>>> +     * is able to emit this enum in dwarf due to the following
>>> +     * BUILD_BUG_ON test in net/ipv4/tcp.c:
>>> +     *   BUILD_BUG_ON((int)BPF_TCP_ESTABLISHED != 
>>> (int)TCP_ESTABLISHED);
>>> +     * clang built vmlinux does not have this enum in dwarf
>>> +     * since clang removes the above code before generating 
>>> IR/debuginfo.
>>> +     * Let us explicitly emit the type debuginfo to ensure the
>>> +     * above-mentioned anonymous enum in the vmlinux dwarf and hence 
>>> BTF
>>> +     * regardless of which compiler is used.
>>> +     */
>>> +    BTF_TYPE_EMIT_ENUM(BPF_TCP_ESTABLISHED);
>>> +
>>> +    return 0;
>>> +}
>>> +late_initcall(bpf_emit_btf_type);
>>
>> Does this have to be late_initcall() given this adds minor init call
>> overhead, what if this would be exported as symbol for modules instead?

You mean EXPORT_SYMBOL, right? I think it should work, just one extra
unused empty global function, but a little bit misleading since the 
function is not intended to use by anybody. I am using init call and
wants to remove this function after init...

> 
> If issuing types in module, if I understand correctly, it will not be in
> main vmlinux btf, so programs will not be able to use unless module
> is loaded. I would prefer such types always available in vmlinux btf.
> 
> I am using a separate late_initcall just to cleaner codes. But
> this BTF_TYPE_EMIT_ENUM can be in any init call.
> 
> $ grep _initcall *.c
> btf.c:fs_initcall(btf_module_init);
> core.c:pure_initcall(bpf_jit_charge_init);
> cpumap.c:subsys_initcall(cpu_map_init);
> devmap.c:subsys_initcall(dev_map_init);
> inode.c:fs_initcall(bpf_init);
> map_iter.c:late_initcall(bpf_map_iter_init);
> net_namespace.c:subsys_initcall(netns_bpf_init);
> prog_iter.c:late_initcall(bpf_prog_iter_init);
> stackmap.c:subsys_initcall(stack_map_init);
> sysfs_btf.c:subsys_initcall(btf_vmlinux_init);
> task_iter.c:late_initcall(task_iter_init);
> trampoline.c:late_initcall(init_trampolines);
> $
> 
> I think we can use any above in kernel/bpf directory. This way,
> we will have 0 runtime overhead as the code will be optimized away.
> Any preference?
> 
>>
>> Thanks,
>> Daniel