[PATCH] sched/wake_q: Restore task_struct::wake_q type safety

[Ingo Molnar <mingo@kernel.org>]

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

> On Wed, Feb 8, 2017 at 10:34 AM, Ingo Molnar <mingo@kernel.org> wrote:
> > To be able to decouple wake_q functionality from <linux/sched.h> make
> > the basic task_struct::wake_q pointer an opaque void *.
> 
> Please don't use "void *" for opaque pointers.
> 
> You lose all typechecking, and this is just complete garbage:
> 
> +       struct wake_q_node *node = (void *)&task->wake_q;
> 
> What? You're casting a "void *" to "void *"? WTF?

Well, I'm casting 'void **' to 'void *', so the cast is necessary in this specific 
scheme - but yeah, this is still pretty ugly.

> The proper way to do opaque pointers is to declare them as pointers to
> a structure that hasn't been defined (ie use a "struct xyz;" forward
> declaration), and then that structure is only actually defined in the
> places that use the otherwise opaque pointer.
> 
> That way you
> 
>  (a) never need to cast anything
> 
>  (b) get proper (and strong) type checking for the pointers.
> 
> and the people who need to look into it automatically do the right
> thing, while anybody else who tries to dereference or otherwise use
> the struct pointer will get a compiler error.

So the problem here is that we don't really want an opaque pointer in a classic 
sense, but an opaque field.

struct wake_q is:

  struct wake_q_node {
          struct wake_q_node *next;
  };

i.e. it's really just a single linked list node, a single pointer. This line:

> +       struct wake_q_node *node = (void *)&task->wake_q;

gets the pointer to the list node.

Couldn't think of a nicer way to do this - so let's not do this at all: we can 
just declare struct wake_q_node in sched.h and be done with it. We saved thousands 
of lines of code from it already, it's not worth doing it at the cost of strong 
type safety.

I.e. something like the patch attached below on top of WIP.sched/core?

Untested.

Thanks,

	Ingo


==============================>
>From fe7014cb9736f84f01f36003d72ed69d795ea82b Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@kernel.org>
Date: Wed, 8 Feb 2017 22:32:31 +0100
Subject: [PATCH] sched/wake_q: Restore task_struct::wake_q type safety

Linus correctly observed that task_struct::wake_q is an opaque pointer
in a rather ugly way, through which we lose not just readability but
also type safety, for only marginal savings in sched.h.

Just restore the 'struct wake_q_node' type in sched.h and include
sched.h in sched/wake_q.h.

Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 include/linux/sched.h        | 6 +++++-
 include/linux/sched/wake_q.h | 6 +-----
 ipc/msg.c                    | 1 -
 kernel/fork.c                | 2 +-
 kernel/sched/core.c          | 6 +++---
 5 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 2cc4d7902418..d67eee84fd43 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -476,6 +476,10 @@ enum perf_event_task_context {
 	perf_nr_task_contexts,
 };
 
+struct wake_q_node {
+	struct wake_q_node *next;
+};
+
 struct task_struct {
 #ifdef CONFIG_THREAD_INFO_IN_TASK
 	/*
@@ -765,7 +769,7 @@ struct task_struct {
 	/* Protection of the PI data structures: */
 	raw_spinlock_t			pi_lock;
 
-	void				*wake_q;
+	struct wake_q_node		wake_q;
 
 #ifdef CONFIG_RT_MUTEXES
 	/* PI waiters blocked on a rt_mutex held by this task: */
diff --git a/include/linux/sched/wake_q.h b/include/linux/sched/wake_q.h
index e6774a0385fb..d03d8a9047dc 100644
--- a/include/linux/sched/wake_q.h
+++ b/include/linux/sched/wake_q.h
@@ -28,11 +28,7 @@
  * wakeup condition has in fact occurred.
  */
 
-struct task_struct;
-
-struct wake_q_node {
-	struct wake_q_node *next;
-};
+#include <linux/sched.h>
 
 struct wake_q_head {
 	struct wake_q_node *first;
diff --git a/ipc/msg.c b/ipc/msg.c
index ecc387e573f6..104926dc72be 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -30,7 +30,6 @@
 #include <linux/proc_fs.h>
 #include <linux/list.h>
 #include <linux/security.h>
-#include <linux/sched.h>
 #include <linux/sched/wake_q.h>
 #include <linux/syscalls.h>
 #include <linux/audit.h>
diff --git a/kernel/fork.c b/kernel/fork.c
index 4a033bb7d660..68b1706c75d0 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -548,7 +548,7 @@ static struct task_struct *dup_task_struct(struct task_struct *orig, int node)
 #endif
 	tsk->splice_pipe = NULL;
 	tsk->task_frag.page = NULL;
-	tsk->wake_q = NULL;
+	tsk->wake_q.next = NULL;
 
 	account_kernel_stack(tsk, 1);
 
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 0abcf7307428..a4fdb6b03577 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -431,7 +431,7 @@ static bool set_nr_if_polling(struct task_struct *p)
 
 void wake_q_add(struct wake_q_head *head, struct task_struct *task)
 {
-	struct wake_q_node *node = (void *)&task->wake_q;
+	struct wake_q_node *node = &task->wake_q;
 
 	/*
 	 * Atomically grab the task, if ->wake_q is !nil already it means
@@ -460,11 +460,11 @@ void wake_up_q(struct wake_q_head *head)
 	while (node != WAKE_Q_TAIL) {
 		struct task_struct *task;
 
-		task = container_of((void *)node, struct task_struct, wake_q);
+		task = container_of(node, struct task_struct, wake_q);
 		BUG_ON(!task);
 		/* Task can safely be re-inserted now: */
 		node = node->next;
-		task->wake_q = NULL;
+		task->wake_q.next = NULL;
 
 		/*
 		 * wake_up_process() implies a wmb() to pair with the queueing