On Tue, Jul 1, 2025 at 1:54 PM Viktor Malik <vmalik@xxxxxxxxxx> wrote: > > On 7/1/25 22:28, Andrii Nakryiko wrote: > > On Tue, Jul 1, 2025 at 12:50 PM Alexei Starovoitov > > <alexei.starovoitov@xxxxxxxxx> wrote: > >> > >> On Tue, Jul 1, 2025 at 12:43 PM Viktor Malik <vmalik@xxxxxxxxxx> wrote: > >>> > >>> On 7/1/25 19:46, Alexei Starovoitov wrote: > >>>> On Mon, Jun 30, 2025 at 6:35 AM Viktor Malik <vmalik@xxxxxxxxxx> wrote: > >>>>> > >>>>> BPF selftests compilation fails on systems with CONFIG_NET_SCH_BPF=n. > >>>>> The reason is that qdisc-related kfuncs are included via vmlinux.h but > >>>>> when qdisc is disabled, they are not defined and do not appear in > >>>>> vmlinux.h. > >>>> > >>>> Yes and that's expected behavior. It's not a bug. > >>>> That's why we have CONFIG_NET_SCH_BPF=y in > >>>> selftests/bpf/config > >>>> and CI picks it up automatically. > >>>> > >>>> If we add these kfuncs to bpf_qdisc_common.h where would we > >>>> draw the line when the kfuncs should be added or not ? > >>> > >>> I'd say that we should add kfuncs which are only included in vmlinux.h > >>> under certain configurations. Obviously stuff like CONFIG_BPF=y can be > >>> presumed but there're tons of configs options which may be disabled on a > >>> system and it still makes sense to compile and run at least a part of > >>> test_progs on them. > >>> > >>>> Currently we don't add any new kfuncs, since they all > >>>> should be in vmlinux.h > >>> > >>> This way, we're preventing people to build and therefore run *any* > >>> test_progs on systems which do not have all the configs required in > >>> selftests/bpf/config. Running selftests on such systems may reveal bugs > >>> not captured by the CI so I think that it may be eventually beneficial > >>> for everyone. > >> > >> Not quite. What's stopping people to build selftests > >> with 'make -k' ? > >> Some bpf progs will not compile, but test_progs binary will be built and > >> it will run the rest of the tests. > > I don't think test_progs will be built if some of the objects from > progs/ do not build. I just tried to run `make -k` on a kernel with > CONFIG_NET_SCH_BPF=n. The compilation finished, some test binaries > exist, but not test_progs. > > In addition, we generally don't want to ignore all the errors as some of > them may be important. > > >> > >> We can take this patch, but let's define the rules for adding > >> kfuncs explicitly. > > > > Note, we have a VMLINUX_H argument that can be passed into BPF > > selftests' makefile. We used to use this for libbpf CI to build latest > > selftests against (very) old kernels, and it worked well. > > > > I don't think we need to make exceptions for a few kfuncs, all it > > takes is to have vmlinux.h generated from kernel image built from > > proper configuration. > > > > Also note, that "proper configuration" only applies to *built* kernel, > > not the actually running host kernel. See how VMLINUX_BTF_PATHS is > > defined and handled: host kernel is the last thing we use for > > vmlinux.h generation, only if all other options are unavailable. > > This is a good point but the problem here is the extra kernel build. If > you want to check that BPF in your kernel is working properly, you don't > want to do another kernel build with a different config just for the > sake of being able to build selftests. What exactly is problematic? That's what I and others do all the time. If kernel build time is a concern, then pre-generate/pre-package vmlinux.h separately and use it to avoid building the kernel. (but BPF selftest *expects* kernel to be built first, we also build bpf_testmod against that kernel). Or just build/package test_progs itself, if that's what works better. Basically, we have that selftest/bpf/config file for a reason: so that we don't guard every single thing that might not build or work properly if some of the Kconfig value is not set. > > >> What are you proposing exactly ? > >> Anything that is gated by some CONFIG_FOO _must_ be added explicitly ? > >> Assuming we won't be going back and retroactively adding them ? > > Yes, exactly like that. Except for this qdisc one, we haven't run into we should be getting rid of all those __ksym __weak kfunc redefinitions because they now should come from vmlinux.h, not add more of that, IMO. > any issues for a long time, so I don't think that it's necessary to > retroactively add the kfuncs. > > But if you prefer to have it unified, I can take some time to clean it > up - add config-gated kfuncs where they are missing and replace > universally-available kfuncs by vmlinux.h. >
