From patchwork Tue Jun 7 14:10:28 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: "Zanoni, Paulo R" X-Patchwork-Id: 9161461 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 691EA60572 for ; Tue, 7 Jun 2016 14:10:54 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 58C8F264F4 for ; Tue, 7 Jun 2016 14:10:54 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 4DACA27248; Tue, 7 Jun 2016 14:10:54 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 9FFD3264F4 for ; Tue, 7 Jun 2016 14:10:53 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1161304AbcFGOKd (ORCPT ); Tue, 7 Jun 2016 10:10:33 -0400 Received: from mga03.intel.com ([134.134.136.65]:42145 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1161033AbcFGOKa (ORCPT ); Tue, 7 Jun 2016 10:10:30 -0400 Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga103.jf.intel.com with ESMTP; 07 Jun 2016 07:10:29 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,433,1459839600"; d="scan'208";a="715260323" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by FMSMGA003.fm.intel.com with ESMTP; 07 Jun 2016 07:10:30 -0700 Received: from fmsmsx104.amr.corp.intel.com ([169.254.3.14]) by FMSMSX105.amr.corp.intel.com ([169.254.4.95]) with mapi id 14.03.0248.002; Tue, 7 Jun 2016 07:10:29 -0700 From: "Zanoni, Paulo R" To: "yamada.masahiro@socionext.com" , "mmarek@suse.cz" CC: "linux-kbuild@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "nicolas.pitre@linaro.org" Subject: Re: Regression in "kbuild: fix if_change and friends to consider argument order" Thread-Topic: Regression in "kbuild: fix if_change and friends to consider argument order" Thread-Index: AQHRwF1R1qIRFa3s/0KanTGyfUyyU5/eNUcAgAAFpwCAAAF+AIAADGmAgAALn4CAACzYgA== Date: Tue, 7 Jun 2016 14:10:28 +0000 Message-ID: <1465308627.3885.61.camel@intel.com> References: <1465263518.3885.9.camel@intel.com> <5756960E.5020706@suse.com> <57569ACC.8050503@suse.cz> <5756A675.3080003@suse.cz> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.254.1.155] Content-ID: <72C24DECA31A524A8CF00FB3C13FDF2B@intel.com> MIME-Version: 1.0 Sender: linux-kbuild-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kbuild@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Em Ter, 2016-06-07 às 20:29 +0900, Masahiro Yamada escreveu: > 2016-06-07 19:48 GMT+09:00 Michal Marek : > > > > On 2016-06-07 12:03, Masahiro Yamada wrote: > > > > > > 2016-06-07 18:58 GMT+09:00 Michal Marek : > > > > > > > > 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  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! > > > --- /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+'