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 <[email protected]> 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 <[email protected]> > TODO-NEEDS-IHOR: Signed-off-by: Ihor Solodrai <[email protected]> > Signed-off-by: Jay Wang <[email protected]> [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/[email protected]?part=9
