Date:   Mon, 6 Feb 2017 22:35:01 +0100
From:   Ingo Molnar <mingo@...nel.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Mike Galbraith <efault@....de>,
        Oleg Nesterov <oleg@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH 20/89] sched/headers, signals: Separate out
 task_struct::signal and task_struct::sighand types and accessors into
 <linux/sched/signal.h>


* Linus Torvalds <torvalds@...ux-foundation.org> wrote:

> On Mon, Feb 6, 2017 at 5:28 AM, Ingo Molnar <mingo@...nel.org> wrote:
> > task_struct::signal and task_struct::sighand are pointers, which would normally make it
> > straightforward to not define those types in sched.h.
> >
> > That is not so, because the types are accompanied by a myriad of APIs (macros and inline
> > functions) that dereference them.
> >
> > Split the types and the APIs out of sched.h and put them into a new header, <linux/sched/signal.h>.
> 
> So I still really like the split, but I absolutely *hate* slogging
> though these patches. I think the patches are really badly split up.
> 
> The actual *meat* of the patch (the part you want to look at) ends up
> being almost entirely hidden by the hundreds of lines of diff that are
> just this part:

Yeah, in some cases I solved this problem by doing a 'preparatory patch', such as:

 [PATCH 81/89] sched/headers: Remove spurious <linux/sched.h> inclusion dependencies

which are easy to review and which are then followed by the 'meat patches', such 
as:

 [PATCH 85/89] hrtimer: Remove the <linux/wait.h> include from <linux/hrtimer.h>
 [PATCH 86/89] sched/headers: Remove the <linux/topology.h> include from <linux/sched.h>

I considered doing this for every patch but decided against it due to the high 
patch count...

I was in a bit of a catch-22: the only way to demonstrate the utility of so many 
intrusive changes was to achieve some true reduction in generated code size ...
:-/

Will do some more splitting up, in the fashion you suggest, I fully agree with it:

> and I really think this whole split-up needs to be done differently.
> 
> What I would suggest is that it's done in two phases:
> 
>  (a) split up the code into a new header file, with absolutely _zero_
> semantic changes, because you leave a simple
> 
>         #include <linux/sched/new.h>
> 
>      in the <linux/sched.h> file.
> 
>  (b) a separate patch that just removes that one line from
> <linux/sched.h> and then has all this other "one-line noise" stuff.

Yeah.

BTW., most of the real work was in identifying and generating that "noise" - but 
to reviewers it's obviously the least interesting bits.

Also note that beyond the header splitup the "noise" is actually what improves 
kernel code the most all around, as changes like this:

--- a/kernel/utsname_sysctl.c
+++ b/kernel/utsname_sysctl.c
@@ -14,6 +14,7 @@
 #include <linux/utsname.h>
 #include <linux/sysctl.h>
 #include <linux/wait.h>
+#include <linux/rwsem.h>

 #ifdef CONFIG_PROC_SYSCTL

... make implicit header dependencies more explicit and decrease our existing 
spaghetti.

But the repetitive header dependency fixes/extensions should be clearly identified 
as such and shouldn't mix in other, more complex changes - I'll try to come up 
with a patch title pattern as well to make it easier to skip through these during 
review.

How about something like this:

 [PATCH] sched/headers: Propagate new header dependencies after changes to <linux/sched/task.h>
 [PATCH] sched/headers: Propagate new header dependencies after changes to <linux/sched/stat.h>
 [PATCH] sched/headers: Propagate new header dependencies after changes to <linux/sched/signal.h>
 [PATCH] sched/headers: Propagate new header dependencies before changes to <linux/sched.h>
 [PATCH] sched/headers: Propagate new header dependencies before changes to <linux/timer.h>
 ....

That way during review you can skip over these without much effort, you only have 
to check that these patches confirm to the expected 'trivial one line noise' 
pattern and don't do anything else.

The postfix style title will also make these stand out visually in the series.

> That way, in (a) it's really easy to see that you only moved things (and the 
> patch won't have all that noise in it), and then in (b) it's trivial to see that 
> all you do is fix up the #include things, because it will all _just_ be the 
> one-line noise.
> 
> This mix of noise and real changes is just very frustrating to look through. 
> Nobody sane will do it - you inevitably start skimming, because the one-liner 
> noise that is only relevant for the "it still builds correctly on all configs 
> and architectures" is simply not human-readable.
> 
> So this request is separate from the whole "please don't make semantic changes 
> at the same time" issue.
> 
> I realize that you want to do the header file fixups at the same time (in order 
> to find out whether you missed some issue that makes the split not useful), but 
> from a maintenance angle and a "encourage people to actually read through the 
> patches" angle this patch-series is just horrible.

No, your request is entirely fair, and I considered such techniques and did it in 
a few cases, and only decided against it to keep the patch count of the series 
down. Doing this will increase the patch count by at least 50%.

Will post them in 4 groups of 40 patches each or so - would that work for you?

Thanks,

	Ingo

Powered by blists - more mailing lists