Message-ID: <20170206211611.GA9788@gmail.com>
Date:   Mon, 6 Feb 2017 22:16:11 +0100
From:   Ingo Molnar <mingo@...nel.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>,
        "Paul E. McKenney" <paulmck@...ibm.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Mike Galbraith <efault@....de>,
        Oleg Nesterov <oleg@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: [PATCH v2 11/89] sched/headers, delayacct: Move the 'struct
 task_delay_info' definition from <linux/sched.h> to <linux/delayacct.h>


* Linus Torvalds <torvalds@...ux-foundation.org> wrote:

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

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

>From 387691c93599bc932f5c1c39b0d791d1cbca3cf7 Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@...nel.org>
Date: Wed, 1 Feb 2017 18:00:26 +0100
Subject: [PATCH] sched/headers, delayacct: Move the 'struct task_delay_info' definition from <linux/sched.h> to <linux/delayacct.h>

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

Cc: Peter Zijlstra <peterz@...radead.org>
Cc: Mike Galbraith <efault@....de>
Cc: Thomas Gleixner <tglx@...utronix.de>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: linux-kernel@...r.kernel.org
Signed-off-by: Ingo Molnar <mingo@...nel.org>
---
 include/linux/delayacct.h | 38 ++++++++++++++++++++++++++++++++++++--
 include/linux/sched.h     | 39 ++++-----------------------------------
 2 files changed, 40 insertions(+), 37 deletions(-)

diff --git a/include/linux/delayacct.h b/include/linux/delayacct.h
index 00e60f79a9cc..4178d2493547 100644
--- a/include/linux/delayacct.h
+++ b/include/linux/delayacct.h
@@ -18,8 +18,6 @@
 #define _LINUX_DELAYACCT_H
 
 #include <uapi/linux/taskstats.h>
-#include <linux/sched.h>
-#include <linux/slab.h>
 
 /*
  * Per-task flags relevant to delay accounting
@@ -30,7 +28,43 @@
 #define DELAYACCT_PF_BLKIO	0x00000002	/* I am waiting on IO */
 
 #ifdef CONFIG_TASK_DELAY_ACCT
+struct task_delay_info {
+	spinlock_t	lock;
+	unsigned int	flags;	/* Private per-task flags */
+
+	/* For each stat XXX, add following, aligned appropriately
+	 *
+	 * struct timespec XXX_start, XXX_end;
+	 * u64 XXX_delay;
+	 * u32 XXX_count;
+	 *
+	 * Atomicity of updates to XXX_delay, XXX_count protected by
+	 * single lock above (split into XXX_lock if contention is an issue).
+	 */
+
+	/*
+	 * XXX_count is incremented on every XXX operation, the delay
+	 * associated with the operation is added to XXX_delay.
+	 * XXX_delay contains the accumulated delay time in nanoseconds.
+	 */
+	u64 blkio_start;	/* Shared by blkio, swapin */
+	u64 blkio_delay;	/* wait for sync block io completion */
+	u64 swapin_delay;	/* wait for swapin block io completion */
+	u32 blkio_count;	/* total count of the number of sync block */
+				/* io operations performed */
+	u32 swapin_count;	/* total count of the number of swapin block */
+				/* io operations performed */
+
+	u64 freepages_start;
+	u64 freepages_delay;	/* wait for memory reclaim */
+	u32 freepages_count;	/* total count of memory reclaim */
+};
+#endif
 
+#include <linux/sched.h>
+#include <linux/slab.h>
+
+#ifdef CONFIG_TASK_DELAY_ACCT
 extern int delayacct_on;	/* Delay accounting turned on/off */
 extern struct kmem_cache *delayacct_cache;
 extern void delayacct_init(void);
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 4b49d159f4d5..59d28fd8e99b 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -901,39 +901,7 @@ struct sched_info {
 };
 #endif /* CONFIG_SCHED_INFO */
 
-#ifdef CONFIG_TASK_DELAY_ACCT
-struct task_delay_info {
-	spinlock_t	lock;
-	unsigned int	flags;	/* Private per-task flags */
-
-	/* For each stat XXX, add following, aligned appropriately
-	 *
-	 * struct timespec XXX_start, XXX_end;
-	 * u64 XXX_delay;
-	 * u32 XXX_count;
-	 *
-	 * Atomicity of updates to XXX_delay, XXX_count protected by
-	 * single lock above (split into XXX_lock if contention is an issue).
-	 */
-
-	/*
-	 * XXX_count is incremented on every XXX operation, the delay
-	 * associated with the operation is added to XXX_delay.
-	 * XXX_delay contains the accumulated delay time in nanoseconds.
-	 */
-	u64 blkio_start;	/* Shared by blkio, swapin */
-	u64 blkio_delay;	/* wait for sync block io completion */
-	u64 swapin_delay;	/* wait for swapin block io completion */
-	u32 blkio_count;	/* total count of the number of sync block */
-				/* io operations performed */
-	u32 swapin_count;	/* total count of the number of swapin block */
-				/* io operations performed */
-
-	u64 freepages_start;
-	u64 freepages_delay;	/* wait for memory reclaim */
-	u32 freepages_count;	/* total count of memory reclaim */
-};
-#endif	/* CONFIG_TASK_DELAY_ACCT */
+struct task_delay_info;
 
 static inline int sched_info_on(void)
 {
@@ -1615,9 +1583,10 @@ struct task_struct {
 
 	struct page_frag task_frag;
 
-#ifdef	CONFIG_TASK_DELAY_ACCT
-	struct task_delay_info *delays;
+#ifdef CONFIG_TASK_DELAY_ACCT
+	struct task_delay_info		*delays;
 #endif
+
 #ifdef CONFIG_FAULT_INJECTION
 	int make_it_fail;
 #endif

Powered by blists - more mailing lists