Messages in this thread | | | From | Linus Torvalds <> | Date | Mon, 6 Feb 2017 13:11:39 -0800 | 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> |
| |
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
| |