Message-ID: <20190510190422.GB219679@google.com>
Date:   Fri, 10 May 2019 15:04:22 -0400
From:   Joel Fernandes <joel@...lfernandes.org>
To:     Masahiro Yamada <yamada.masahiro@...ionext.com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Greg KH <gregkh@...uxfoundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        "Cc: Android Kernel" <kernel-team@...roid.com>,
        Linux List Kernel Mailing <linux-kernel@...r.kernel.org>
Subject: Re: [GIT PULL] Driver core patches for 5.2-rc1

On Fri, May 10, 2019 at 04:58:32PM +0900, Masahiro Yamada wrote:
> Hi Joel,
> 
> On Fri, May 10, 2019 at 11:38 AM Joel Fernandes <joel@...lfernandes.org> wrote:
> >
> > On Thu, May 09, 2019 at 01:47:54PM -0700, Linus Torvalds wrote:
> > > [ Ok, this may look irrelevant to people, but I actually notice this
> > > because I do quick rebuilds *all* the time, so the 30s vs 41s
> > > difference is actually something I reacted to and then tried to figure
> > > out... ]
> > >
> > > On Tue, May 7, 2019 at 10:59 AM Greg KH <gregkh@...uxfoundation.org> wrote:
> > > >
> > > > Joel Fernandes (Google) (2):
> > > >       Provide in-kernel headers to make extending kernel easier
> > >
> > > Joel and Masahiro,
> > >  this commit does annoying things. It's a small thing, but it ends up
> > > grating on my kernel rebuild times, so I hope somebody can take a look
> > > at it..
> > >
> > > Try building a kernel with no changes, and it shouldn't re-link.
> > >
> > > HOWEVER.
> > >
> > > If you re-make the config in between, the kernel/kheaders_data.tar.xz
> > > is re-generated too. I think it checks timestamps despite having that
> > > "CHK" phase that should verify just contents.
> > >
> > > I think the kernel/config_data.gz rules do the same thing, judging by
> > > the output.
> > >
> > > I use "make allmodconfig" to re-generate the same kernel config, which
> > > triggers this. The difference between "nothing changed" and "rerun
> > > 'make allmodconfig' and nothing _still_ should have changed" is quite
> > > stark:
> > [snip]
> > > No, this isn't the end of the world, but if somebody sees a simple
> > > solution to avoid that extra ten seconds, I'd appreciate it.
> >
> > Hi Linus,
> > The following patch should fix the issue. The patch depends on [1] though. So
> > that will have to be pulled first (which I believe Greg is going to pull soon
> > since it is in his pipeline, and Steven Rostedt already Acked it)
> > [1] https://lore.kernel.org/patchwork/patch/1070199/
> >
> > For the below patch which fixes this issue, I have tested it and it fixes the
> > allmodconfig issue. Could you try it out as well? As mentioned above, the
> > patch at [1] should be applied first. Thanks a lot and let me know how it goes.
> >
> > (I am going to be on a long haul flight shortly so I may not be available for
> > next 24-48 hours but will be there after, thanks).
> > ---8<-----------------------
> >
> > From: "Joel Fernandes (Google)" <joel@...lfernandes.org>
> > Subject: [PATCH] gen_kheaders: Do not regenerate archive if config is not
> >  changed
> >
> > Linus reported that allmodconfig config was causing the kheaders archive
> > to be regenerated even though the config is the same. This is due to the
> > fact that the generated config header files are rewritten even if they
> > were the same from a previous run.
> 
> 
> I will fix the root cause in Kconfig anyway.
> 
> 
> This patch is still useful in case
> "one of Kconfig files is changed,
> but the resulted configuration is still the same"
> 
> $ touch Kconfig
> $ make -j8
> 
> 
> But, you should simplify the code.
> 
> 
> > To fix the issue, we ignore changes to these files and use md5sum on
> > auto.conf to determine if the config really changed. And regenerate the
> > header archive if it has.
> 
> 
> Nope. This is really unnecessary.
> 
> When CONFIG_FOO_BAR is changed, include/config/foo/bar.h
> should have already been touched by Kconfig,
> so the change of the kernel configuration
> is correctly detected.
> 
> If you want to know the this Kbuild magic,
> read the comment block of scripts/basic/fixdep.c
> 
> 
> Please remove config_md5 and correct the commit log.

Agreed! Will remove the md5 check and keep the rest of the patch the same
(except the commit log).

thanks!

 - Joel


> 
> Thanks.
> 
> 
> 
> 
> > Reported-by: Linus Torvalds <torvalds@...ux-foundation.org>
> > Signed-off-by: Joel Fernandes (Google) <joel@...lfernandes.org>
> > ---
> >  kernel/gen_kheaders.sh | 16 ++++++++++++++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/gen_kheaders.sh b/kernel/gen_kheaders.sh
> > index 581b83534587..f621242037f4 100755
> > --- a/kernel/gen_kheaders.sh
> > +++ b/kernel/gen_kheaders.sh
> > @@ -33,7 +33,7 @@ arch/$SRCARCH/include/
> >  # Uncomment it for debugging.
> >  # iter=1
> >  # if [ ! -f /tmp/iter ]; then echo 1 > /tmp/iter;
> > -# else;        iter=$(($(cat /tmp/iter) + 1)); fi
> > +# else         iter=$(($(cat /tmp/iter) + 1)); fi
> >  # find $src_file_list -type f | xargs ls -lR > /tmp/src-ls-$iter
> >  # find $obj_file_list -type f | xargs ls -lR > /tmp/obj-ls-$iter
> >
> > @@ -43,16 +43,27 @@ arch/$SRCARCH/include/
> >  pushd $kroot > /dev/null
> >  src_files_md5="$(find $src_file_list -type f                       |
> >                 grep -v "include/generated/compile.h"              |
> > +               grep -v "include/generated/autoconf.h"             |
> > +               grep -v "include/config/auto.conf"                 |
> > +               grep -v "include/config/auto.conf.cmd"             |
> > +               grep -v "include/config/tristate.conf"             |
> >                 xargs ls -lR | md5sum | cut -d ' ' -f1)"
> >  popd > /dev/null
> >  obj_files_md5="$(find $obj_file_list -type f                       |
> >                 grep -v "include/generated/compile.h"              |
> > +               grep -v "include/generated/autoconf.h"             |
> > +               grep -v "include/config/auto.conf"                 |
> > +               grep -v "include/config/auto.conf.cmd"             |
> > +               grep -v "include/config/tristate.conf"             |
> >                 xargs ls -lR | md5sum | cut -d ' ' -f1)"
> >
> > +config_md5="$(md5sum include/config/auto.conf | cut -d ' ' -f1)"
> > +
> >  if [ -f $tarfile ]; then tarfile_md5="$(md5sum $tarfile | cut -d ' ' -f1)"; fi
> >  if [ -f kernel/kheaders.md5 ] &&
> >         [ "$(cat kernel/kheaders.md5|head -1)" == "$src_files_md5" ] &&
> >         [ "$(cat kernel/kheaders.md5|head -2|tail -1)" == "$obj_files_md5" ] &&
> > +       [ "$(cat kernel/kheaders.md5|head -3|tail -1)" == "$config_md5" ] &&
> >         [ "$(cat kernel/kheaders.md5|tail -1)" == "$tarfile_md5" ]; then
> >                 exit
> >  fi
> > @@ -82,8 +93,9 @@ find $cpio_dir -type f -print0 |
> >
> >  tar -Jcf $tarfile -C $cpio_dir/ . > /dev/null
> >
> > -echo "$src_files_md5" > kernel/kheaders.md5
> > +echo "$src_files_md5" >  kernel/kheaders.md5
> >  echo "$obj_files_md5" >> kernel/kheaders.md5
> > +echo "$config_md5"    >> kernel/kheaders.md5
> >  echo "$(md5sum $tarfile | cut -d ' ' -f1)" >> kernel/kheaders.md5
> >
> >  rm -rf $cpio_dir
> > --
> > 2.21.0.1020.gf2820cf01a-goog
> >
> 
> 
> --
> Best Regards
> Masahiro Yamada

Powered by blists - more mailing lists