On Mon, Aug 26, 2024 at 6:22 PM Ihor Solodrai <ihor.solodrai@xxxxx> wrote: > > Hi Andrii, thanks for a review. > > On Monday, August 26th, 2024 at 2:59 PM, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > [...] > > > I'm not sure what md5sum buys us here, tbh... To compute checksum you > > need to read entire contents anyways, so you are not really saving > > anything performance-wise. > > > > I was originally thinking that we'll extend existing rule for > > $(INCLUDE_DIR)/vmlinux.h to do bpftool dump into temporary file, then > > do `cmp --silent` over it and existing vmlinux.h (if it does exist, of > > course), and if they are identical just exit and not modify anything. > > If not, we just mv temp file over destination vmlinux.h. > > > > > In my head this would prevent make from triggering dependent targets > > because vmlinux.h's modification time won't change. > > > > Does the above not work? > > I tried your suggestion and it works too. I like it better, as it's a > smaller change (see below). > > A checksum was just the first idea I had about saving the previous > state of vmlinux.h, and I went with it. Copying an entire file seemed > excessive to me, but it's not necessary as it turns out. > > Please let me know if the cmp version is ok, and I'll send v2 of the > patch. > > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile > index c120617b64ad..25412b9194bd 100644 > --- a/tools/testing/selftests/bpf/Makefile > +++ b/tools/testing/selftests/bpf/Makefile > @@ -402,7 +402,8 @@ endif > $(INCLUDE_DIR)/vmlinux.h: $(VMLINUX_BTF) $(BPFTOOL) | $(INCLUDE_DIR) > ifeq ($(VMLINUX_H),) > $(call msg,GEN,,$@) > - $(Q)$(BPFTOOL) btf dump file $(VMLINUX_BTF) format c > $@ > + $(Q)$(BPFTOOL) btf dump file $(VMLINUX_BTF) format c > $(INCLUDE_DIR)/.vmlinux.h.tmp > + $(Q)cmp -s $(INCLUDE_DIR)/.vmlinux.h.tmp $@ || mv $(INCLUDE_DIR)/.vmlinux.h.tmp $@ great, just maybe leave a small comment that we do this to avoid updating timestamps if contents didn't change, to not trigger expensive recompilation of many dependent files > else > $(call msg,CP,,$@) > $(Q)cp "$(VMLINUX_H)" $@ > @@ -516,6 +517,12 @@ 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 := $(wildcard $(BPFDIR)/*.bpf.h) \ > + $(addprefix $(BPFDIR)/, bpf_core_read.h \ > + bpf_endian.h \ > + bpf_helpers.h \ > + bpf_tracing.h) > + let's split this change into a separate patch? > # Set up extra TRUNNER_XXX "temporary" variables in the environment (relies on > # $eval()) and pass control to DEFINE_TEST_RUNNER_RULES. > # Parameters: > @@ -566,8 +573,7 @@ $(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) \ > | $(TRUNNER_OUTPUT) $$(BPFOBJ) > $$(call $(TRUNNER_BPF_BUILD_RULE),$$<,$$@, \ > $(TRUNNER_BPF_CFLAGS) \ > -- > 2.34.1 > >
