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

[Ingo Molnar]

* Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:

> 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.

Yeah, indeed, that was a brainfart: I mixed it up with the cases where it's not 
pointer-included but directly embedded in task_struct. In which case such a change 
is valid, see for example:

   [PATCH 88/89] sched/headers: Remove #ifdefs from <linux/sched.h>

So you are completely right, and this was a simple oversight on my part, and I've 
fixed it with the patch attached below, with the fix folded back and the bogus 
changelog fixed as well. The new WIP.sched/core is pushed out as of this minute, 
with:

   387691c93599 sched/headers, delayacct: Move the 'struct task_delay_info' definition from <linux/sched.h> to <linux/delayacct.h>

So this embarrasing episode never happened!

> 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?

Yeah, absolutely, fully agreed!

> 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.

This semantic change was unintentional, an embarrasing bug in essence.

I too want to keep the series a data structure invariant for the reasons you 
outlined, without any actual changes to the layout of task_struct et al.

I'll review the series once more to eliminate such changes.

There's one other very small change to generated code that was essential, the 
uninlining of two RCU APIs in the RCU_TINY uniprocessor case, to be able to 
decouple sched.h from completion.h:

  [PATCH 82/89] rcu: Separate the rcu synchronization types and APIs into <linux/rcupdate_wait.h>

... but that has to be acked by Paul anyway.

I'll try to be very careful with all this, OK?

( I've done a fair amount of boot testing as well along the way, so I'm fairly 
  certain there's no harmful runtime effect - this bug caused an unintentional, 
  unused pointer that bloated task_struct by 8 on configs that had that option 
  off. Famous last words ... )

Thanks,

	Ingo

=============================================================================