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

[Yonghong Song <yhs@fb.com>]

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?

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