Re: [PATCH 11/89] sched/headers, delayacct: Move the 'struct task_delay_info' definition from <linux/sched.h> to <linux/delayacct.h>

[Linus Torvalds]

On Mon, Feb 6, 2017 at 5:28 AM, Ingo Molnar <mingo@xxxxxxxxxx> wrote:
> The 'struct task_delay_info' definition does not have to be in sched.h,
> because task_struct only has a pointer to it.
>
> So move it to <linux/delayacct.h> to reduce the size of <linux/sched.h>.
>
> As an additional improvement make the type defined but empty in the
> !CONFIG_TASK_DELAY_ACCT case - to eliminate the ugly #ifdef
> around the task_struct field as well.

No. This is completely wrong.

Even if the structure is empty, the _pointer_ to the structure is not.
So now you removed the #ifdef, and the 'struct task_struct' becomes
unconditionally (and pointlessly) larger.

So your removal if the #ifdef and making that structure empty is
completely pointless: it wastes exactly the same amount of space even
when it is empty, because that pointer stays around and is not an
empty pointer.

In general, I heartily approve of the sched.h split-up, but quite
frankly, when there are almost a hundred patches, and a lot of them
are pure code movement (so they are *big*, and essentially impossible
to actually confirm), I *really* really think that this patch series
should be re-done so that it does *not* make these kinds of "clever"
changes.

I'd be much happier if the cleanups were all completely non-semantic.
Nothing like this. At least in the big patch series.

Then you can have a separate series that does things that isn't just
about code movement.

Ok?

Because these emails aren't easy to read as-is (well, part of them are
obvious, but others are "move a hundreds of lines from one file to
another").

And having to worry about "oh, and btw, hidden in the movement is this
small semantic change that may or may not be completely and utterly
bogus" makes it much much worse.

                  Linus