Re: [PATCH v3 08/12] testing: net-drv: add basic shaper test

[Simon Horman <horms@kernel.org>]

On Thu, Aug 08, 2024 at 07:17:54AM -0700, Jakub Kicinski wrote:
> On Thu, 8 Aug 2024 13:20:42 +0100 Simon Horman wrote:
> > Thanks again for the information.
> > 
> > I have now taken another look at this problem.
> > 
> > Firstly, my analysis is that the cause of the problem is a combination of
> > the way the patchset is constricted, and the way that the build tests (I
> > have focussed on build_allmodconfig_warn.sh [1]).
> > 
> > [1] https://github.com/linux-netdev/nipa/blob/main/tests/patch/build_allmodconfig_warn/build_allmodconfig.sh
> > 
> > What I believe happens is this: The patches 01/12 - 07/12 modify some
> > header files, adds a new Kconfig entry, and does a bunch of other normal
> > stuff. Each of those patches is tested in turn, and everything seems fine.
> > 
> > Then we get to patch 08/12. The key thing about this patch is that it
> > enables the CONFIG_NET_SHAPER Kconfig option, in the context of an
> > allmodconfig build. That in turn modifies the headers
> > include/linux/netdevice.h and net/core/dev.h (and net/Makefile). Not in the
> > in terms of their on-disk contents changing, but rather in the case of the
> > header files, in terms of preprocessor output. And this is, I believe,
> > where everything goes wrong.
> 
> That's strange, make does not understand preprocessor, does it?
> Either file has been modified or it has not.

True, that is a good point.
I would say there is something deeper going on than
I have been able to discover at this time: probably something very obvious.

> I guess it doesn't matter, given your solution  

Right, in any case I think the script needs to be enhanced.
My solution may prove heavy handed, but it can be improved over time.

> > NIPA arrives at running build_allmodconfig_warn.sh for patch 08/12 with the tree
> > built for the previous patch, 07/12. It then:
> > 
> > * touches $output_dir/include/generated/utsrelease.h
> > * checks out HEAD~ (patch 07/12)
> > * prepares the kernel config
> > * builds kernel and records incumbent errors (49)
> > 
> > The thing to note here is that the tree has been little perturbed since build
> > tests were run for patch 07/12, and thus few files are rebuilt.
> > 
> > Moving on, simplifying things, the following then runs:
> > 
> > * touches $output_dir/include/generated/utsrelease.h
> > * checks out $HEAD (patch 08/12)
> > * prepares the kernel config
> > * builds kernel and records current errors (4219)
> > 
> > The key to understanding why the large delta between 49 and 4219 is
> > that vastly more files have been rebuilt. Because the preprocessor output
> > of netdevice.h and dev.h have changes since the last build, and those
> > headers are included, directly or indirectly, by a lot of files (and
> > compilation results in warnings for many of those files).
> > 
> > 
> > I was able to reproduce the result by running build_allmodconfig_warn.sh
> > over patch 07/12 and then 07/12 with FIRST_IN_SERIES=0.

Correction: this should have read "07/12 and then 08/12"

> > I was able to get the desired result no new compiler warnings
> > by doing the same again, but with FIRST_IN_SERIES=1 for the
> > invocation of build_allmodconfig_warn.sh for 08/12.
> > 
> > I believe this is entirely due to a baseline rebuild being run due to the
> > FIRST_IN_SERIES=1 parameter. And, FWIIW, I believe the invocation of
> > build_allmodconfig_warn.sh for 07/12 ensures reproducibility.
> > 
> > My suggestion is that while we may consider reorganising the patch-set,
> > that is really only a work around. And it would be best to make the CI more
> > robust in the presence of such constructions.
> > 
> > It may be a bit heavy handed, but my tested solution is to invoke a
> > baseline rebuild if a Kconfig change is made. At the very last it does
> > address the problem at hand. (In precisely the same way as manually setting
> > FIRST_IN_SERIES=1.)
> > 
> > The patch implementing this for build_allmodconfig.sh which I tested is
> > below. If we want to go ahead with this approach then I expect it is best
> > to add it to other build tests too. But this seems to be a good point
> > to report my findings, so here we are.
> > 
> > --- build_allmodconfig.sh.orig  2024-08-08 07:30:56.599372164 +0000
> > +++ build_allmodconfig.sh       2024-08-08 09:58:22.692206313 +0000
> > @@ -34,8 +34,10 @@
> >  echo "Tree base:"
> >  git log -1 --pretty='%h ("%s")' HEAD~
> > 
> > -if [ x$FIRST_IN_SERIES == x0 ]; then
> > -    echo "Skip baseline build, not the first patch"
> > +if [ x$FIRST_IN_SERIES == x0 ] && \
> > +   ! git diff --name-only HEAD~ | grep -q -E "Kconfig$"
> > +then
> > +    echo "Skip baseline build, not the first patch and no Kconfig updates"
> >  else
> >      echo "Baseline building the tree"
> 
> Excellent idea, let's try it! Could you send a PR to NIPA?

Yes, can do.

> Note that the code is copied 3 times for each flavor of building :(

I assumed so :)