Message-ID: <1465308627.3885.61.camel@intel.com>
Date:	Tue, 7 Jun 2016 14:10:28 +0000
From:	"Zanoni, Paulo R" <paulo.r.zanoni@...el.com>
To:	"yamada.masahiro@...ionext.com" <yamada.masahiro@...ionext.com>,
	"mmarek@...e.cz" <mmarek@...e.cz>
CC:	"linux-kbuild@...r.kernel.org" <linux-kbuild@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"nicolas.pitre@...aro.org" <nicolas.pitre@...aro.org>
Subject: Re: Regression in "kbuild: fix if_change and friends to consider
 argument order"

Em Ter, 2016-06-07 às 20:29 +0900, Masahiro Yamada escreveu:
> 2016-06-07 19:48 GMT+09:00 Michal Marek <mmarek@...e.cz>:
> > 
> > On 2016-06-07 12:03, Masahiro Yamada wrote:
> > > 
> > > 2016-06-07 18:58 GMT+09:00 Michal Marek <mmarek@...e.cz>:
> > > > 
> > > > On 2016-06-07 11:38, Michal Marek wrote:
> > > > > 
> > > > > On 2016-06-07 03:38, Zanoni, Paulo R wrote:
> > > > > > 
> > > > > > Hi
> > > > > > 
> > > > > > I recently noticed that alternating between "make" and
> > > > > > "make targz-pkg"
> > > > > > rebuilds the whole Kernel. This was not happening before.
> > > > > > As a Kernel
> > > > > > developer, my build/install/test environment heavily relies
> > > > > > on the fact
> > > > > > that "make targz-pkg" only quickly generates the tarball if
> > > > > > everything
> > > > > > is already built, so this change is heavily impacting my
> > > > > > development
> > > > > > environment.
> > > > > > 
> > > > > > I did some bisection and concluded that the first bad
> > > > > > commit is:
> > > > > > 
> > > > > > commit 9c8fa9bc08f60ac657751daba9fccf828a36cfed
> > > > > > Author: Masahiro Yamada
> > > > > > Date:   Sat May 7 15:48:26 2016 +0900
> > > > > >     kbuild: fix if_change and friends to consider argument
> > > > > > order
> > > > > > 
> > > > > > I also verified that if I just revert this commit on top of
> > > > > > the
> > > > > > most recent tree it goes back to the usual behavior.
> > > > > > 
> > > > > > I read the commit message and it seems that some unneeded
> > > > > > rebuilds are
> > > > > > somewhat expected, but I can't understand why such a change
> > > > > > in the
> > > > > > command line like the one I did triggers everything to be
> > > > > > rebuilt.
> > > > > > IMHO, it really shouldn't. I also wonder that maybe the
> > > > > > regression I'm
> > > > > > experiencing was not expected in the original change, so
> > > > > > maybe there's
> > > > > > a way to keep the original improvement caused by the
> > > > > > mentioned patch
> > > > > > without the regression I'm experiencing.
> > > > > > 
> > > > > > How to reproduce (exact commands I used at every bisect
> > > > > > step):
> > > > > > 
> > > > > > $ make tinyconfig
> > > > > > $ time make -j4 V=2 # this should build things
> > > > > > $ time make -j4 V=2 # just to make sure nothing will be
> > > > > > rebuilt
> > > > > > $ time make -j4 V=2 targz-pkg
> > > > > I can reproduce it.
> > > > Try the attached patch.
> > > > 
> > > > Michal
> > > > 
> > > 
> > > Right.
> > > 
> > > I had already sent a similar patch.
> > > https://patchwork.kernel.org/patch/9159863/
> > I see. I hadn't read all my mail before replying.
> > 
> > 
> > > 
> > > My concern is it is effectively
> > > reverting e8f5bdb02ce0.
> > > I hope Rik can comment on that.
> > My patch just resets NOSTDINC_FLAGS before including the arch
> > Makefile.
> > 
> Ah, I missed that, sorry.
> 
> Then, yours should be no problem.

I tested both patches you provided:
 - kbuild: do not append NOSTDINC_FLAGS to avoid rebuild in package
targets
 - kbuild: Initialize NOSTDINC_CFLAGS

Both seem to improve the situation to a point where, at least for a
tinyconfig, timings are acceptable. But it's important to notice that
none of the changes are equivalent to just reverting the first bad
commit. We still recompile some additional files that were not compiled
with the full revert. Let me show you:

$ # starting from a clean tree
$ git revert 9c8fa9bc08f60ac657751daba9fccf828a36cfed
$ time make -j4 V=2
$ time make -j4 V=2 targz-pkg 2>&1 > /tmp/revert-log.patch
$ git reset --hard HEAD~1 # go back to a clean tree, no revert
$ git am patches/0001-kbuild-Initialize-NOSTDINC_CFLAGS.patch
$ time make -j4 V=2
$ time make -j4 V=2 targz-pkg 2>&1 > /tmp/marek-fix-log.patch
$ diff -Nrup /tmp/revert-log.patch /tmp/marek-fix-log.patch 
--- /tmp/revert-log.patch	2016-06-07 10:30:47.118129155 -0300
+++ /tmp/marek-fix-log.patch	2016-06-07 10:33:00.271359159 -0300
@@ -8,7 +8,35 @@ make KBUILD_SRC=
   CHK     include/generated/asm-offsets.h
   CALL    scripts/checksyscalls.sh - due to target missing
   CHK     include/generated/compile.h
-Kernel: arch/x86/boot/bzImage is ready  (#39)
+  LINK    vmlinux - due to command line change
+  LD      vmlinux.o
+  MODPOST vmlinux.o - due to vmlinux.o not in $(targets)
+  GEN     .version
+  CHK     include/generated/compile.h
+  UPD     include/generated/compile.h
+  CC      init/version.o - due to: include/generated/compile.h
+  LD      init/built-in.o - due to: init/version.o
+  LD      vmlinux
+  SORTEX  vmlinux
+  SYSMAP  System.map
+  CC      arch/x86/boot/version.o - due to:
include/generated/compile.h
+  VOFFSET arch/x86/boot/compressed/../voffset.h - due to: vmlinux
+  OBJCOPY arch/x86/boot/compressed/vmlinux.bin - due to: vmlinux
+  XZKERN  arch/x86/boot/compressed/vmlinux.bin.xz - due to:
arch/x86/boot/compressed/vmlinux.bin
+  CC      arch/x86/boot/compressed/misc.o - due to:
arch/x86/boot/compressed/../voffset.h
+  MKPIGGY arch/x86/boot/compressed/piggy.S - due to:
arch/x86/boot/compressed/vmlinux.bin.xz
+  AS      arch/x86/boot/compressed/piggy.o - due to:
arch/x86/boot/compressed/piggy.S
+  LD      arch/x86/boot/compressed/vmlinux - due to:
arch/x86/boot/compressed/misc.o arch/x86/boot/compressed/piggy.o
+  ZOFFSET arch/x86/boot/zoffset.h - due to:
arch/x86/boot/compressed/vmlinux
+  OBJCOPY arch/x86/boot/vmlinux.bin - due to:
arch/x86/boot/compressed/vmlinux
+  AS      arch/x86/boot/header.o - due to: arch/x86/boot/zoffset.h
+  LD      arch/x86/boot/setup.elf - due to: arch/x86/boot/header.o
arch/x86/boot/version.o
+  OBJCOPY arch/x86/boot/setup.bin - due to: arch/x86/boot/setup.elf
+  BUILD   arch/x86/boot/bzImage - due to: arch/x86/boot/setup.bin
arch/x86/boot/vmlinux.bin
+Setup is 15484 bytes (padded to 15872 bytes).
+System is 361 kB
+CRC 85405354
+Kernel: arch/x86/boot/bzImage is ready  (#40)
 /bin/bash ./scripts/package/buildtar targz-pkg
 './System.map' -> './tar-install/boot/System.map-4.7.0-rc2+'
 './.config' -> './tar-install/boot/config-4.7.0-rc2+'


Now, let's try to debug the vmlinux rebuild:

$ time make -j4 V=2
$ cp .vmlinux.cmd /tmp/vmlinux-before.cmd
$ time make -j4 V=2 targz-pkg
$ diff -Nrup /tmp/vmlinux-before.cmd .vmlinux.cmd

--- /tmp/vmlinux-before.cmd	2016-06-07 10:39:08.791687067 -0300
+++ .vmlinux.cmd	2016-06-07 10:39:52.606774770 -0300
@@ -1 +1 @@
-cmd_vmlinux := /bin/bash scripts/link-vmlinux.sh ld -m elf_i386 --
build-id
+cmd_vmlinux := /bin/bash scripts/link-vmlinux.sh ld -m elf_i386 --
build-id --build-id

(my email client broke the long lines on the patch file, but you can
clearly see that targz-pkg added an extra --build-id to the cmd_vmlinux
declaration)

So after some grepping, I tried to also initialize LDFLAGS_vmlinux, in
the same way you did with NOSTDINC_FLAGS, and it fixed the problem for
me:

$ # This is all on top of Marek's patch
$ git diff
diff --git a/Makefile b/Makefile
index 5b3f0e7..9537635 100644
--- a/Makefile
+++ b/Makefile
@@ -364,6 +364,7 @@ CHECK		= sparse
 CHECKFLAGS     := -D__linux__ -Dlinux -D__STDC__ -Dunix -D__unix__ \
 		  -Wbitwise -Wno-return-void $(CF)
 NOSTDINC_FLAGS  =
+LDFLAGS_vmlinux =
 CFLAGS_MODULE   =
 AFLAGS_MODULE   =
 LDFLAGS_MODULE  =
$ time make -j4 V=2
$ time make -j4 V=2 2>&1 > /tmp/full-fix.patch
$ diff -Nrup /tmp/revert-log.patch /tmp/full-fix.patch
--- /tmp/revert-log.patch	2016-06-07 10:30:47.118129155 -0300
+++ /tmp/full-fix.patch	2016-06-07 10:50:44.851252745 -0300
@@ -8,7 +8,7 @@ make KBUILD_SRC=
   CHK     include/generated/asm-offsets.h
   CALL    scripts/checksyscalls.sh - due to target missing
   CHK     include/generated/compile.h
-Kernel: arch/x86/boot/bzImage is ready  (#39)
+Kernel: arch/x86/boot/bzImage is ready  (#45)
 /bin/bash ./scripts/package/buildtar targz-pkg
 './System.map' -> './tar-install/boot/System.map-4.7.0-rc2+'
 './.config' -> './tar-install/boot/config-4.7.0-rc2+'

I also wondered that maybe we need to also initialize
KBUILD_LDFLAGS_MODULE, but it seems what fixed the problem was just
LDFLAGS_vmlinux. So I'm not sure if we'll need this. I also have no
idea whether this would cause other unintended regressions. It's up to
you, Makefile maintainers, to judge. 

Thanks for the quick responses!

> 
> 
> 

Powered by blists - more mailing lists