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]

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