| Message ID | 20220426155229.436681-1-mailhol.vincent@wanadoo.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] kbuild: call checksyscalls.sh and check-atomics.sh only if prerequisites change | expand |
On Wed, Apr 27, 2022 at 12:52 AM Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote: > > Currently, checksyscalls.sh and check-atomics.sh are executed > unconditionally. Most developers will not modify the files being > checked by those scripts and thus do not need to execute these again > for each iterative make. Change Kbuild target so that those two > scripts get executed only if the prerequisite are modified. > > In order to implement this we: > > 1. use the if_change macro instead of cmd. c.f. [1] > > 2. create two dot files: scripts/.checksyscalls and > scripts/atomic/.check-atomics to keep track of whether the script > were already executed or not. Otherwise, the prerequisite would > always be considered as newer than the target (c.f. output "due to > target missing" of make V=2). > > 3. modify the CLEAN_FILES target of the root Makefile to removed the > two temporary dot files created in 2. > > We also added an additional dependency to include/linux/atomic/* for > check-atomics.sh to make sure that the script gets executed again if > the header are modified. check-atomics.sh already has a dependency > toward include/generated/asm-offsets.h and so no additional > dependencies were added. > > [1] https://www.kernel.org/doc/html/latest/kbuild/makefiles.html#command-change-detection > > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > --- > Sending this as RFC because I am not an expert of Kbuild. The use of > the dot files was my best shot at tackling this issue. Maybe there is > a smarter way which I just missed? > > If I receive no comments for the next two weeks, I will resend this > patch without the RFC tag. > --- > Kbuild | 14 ++++++++------ > Makefile | 3 ++- > 2 files changed, 10 insertions(+), 7 deletions(-) > > diff --git a/Kbuild b/Kbuild > index fa441b98c9f6..d579f4971aa3 100644 > --- a/Kbuild > +++ b/Kbuild > @@ -39,21 +39,23 @@ $(offsets-file): arch/$(SRCARCH)/kernel/asm-offsets.s FORCE > ##### > # Check for missing system calls > > -always-y += missing-syscalls > +always-y += scripts/.missing-syscalls > > quiet_cmd_syscalls = CALL $< > cmd_syscalls = $(CONFIG_SHELL) $< $(CC) $(c_flags) $(missing_syscalls_flags) > > -missing-syscalls: scripts/checksyscalls.sh $(offsets-file) FORCE > - $(call cmd,syscalls) > +scripts/.missing-syscalls: scripts/checksyscalls.sh $(offsets-file) FORCE > + $(call if_changed,syscalls) > + @touch $@ I am not sure about this hunk. Avoiding the needless preprocess is good. But, it is better to display a warning somewhere (maybe 'all' target) until the required syscall is implemented. Also, you need to check the timestamp of syscall_32.tbl. When it is updated (i.e. when a new syscall is added), this check must be re-run. > ##### > # Check atomic headers are up-to-date > > -always-y += old-atomics > +always-y += scripts/atomic/.old-atomics > > quiet_cmd_atomics = CALL $< > cmd_atomics = $(CONFIG_SHELL) $< > > -old-atomics: scripts/atomic/check-atomics.sh FORCE > - $(call cmd,atomics) > +scripts/atomic/.old-atomics: scripts/atomic/check-atomics.sh $(wildcard include/linux/atomic/*) FORCE > + $(call if_changed,atomics) > + @touch $@ Presumably, this is wrong. If the header is manually edited, Kbuild must stop the build. This change just lets it keep going, and what is worse, the warning is completely silenced second time. > diff --git a/Makefile b/Makefile > index fa5112a0ec1b..b18af9d4248a 100644 > --- a/Makefile > +++ b/Makefile > @@ -1483,7 +1483,8 @@ endif # CONFIG_MODULES > # Directories & files removed with 'make clean' > CLEAN_FILES += include/ksym vmlinux.symvers modules-only.symvers \ > modules.builtin modules.builtin.modinfo modules.nsdeps \ > - compile_commands.json .thinlto-cache > + compile_commands.json .thinlto-cache \ > + scripts/.missing-syscalls scripts/atomic/.old-atomics > > # Directories & files removed with 'make mrproper' > MRPROPER_FILES += include/config include/generated \ > -- > 2.35.1 >
On Sat. 30 Apr 2022 at 02:11, Masahiro Yamada <masahiroy@kernel.org> wrote: > On Wed, Apr 27, 2022 at 12:52 AM Vincent Mailhol > <mailhol.vincent@wanadoo.fr> wrote: > > > > Currently, checksyscalls.sh and check-atomics.sh are executed > > unconditionally. Most developers will not modify the files being > > checked by those scripts and thus do not need to execute these again > > for each iterative make. Change Kbuild target so that those two > > scripts get executed only if the prerequisite are modified. > > > > In order to implement this we: > > > > 1. use the if_change macro instead of cmd. c.f. [1] > > > > 2. create two dot files: scripts/.checksyscalls and > > scripts/atomic/.check-atomics to keep track of whether the script > > were already executed or not. Otherwise, the prerequisite would > > always be considered as newer than the target (c.f. output "due to > > target missing" of make V=2). > > > > 3. modify the CLEAN_FILES target of the root Makefile to removed the > > two temporary dot files created in 2. > > > > We also added an additional dependency to include/linux/atomic/* for > > check-atomics.sh to make sure that the script gets executed again if > > the header are modified. check-atomics.sh already has a dependency > > toward include/generated/asm-offsets.h and so no additional > > dependencies were added. > > > > [1] https://www.kernel.org/doc/html/latest/kbuild/makefiles.html#command-change-detection > > > > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > > --- > > Sending this as RFC because I am not an expert of Kbuild. The use of > > the dot files was my best shot at tackling this issue. Maybe there is > > a smarter way which I just missed? > > > > If I receive no comments for the next two weeks, I will resend this > > patch without the RFC tag. > > --- > > Kbuild | 14 ++++++++------ > > Makefile | 3 ++- > > 2 files changed, 10 insertions(+), 7 deletions(-) > > > > diff --git a/Kbuild b/Kbuild > > index fa441b98c9f6..d579f4971aa3 100644 > > --- a/Kbuild > > +++ b/Kbuild > > @@ -39,21 +39,23 @@ $(offsets-file): arch/$(SRCARCH)/kernel/asm-offsets.s FORCE > > ##### > > # Check for missing system calls > > > > -always-y += missing-syscalls > > +always-y += scripts/.missing-syscalls > > > > quiet_cmd_syscalls = CALL $< > > cmd_syscalls = $(CONFIG_SHELL) $< $(CC) $(c_flags) $(missing_syscalls_flags) > > > > -missing-syscalls: scripts/checksyscalls.sh $(offsets-file) FORCE > > - $(call cmd,syscalls) > > +scripts/.missing-syscalls: scripts/checksyscalls.sh $(offsets-file) FORCE > > + $(call if_changed,syscalls) > > + @touch $@ > > I am not sure about this hunk. > > Avoiding the needless preprocess is good. > But, it is better to display a warning somewhere (maybe 'all' target) > until the required syscall is implemented. The classic way to achieve is to raise an error, not a warning. This should have been achievable through CONFIG_WERROR, however, the below commit deactivated it with -Wno-error (rationale of why this was an issue is missing). commit 20fbb11fe4ea ("don't make the syscall checking produce errors from warnings") If we just warn, I am not sure how to emit a warning each time until it gets fixed. The normal behaviour everywhere else is to only warn during the first build and be quiet (unless dependencies change). > Also, you need to check the timestamp of syscall_32.tbl. > When it is updated (i.e. when a new syscall is added), > this check must be re-run. ACK Regardless of the above discussion, I think I will give up on checksyscalls.sh, at least for the moment. The patch assumed that checksyscalls.sh would only be called from ./Kbuild. It appears that there is another user: arch/mips/Makefile c.f. kernel test robot's report: https://lore.kernel.org/llvm/202205030015.JCmg5yPS-lkp@intel.com/ Because of that, only creating a single empty file to check the timestamp is insufficient and I could not find a smart way to manage all this. > > ##### > > # Check atomic headers are up-to-date > > > > -always-y += old-atomics > > +always-y += scripts/atomic/.old-atomics > > > > quiet_cmd_atomics = CALL $< > > cmd_atomics = $(CONFIG_SHELL) $< > > > > -old-atomics: scripts/atomic/check-atomics.sh FORCE > > - $(call cmd,atomics) > > +scripts/atomic/.old-atomics: scripts/atomic/check-atomics.sh $(wildcard include/linux/atomic/*) FORCE > > + $(call if_changed,atomics) > > + @touch $@ > > > Presumably, this is wrong. > If the header is manually edited, Kbuild must stop the build. Currently (i.e. before my patch), Kbuild does not stop the build either. c.f. check-atomics.sh which returns 0 inconditionally: https://elixir.bootlin.com/linux/v5.17/source/scripts/atomic/check-atomics.sh#L33 This can be easily remediated by making check-atomics.sh return an error code if the check does not succeed. > This change just lets it keep going, and > what is worse, the warning is completely silenced > second time. Same comment as before, I expect warnings to only be raised once, making this an error would solve the issue. I will send a v2 but only for check-atomics.sh Yours sincerely, Vincent Mailhol
diff --git a/Kbuild b/Kbuild index fa441b98c9f6..d579f4971aa3 100644 --- a/Kbuild +++ b/Kbuild @@ -39,21 +39,23 @@ $(offsets-file): arch/$(SRCARCH)/kernel/asm-offsets.s FORCE ##### # Check for missing system calls -always-y += missing-syscalls +always-y += scripts/.missing-syscalls quiet_cmd_syscalls = CALL $< cmd_syscalls = $(CONFIG_SHELL) $< $(CC) $(c_flags) $(missing_syscalls_flags) -missing-syscalls: scripts/checksyscalls.sh $(offsets-file) FORCE - $(call cmd,syscalls) +scripts/.missing-syscalls: scripts/checksyscalls.sh $(offsets-file) FORCE + $(call if_changed,syscalls) + @touch $@ ##### # Check atomic headers are up-to-date -always-y += old-atomics +always-y += scripts/atomic/.old-atomics quiet_cmd_atomics = CALL $< cmd_atomics = $(CONFIG_SHELL) $< -old-atomics: scripts/atomic/check-atomics.sh FORCE - $(call cmd,atomics) +scripts/atomic/.old-atomics: scripts/atomic/check-atomics.sh $(wildcard include/linux/atomic/*) FORCE + $(call if_changed,atomics) + @touch $@ diff --git a/Makefile b/Makefile index fa5112a0ec1b..b18af9d4248a 100644 --- a/Makefile +++ b/Makefile @@ -1483,7 +1483,8 @@ endif # CONFIG_MODULES # Directories & files removed with 'make clean' CLEAN_FILES += include/ksym vmlinux.symvers modules-only.symvers \ modules.builtin modules.builtin.modinfo modules.nsdeps \ - compile_commands.json .thinlto-cache + compile_commands.json .thinlto-cache \ + scripts/.missing-syscalls scripts/atomic/.old-atomics # Directories & files removed with 'make mrproper' MRPROPER_FILES += include/config include/generated \
Currently, checksyscalls.sh and check-atomics.sh are executed unconditionally. Most developers will not modify the files being checked by those scripts and thus do not need to execute these again for each iterative make. Change Kbuild target so that those two scripts get executed only if the prerequisite are modified. In order to implement this we: 1. use the if_change macro instead of cmd. c.f. [1] 2. create two dot files: scripts/.checksyscalls and scripts/atomic/.check-atomics to keep track of whether the script were already executed or not. Otherwise, the prerequisite would always be considered as newer than the target (c.f. output "due to target missing" of make V=2). 3. modify the CLEAN_FILES target of the root Makefile to removed the two temporary dot files created in 2. We also added an additional dependency to include/linux/atomic/* for check-atomics.sh to make sure that the script gets executed again if the header are modified. check-atomics.sh already has a dependency toward include/generated/asm-offsets.h and so no additional dependencies were added. [1] https://www.kernel.org/doc/html/latest/kbuild/makefiles.html#command-change-detection Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> --- Sending this as RFC because I am not an expert of Kbuild. The use of the dot files was my best shot at tackling this issue. Maybe there is a smarter way which I just missed? If I receive no comments for the next two weeks, I will resend this patch without the RFC tag. --- Kbuild | 14 ++++++++------ Makefile | 3 ++- 2 files changed, 10 insertions(+), 7 deletions(-)