[PATCHv3,5/6] atomics: check generated headers are up-to-date

Message ID 20180904104830.2975-6-mark.rutland@arm.com
State Accepted
Commit 8d32588077bdc390420cfa6946f407033a20d7a8
Headers show
Series
  • atomics: generate atomic headers / instrument arm64
Related show

Commit Message

Mark Rutland Sept. 4, 2018, 10:48 a.m.
Now that all the generated atomic headers are in place, it would be good
to ensure that:

a) the headers are up-to-date when scripting changes.

b) developers don't directly modify the generated headers.

To ensure both of these properties, let's add a Kbuild step to check
that the generated headers are up-to-date.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>

Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Will Deacon <will.deacon@arm.com>
---
 Kbuild                          | 18 ++++++++++++++++--
 scripts/atomic/check-atomics.sh | 19 +++++++++++++++++++
 2 files changed, 35 insertions(+), 2 deletions(-)
 create mode 100755 scripts/atomic/check-atomics.sh

-- 
2.11.0

Comments

Ingo Molnar Nov. 21, 2018, 8:02 a.m. | #1
* tip-bot for Mark Rutland <tipbot@zytor.com> wrote:

> Commit-ID:  8d32588077bdc390420cfa6946f407033a20d7a8

> Gitweb:     https://git.kernel.org/tip/8d32588077bdc390420cfa6946f407033a20d7a8

> Author:     Mark Rutland <mark.rutland@arm.com>

> AuthorDate: Tue, 4 Sep 2018 11:48:29 +0100

> Committer:  Ingo Molnar <mingo@kernel.org>

> CommitDate: Thu, 1 Nov 2018 11:01:10 +0100

> 

> locking/atomics: Check generated headers are up-to-date

> 

> Now that all the generated atomic headers are in place, it would be good

> to ensure that:

> 

> a) the headers are up-to-date when scripting changes.

> 

> b) developers don't directly modify the generated headers.

> 

> To ensure both of these properties, let's add a Kbuild step to check

> that the generated headers are up-to-date.

> 

> Signed-off-by: Mark Rutland <mark.rutland@arm.com>

> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

> Cc: linux-arm-kernel@lists.infradead.org

> Cc: catalin.marinas@arm.com

> Cc: Will Deacon <will.deacon@arm.com>

> Cc: linuxdrivers@attotech.com

> Cc: dvyukov@google.com

> Cc: Boqun Feng <boqun.feng@gmail.com>

> Cc: arnd@arndb.de

> Cc: aryabinin@virtuozzo.com

> Cc: glider@google.com

> Link: http://lkml.kernel.org/r/20180904104830.2975-6-mark.rutland@arm.com

> Signed-off-by: Ingo Molnar <mingo@kernel.org>

> ---

>  Kbuild                          | 18 ++++++++++++++++--

>  scripts/atomic/check-atomics.sh | 19 +++++++++++++++++++

>  2 files changed, 35 insertions(+), 2 deletions(-)


These scripts are *awfully* slow to be run at every kernel build - even a 
reasonably fast machine:

  model name	: Intel(R) Xeon(R) CPU E5-2680 v2 @ 2.80GHz

... they are adding 3-4 seconds to the build time:

[Before]:

  galatea:~/linux/linux> perf stat --null --repeat 3 make kernel/sched/core.o
  CALL    scripts/checksyscalls.sh
  DESCEND  objtool
  CALL    scripts/checksyscalls.sh
  DESCEND  objtool
  CALL    scripts/checksyscalls.sh
  DESCEND  objtool

 Performance counter stats for 'make kernel/sched/core.o' (3 runs):

          1.201874 +- 0.000371 seconds time elapsed  ( +-  0.03% )


[After]:

  galatea:~/linux/linux> perf stat --null --repeat 3 make kernel/sched/core.o

  CALL    scripts/checksyscalls.sh
  CALL    scripts/atomic/check-atomics.sh
  DESCEND  objtool
  CALL    scripts/checksyscalls.sh
  CALL    scripts/atomic/check-atomics.sh
  DESCEND  objtool
  CALL    scripts/checksyscalls.sh
  CALL    scripts/atomic/check-atomics.sh
  DESCEND  objtool

 Performance counter stats for 'make kernel/sched/core.o' (3 runs):

            4.5987 +- 0.0109 seconds time elapsed  ( +-  0.24% )

Could we please get this fixed so that proper dependencies are checked 
and it's only regenerated when needed? This slowdown makes additive-build 
kernel development quite painful, as ~5 seconds is in the 'too long' 
category already, while 1.2 seconds is basically instantaneous.

I cannot even imagine the slowdown on a truly slow box where kernel 
development *has* to be additive.

Thanks,

	Ingo
Mark Rutland Nov. 21, 2018, 11:40 a.m. | #2
Hi Ingo,

On Wed, Nov 21, 2018 at 09:02:58AM +0100, Ingo Molnar wrote:
> 

> * tip-bot for Mark Rutland <tipbot@zytor.com> wrote:

> 

> > Commit-ID:  8d32588077bdc390420cfa6946f407033a20d7a8

> > Gitweb:     https://git.kernel.org/tip/8d32588077bdc390420cfa6946f407033a20d7a8

> > Author:     Mark Rutland <mark.rutland@arm.com>

> > AuthorDate: Tue, 4 Sep 2018 11:48:29 +0100

> > Committer:  Ingo Molnar <mingo@kernel.org>

> > CommitDate: Thu, 1 Nov 2018 11:01:10 +0100

> > 

> > locking/atomics: Check generated headers are up-to-date

> > 

> > Now that all the generated atomic headers are in place, it would be good

> > to ensure that:

> > 

> > a) the headers are up-to-date when scripting changes.

> > 

> > b) developers don't directly modify the generated headers.

> > 

> > To ensure both of these properties, let's add a Kbuild step to check

> > that the generated headers are up-to-date.

> > 

> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>

> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

> > Cc: linux-arm-kernel@lists.infradead.org

> > Cc: catalin.marinas@arm.com

> > Cc: Will Deacon <will.deacon@arm.com>

> > Cc: linuxdrivers@attotech.com

> > Cc: dvyukov@google.com

> > Cc: Boqun Feng <boqun.feng@gmail.com>

> > Cc: arnd@arndb.de

> > Cc: aryabinin@virtuozzo.com

> > Cc: glider@google.com

> > Link: http://lkml.kernel.org/r/20180904104830.2975-6-mark.rutland@arm.com

> > Signed-off-by: Ingo Molnar <mingo@kernel.org>

> > ---

> >  Kbuild                          | 18 ++++++++++++++++--

> >  scripts/atomic/check-atomics.sh | 19 +++++++++++++++++++

> >  2 files changed, 35 insertions(+), 2 deletions(-)

> 

> These scripts are *awfully* slow to be run at every kernel build - even a 

> reasonably fast machine:


Andrew also reported this, and I'm trying to figure out what to do.

I looked into optimizing the scripts, but that turned out to be a dead end.
The majority of the cost is in the fork() etc for sub-shells, which the scripts
make heavy use of to capture string return values. Trying to avoid sub-shells
rendered the scripts unreadable.

> Could we please get this fixed so that proper dependencies are checked 

> and it's only regenerated when needed? This slowdown makes additive-build 

> kernel development quite painful, as ~5 seconds is in the 'too long' 

> category already, while 1.2 seconds is basically instantaneous.


Just to check, are we happy to eat the full cost for the first build of a
pristine tree?

One reason we do the check rather than (re-)generating the headers is that
Linus requested [1] the generated header be committed so that they show up in
git grep, but it looks like he was happy to be flexible on that.

If we're happy to not commit in the generated headers, and if we're happy to
pay the cost for a pristine tree, that's fairly straightforward to do.
Otherwise, this has to be an optional check.

Thoughts?

Thanks,
Mark.

[1] https://lkml.kernel.org/r/CA+55aFxjU0op8QLLu0n-RjHBs7gQsLvD8jcyedgw6jeZFN7B+Q@mail.gmail.com
Ingo Molnar Nov. 28, 2018, 8:30 a.m. | #3
* Mark Rutland <mark.rutland@arm.com> wrote:

> > Could we please get this fixed so that proper dependencies are checked 

> > and it's only regenerated when needed? This slowdown makes additive-build 

> > kernel development quite painful, as ~5 seconds is in the 'too long' 

> > category already, while 1.2 seconds is basically instantaneous.

> 

> Just to check, are we happy to eat the full cost for the first build of a

> pristine tree?


No, not happy to add 3-4 seconds to a full build that usually takes less 
than 60 seconds. This stuff isn't parallelized nor particularly well 
optimized it appears.

This *must* get faster.

> One reason we do the check rather than (re-)generating the headers is 

> that Linus requested [1] the generated header be committed so that they 

> show up in git grep, but it looks like he was happy to be flexible on 

> that.


I think the generated headers should be part of the commit space, the 
grepping is important.

> If we're happy to not commit in the generated headers, and if we're happy to

> pay the cost for a pristine tree, that's fairly straightforward to do.

> Otherwise, this has to be an optional check.


Or faster code, or a different concept!

Thanks,

	Ingo

Patch

diff --git a/Kbuild b/Kbuild
index 005304205482..47c9fe175bd9 100644
--- a/Kbuild
+++ b/Kbuild
@@ -6,7 +6,8 @@ 
 # 2) Generate timeconst.h
 # 3) Generate asm-offsets.h (may need bounds.h and timeconst.h)
 # 4) Check for missing system calls
-# 5) Generate constants.py (may need bounds.h)
+# 5) check atomics headers are up-to-date
+# 6) Generate constants.py (may need bounds.h)
 
 #####
 # 1) Generate bounds.h
@@ -72,7 +73,20 @@  missing-syscalls: scripts/checksyscalls.sh $(offsets-file) FORCE
 	$(call cmd,syscalls)
 
 #####
-# 5) Generate constants for Python GDB integration
+# 5) Check atomic headers are up-to-date
+#
+
+always += old-atomics
+targets += old-atomics
+
+quiet_cmd_atomics = CALL    $<
+      cmd_atomics = $(CONFIG_SHELL) scripts/atomic/check-atomics.sh
+
+old-atomics: scripts/atomic/check-atomics.sh FORCE
+	$(call cmd,atomics)
+
+#####
+# 6) Generate constants for Python GDB integration
 #
 
 extra-$(CONFIG_GDB_SCRIPTS) += build_constants_py
diff --git a/scripts/atomic/check-atomics.sh b/scripts/atomic/check-atomics.sh
new file mode 100755
index 000000000000..c30101cddf2d
--- /dev/null
+++ b/scripts/atomic/check-atomics.sh
@@ -0,0 +1,19 @@ 
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+#
+# Check if atomic headers are up-to-date
+
+ATOMICDIR=$(dirname $0)
+ATOMICTBL=${ATOMICDIR}/atomics.tbl
+LINUXDIR=${ATOMICDIR}/../..
+
+cat <<EOF |
+gen-atomic-instrumented.sh      asm-generic/atomic-instrumented.h
+gen-atomic-long.sh              asm-generic/atomic-long.h
+gen-atomic-fallback.sh          linux/atomic-fallback.h
+EOF
+while read script header; do
+	if ! (${ATOMICDIR}/${script} ${ATOMICTBL} | diff - ${LINUXDIR}/include/${header} > /dev/null); then
+		printf "warning: include/${header} is out-of-date.\n"
+	fi
+done