Re: [PATCH bpf-next 09/13] bpf: keep the vmlinux BTF zstd-compressed in the kernel image

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] bpf: non-standard multi-line comment style in btf_free()
- [Low] bpf: unresolved TODO-NEEDS-IHOR placeholder before Signed-off-by tag
--

commit 569512ffc44983e01a69cb1de127bbcd4ca67c62
Author: Jay Wang <wanjay@amazon.com>

bpf: keep the vmlinux BTF zstd-compressed in the kernel image

This commit adds CONFIG_DEBUG_INFO_BTF_LAZY to store the vmlinux BTF
zstd-compressed in the kernel image and decompress it on first access via
btf_vmlinux_data().

>     bpf: keep the vmlinux BTF zstd-compressed in the kernel image
>     
>     The vmlinux BTF is 5-7 MB of read-only data in every kernel built with
>     CONFIG_DEBUG_INFO_BTF=y, resident whether or not anything on the system
>     ever uses BTF.  Add CONFIG_DEBUG_INFO_BTF_LAZY to keep it compressed
>     with zstd in the image instead, at about a quarter of the size, and
>     decompress it in the kernel when it is first needed.  This is the
>     transport half of the option; the next two patches defer the
>     parsing.
>     
>     kbuild: gen_btf_o() in scripts/gen-btf.sh compresses the final
>     ${ELF}.BTF with zstd -19 and adds it as a second section, .init.BTF.zst,
>     of the same .btf.o, so neither an extra link pass nor a size
>     reservation is needed.  .BTF stays in the vmlinux ELF file as a
>     non-alloc section: pahole --btf_base, resolve_btfids for modules,
>     bpftool and libbpf's on-disk fallback find it by name and keep working
>     unchanged, and so do out-of-tree module builds.  The linker script
>     puts .init.BTF.zst in INIT_DATA, bounded by __start_BTF_zst and
>     __stop_BTF_zst, and .BTF in ELF_DETAILS at address 0.  The BTF macro
>     is renamed RO_BTF, because the token BTF in ".BTF 0 : { ... }" would
>     otherwise be macro-expanded.  x86 strips .BTF from the ELF it puts in
>     the compressed boot image, and arm64 makes its Image with objcopy
>     -O binary, which drops non-alloc sections.  The option is limited to
>     these two architectures for now: several others build their boot
>     image from a stripped ELF copy of vmlinux, which keeps non-alloc
>     sections and would carry the uncompressed BTF.  The inline BTF
>     (CONFIG_DEBUG_INFO_BTF_INLINE) is left as it is: built in, .BTF.inline
>     stays in the image; as a module it is not in memory anyway.
>     
>     kernel: btf_vmlinux_data() returns the raw BTF.  With the option it
>     decompresses the blob once, under a leaf mutex, into vmalloc_user()
>     memory that is never freed; the size comes from the zstd frame header,
>     so /sys/kernel/btf/vmlinux gets its size without decompressing.  A
>     late_initcall_sync copies the blob out of init memory before that is
>     freed, unless it is decompressed by then, and the copy is freed after
>     decompression, so a system that uses BTF ends up with the same
>     footprint as today.  The copy uses vmalloc(): kvmemdup() of the
>     ~1.4 MB blob would get a 2 MB kmalloc_large() allocation.  If the copy
>     fails, the BTF is unavailable for the rest of the boot: that is logged
>     once, and later fetches fail with -ENOMEM right away instead of
>     allocating the raw buffer for nothing each time.  A blob that does not
>     decompress is logged once and dropped, and fetches fail with -EINVAL
>     from then on.  btf_parse_vmlinux() and the read and mmap handlers of
>     /sys/kernel/btf/vmlinux use btf_vmlinux_data(); mmap of the
>     decompressed copy uses remap_vmalloc_range().  Without the option
>     btf_vmlinux_data() returns __start_BTF, and the sysfs file keeps its
>     current handlers.
>     
>     btf_vmlinux_data() lives in sysfs_btf.c, which is now built whenever
>     CONFIG_DEBUG_INFO_BTF is enabled, with the sysfs file itself still
>     under CONFIG_SYSFS: btf_parse_vmlinux() needs the data without sysfs
>     too, and with the option the blob has to be copied out of init memory
>     either way.
>     
>     Reading /sys/kernel/btf/vmlinux decompresses but never parses, and
>     only vmalloc and zstd run under btf_vmlinux_data_lock, so the sysfs
>     mmap path (mmap_lock -> kernfs -> btf_vmlinux_data_lock) cannot form
>     a cycle with module or cand_cache locks.
>     
>     On its own this patch only changes where the bytes come from: initcall
>     kfunc registrations still parse the vmlinux BTF at boot.
>     
>     Measured with a distribution config: the raw .BTF is 5.47 MB on x86_64
>     and 5.57 MB on arm64, and 1.44 and 1.46 MB with zstd -19.  xz -9e would
>     save another 0.17 MB but decodes about 6 times slower (72 vs 12 ms in
>     user space).  Decompression in the kernel takes ~7 ms on x86_64.
>     
>     Trade-offs: the compressed blob stays resident on systems that never
>     use BTF (~1.4 MB), so with the next two patches they save about 80% of
>     what the BTF costs rather than all of it (~6 of ~7.5 MB, the raw BTF
>     plus what parsing it allocates); the x86 bzImage on disk is ~0.3 MB
>     smaller; building needs the zstd tool; __start_BTF/__stop_BTF do not
>     exist with the option (the ksyms and linked_vars selftests use them; a
>     later patch adapts them).
>     
>     Co-developed-by: Ihor Solodrai <ihor.solodrai@linux.dev>
>     TODO-NEEDS-IHOR: Signed-off-by: Ihor Solodrai <ihor.solodrai@linux.dev>
>     Signed-off-by: Jay Wang <wanjay@amazon.com>

[Severity: Low]
This isn't a bug in the code, but the commit message still has an
unresolved TODO-NEEDS-IHOR prefix on the Signed-off-by trailer pairing with
Co-developed-by. Should this placeholder be resolved to a clean
Signed-off-by tag?

[ ... ]

> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 70adc52f5d7c7..a7bb780743fb2 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -1901,7 +1901,8 @@ static void btf_free(struct btf *btf)
>  	kvfree(btf->resolved_sizes);
>  	kvfree(btf->resolved_ids);
>  	/* vmlinux does not allocate btf->data, it simply points it at
> -	 * __start_BTF.
> +	 * __start_BTF, or at the decompressed copy with
> +	 * CONFIG_DEBUG_INFO_BTF_LAZY, which is never freed either.
>  	 */

[Severity: Low]
This isn't a bug, but when updating this multi-line comment in btf_free()
in kernel/bpf/btf.c, could the opening /* be placed on its own line to
follow the BPF coding style?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031130959.23472-1-wanjay@amazon.com?part=9