Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1575142 > unrolled thread

Re: [PATCH 20/89] sched/headers, signals: Separate out task_struct::signal and task_struct::sighand types and accessors into <linux/sched/signal.h>

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  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@linux-foundation.org> - 2017-02-06 22:20 +0100
    Re: [PATCH 20/89] sched/headers, signals: Separate out  task_struct::signal and task_struct::sighand types and accessors into  <linux/sched/signal.h> Ingo Molnar <mingo@kernel.org> - 2017-02-06 22:40 +0100
      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@linux-foundation.org> - 2017-02-06 23:00 +0100
        Re: [PATCH 20/89] sched/headers, signals: Separate out  task_struct::signal and task_struct::sighand types and accessors into  <linux/sched/signal.h> Ingo Molnar <mingo@kernel.org> - 2017-02-06 23:40 +0100

#1575142 — Re: [PATCH 20/89] sched/headers, signals: Separate out task_struct::signal and task_struct::sighand types and accessors into <linux/sched/signal.h>

On Mon, Feb 6, 2017 at 5:28 AM, Ingo Molnar <mingo@kernel.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:

>  arch/alpha/kernel/osf_sys.c                                    |   2 +-
>  arch/alpha/kernel/signal.c                                     |   2 +-
>  arch/alpha/kernel/traps.c                                      |   2 +-
>  arch/alpha/mm/fault.c                                          |   2 +-
>  arch/arc/kernel/traps.c                                        |   2 +-
>  arch/arc/mm/fault.c                                            |   2 +-
>  arch/arm/kernel/ptrace.c                                       |   2 +-
>  arch/arm/kernel/traps.c                                        |   2 +-
>  arch/arm/mm/alignment.c                                        |   2 +-
>  arch/arm/mm/fault.c                                            |   2 +-
>  arch/arm/mm/init.c                                             |   1 +
>  arch/arm/mm/mmap.c                                             |   2 +-
>  arch/arm/vfp/vfpmodule.c                                       |   2 +-
>  arch/arm64/kernel/fpsimd.c                                     |   2 +-
>  arch/arm64/kernel/ptrace.c                                     |   2 +-
>  arch/arm64/kernel/traps.c                                      |   2 +-
>  arch/arm64/mm/fault.c                                          |   2 +-
>  arch/arm64/mm/mmap.c                                           |   2 +-
>  arch/avr32/kernel/traps.c                                      |   2 +-
>  arch/blackfin/kernel/trace.c                                   |   2 +-
>  arch/blackfin/kernel/traps.c                                   |   1 +
>  arch/cris/mm/fault.c                                           |   1 +
... goes on forever - lots of stupid uninteresting one-liner patches ..

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.

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.

                 Linus

[toc] | [next] | [standalone]

#1575165 — 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@linux-foundation.org> wrote:

> On Mon, Feb 6, 2017 at 5:28 AM, Ingo Molnar <mingo@kernel.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

[toc] | [prev] | [next] | [standalone]

#1575179

On Mon, Feb 6, 2017 at 1:35 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
>
> 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

Oh, absolutely agreed. The "noise" is obviously the most important
part and the most challenging to actually generate (although maybe
some automation tool could do a lot of this in a perfect world).

But the noise is also the one where verification is pretty much purely
about "does it still compile", so from a human angle, once the noise
has been generated, it's not actually all that interesting.

It's kind of like being NP complete: verification of the noise is
"trivial" and not all that interesting - once the solution has been
find.

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

I suspect that will be a lot easier.to look at (with at least one
series basically being "there's no point in a human even looking at
it, other than to verify that it's purely #include changes").

It would be good to make sure it also ends up bisecting nicely,
because *if* some problem happens, it would be nice if the bisect then
clearly points to either "oh, some mistake must have happened during
code movement" vs "ooh, some really subtle issue with a missed include
causing some odd fallback code".

Because we *do* end up having code in C files that does things like

   #ifndef ARCH_HAS_XYZ
   static inline void my_generic_xyz_implementation(...)
   ...
    #endif

so you can end up in the situation that if some specific header file
wasn't included correctly, the code will still work, but now it will
use the generic definition rather than the specific one it was
supposed to use.

So these re-organizations do have the potential to cause odd "silent"
breakage. Most breakage by far should presumably be of the type "it
doesn't compile and it's very obvious that the header file movement
missed something", but we *could* have that kind of subtle "it
compiles, and _almost_ even works, but has a cornercase that is
broken" that could be fingered by a bisect.

That's when a good split of patches would be really nice to have too,
where a patch does either code movement or does header file
organization movement, but not both.

So it's not _just_ about actually looking at the patches and trying to
make sense of them.

                     Linus

[toc] | [prev] | [next] | [standalone]

#1575211 — 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@linux-foundation.org> wrote:

> On Mon, Feb 6, 2017 at 1:35 PM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> >
> > 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
> 
> Oh, absolutely agreed. The "noise" is obviously the most important
> part and the most challenging to actually generate (although maybe
> some automation tool could do a lot of this in a perfect world).

BTW., I used half-automation: an (imperfect...) script to recursively follow 
header dependencies to identify the places which I then changed manually, and a 
lot of testing to identify the cases the script missed (or where I made a manual 
mistake).

I started out with a testing based approach alone, but that did not scale very 
well and I had the feeling that it did not converge fast enough either to lead to 
a palatable end result.

Didn't want to invest too much into tooling, as I'd expect this to be a one-off 
effort for one of the worst header dependency problems we have in the kernel 
today. Fortunately most of our other headers are in a much better shape!

> But the noise is also the one where verification is pretty much purely
> about "does it still compile", so from a human angle, once the noise
> has been generated, it's not actually all that interesting.

Yeah, and the repetitive diffs over hundreds of files are actively hindering 
review.

> It's kind of like being NP complete: verification of the noise is
> "trivial" and not all that interesting - once the solution has been
> find.
> 
> > Will post them in 4 groups of 40 patches each or so - would that work for you?
> 
> I suspect that will be a lot easier.to look at (with at least one
> series basically being "there's no point in a human even looking at
> it, other than to verify that it's purely #include changes").
> 
> It would be good to make sure it also ends up bisecting nicely,
> because *if* some problem happens, it would be nice if the bisect then
> clearly points to either "oh, some mistake must have happened during
> code movement" vs "ooh, some really subtle issue with a missed include
> causing some odd fallback code".

Yeah, so I tested it intensively as I was developing it, so barring 
merge/backmerge/conflict-resolution artifacts that happen during rebase (and they 
did happen ...) there shouldn't be any conceptual bisectability hickups in the 
lineup. It wasn't a 'break it and make it work at the end of the series' 
development flow, it was a 'make it work at every commit' approach.

I'll do some bisection validation on the final result to make sure bisectability 
did not bitrot during all the restructuring. Problem is, even on a beefy 60-core 
server with 256 GB RAM a full cross-build test runs for half an hour just for 
defconfigs - so a 150-patch series bisectability test would run for over 3 days 
...

What I can do is some more careful manual review plus the re-testing of a couple 
of random points in the middle of the series - and of course the testing of the 
final result. This should make it reasonably certain that the good bisectability 
that was validated during earlier versions of the patch-set has a good chance of 
being present in the final series as well.

> Because we *do* end up having code in C files that does things like
> 
>    #ifndef ARCH_HAS_XYZ
>    static inline void my_generic_xyz_implementation(...)
>    ...
>     #endif
> 
> so you can end up in the situation that if some specific header file
> wasn't included correctly, the code will still work, but now it will
> use the generic definition rather than the specific one it was
> supposed to use.

Yeah, indeed.

> So these re-organizations do have the potential to cause odd "silent"
> breakage. Most breakage by far should presumably be of the type "it
> doesn't compile and it's very obvious that the header file movement
> missed something", but we *could* have that kind of subtle "it
> compiles, and _almost_ even works, but has a cornercase that is
> broken" that could be fingered by a bisect.
> 
> That's when a good split of patches would be really nice to have too,
> where a patch does either code movement or does header file
> organization movement, but not both.
> 
> So it's not _just_ about actually looking at the patches and trying to
> make sense of them.

Ok, I'm sold on this! This bisectability argument makes me feel much better about 
the resulting 150+ patch-count.

Thanks,

	Ingo

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web