On Thu, Jul 18, 2024 at 3:42 PM Ihor Solodrai <ihor.solodrai@xxxxx> wrote: > > On Thursday, July 18th, 2024 at 8:34 AM, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > [...] > > > > > > - by adding a catch-all clause in the makefile, e.g. making test > > > > > runner depend on all .bpf.o files. > > > > > > > > do we actually need to rebuild final binary if we are still just > > > > loading .bpf.o from disk? We are not embedding such .bpf.o (embedding > > > > is what skeleton headers are adding), so why rebuild .bpf.o? > > > > > > > > Actually thinking about this again, I guess, if we don't want to add > > > > skel.h to track precise dependencies, we don't really need to do > > > > anything extra for those progs/*.c files that are not used through > > > > skeletons. We just need to make sure that they are rebuilt if they are > > > > changed. The rest will work as is because test runner binary will just > > > > load them from disk at the next run (and user space part doesn't have > > > > to be rebuilt, unless it itself changed). > > > > > > Good point. This can be achieved by making $(OUTPUT)/$(TRUNNER_BINARY) > > > dependency on $(TRUNNER_BPF_OBJS) order-only, e.g. here is a modified > > > version of the v2: https://tinyurl.com/4wnhkt32 > > > > > > +1 > > I agree. I'll submit v4 with this change. > > > > > [...] > > > > > > > another side benefit of completely switching to .skel.h is that we can > > > > stop copying all .bpf.o files into BPF CI, because test_progs will be > > > > self-contained (thought that's not 100% true due to btf__* and maybe a > > > > few files more, which is sad and a bit different problem) > > > > > > Hm, this might make sense. > > > There are 410Mb of .bpf.o files generated currently. > > > On the other hand, as you note, one would still need a list of some > > > .bpf.o files, because there are at-least several tests that verify > > > operation on ELF files, not ELF bytes. > > This seems worthwhile to look into, although I think it's a task > independent of this patch. > > > > > [...] > > > > > > > keep in mind that we do want to rebuild .bpf.o if libbpf's BPF-side > > > > headers changed, so let's make sure that stays (or happens, if we > > > > don't do it already) > > > > > > Commands below cause full rebuild (.test.o, .bpf.o) on v2 of this > > > patch-set: > > > $ touch tools/lib/bpf/bpf.h > > > $ touch tools/lib/bpf/libbpf.h > > > > > > yeah, ideally they wouldn't cause bpf.o rebuilds... I think we should > > tune .bpf.o to depend only on BPF-side headers (we'd need to hard-code > > them, but they don't change often: usdt.bpf.h, bpf_tracing.h, > > bpf_helpers.h, etc). I don't think we can get rid of BPF skeletons' > > dependency on bpftool (which depends on any libbpf change), though, > > so .skel.h will be regenerated due to any tiny libbpf change, but > > that's still better, as bpf.o building is probably the slowest part. > > I tried a small experiment, and specifying particular lib/bpf headers > didn't help because of vmlinux.h > > I grepped the list of headers with: > > $ grep -rh 'include <bpf/' progs | sort -u > > #include <bpf/bpf_core_read.h> > #include <bpf/bpf_endian.h> > #include <bpf/bpf_helpers.h> > #include <bpf/bpf_tracing.h> > #include <bpf/usdt.bpf.h> > > Then, changed $(TRUNNER_BPF_OBJS) dependencies like this: > > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile > index 66478446af9d..6fb03bb9b33a 100644 > --- a/tools/testing/selftests/bpf/Makefile > +++ b/tools/testing/selftests/bpf/Makefile > @@ -480,6 +480,13 @@ xdp_features.skel.h-deps := xdp_features.bpf.o > LINKED_BPF_OBJS := $(foreach skel,$(LINKED_SKELS),$($(skel)-deps)) > LINKED_BPF_SRCS := $(patsubst %.bpf.o,%.c,$(LINKED_BPF_OBJS)) > > +HEADERS_FOR_BPF_OBJS := bpf_core_read.h \ > + bpf_endian.h \ > + bpf_helpers.h \ > + bpf_tracing.h \ > + usdt.bpf.h > +HEADERS_FOR_BPF_OBJS := $(addprefix $(BPFDIR)/,$(HEADERS_FOR_BPF_OBJS)) > + > # Set up extra TRUNNER_XXX "temporary" variables in the environment (relies on > # $eval()) and pass control to DEFINE_TEST_RUNNER_RULES. > # Parameters: > @@ -530,14 +537,15 @@ $(TRUNNER_BPF_OBJS): $(TRUNNER_OUTPUT)/%.bpf.o: \ > $(TRUNNER_BPF_PROGS_DIR)/%.c \ > $(TRUNNER_BPF_PROGS_DIR)/*.h \ > $$(INCLUDE_DIR)/vmlinux.h \ > - $(wildcard $(BPFDIR)/bpf_*.h) \ > - $(wildcard $(BPFDIR)/*.bpf.h) \ > + $(HEADERS_FOR_BPF_OBJS) \ I'd leave *.bpf.h as is, it's meant to be BPF-only header (going forward, at least) > | $(TRUNNER_OUTPUT) $$(BPFOBJ) > $$(call $(TRUNNER_BPF_BUILD_RULE),$$<,$$@, \ > $(TRUNNER_BPF_CFLAGS) \ > $$($$<-CFLAGS) \ > $$($$<-$2-CFLAGS)) > > This didn't help because > > $ touch ~/work/kernel-clean/tools/lib/bpf/bpf.h > > triggers rebuild of vmlinux.h, which depends on $(BPFTOOL), and > bpftool depends on $(HOST_BPFOBJ) or $(BPFOBJ), and they in turn > depend on all files in lib/bpf. > > And there is a direct dependency of $(TRUNNER_BPF_OBJS) on vmlinux.h, > which looks like a real dependency to me, but maybe I don't know > something. > Yeah, we need to be smarter with vmlinux.h, I think. I think dependencies are set up correct, though pessimistic. Any libbpf change can cause bpftool change, which can cause different vmlinux.h generation. All that probably has to stay. But we can generate temporary vmlinux.h on the side, and compare it with pre-existing vmlinux.h. If contents didn't change (and we shouldn't have any timestamps in vmlinux.h or anything that just changes from run to run), we shouldn't touch the original vmlinux.h. This way will avoid .bpf.o regeneration. (vmlinux.h generation itself is fast, so I wouldn't bother trying to avoid it)
