[PATCH 0/3] THREAD_INFO_IN_TASK prep work for arm64+s390

[PATCH 0/3] THREAD_INFO_IN_TASK prep work for arm64+s390

[Mark Rutland]

Hi all,

Heiko and I have been working on THREAD_INFO_IN_TASK for s390 and arm64
respectively, and we're both targetting v4.10.

These are the common core changes which we both require, which happen to
touch x86 and some core headers. We'd either need these merged for
v4.9-rc*, or placed on a stable branch/tag that we can both base atop
of.

I've put together a branch [1,2] based on v4.9-rc1, but the patches themselves
are sorely lacking in relevant acks. Are people willing to help change that? ;)

Thanks,
Mark.

[1] git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git core/ti-stack-split
[2] https://git.kernel.org/cgit/linux/kernel/git/mark/linux.git/log/?h=core/ti-stack-split

Heiko Carstens (1):
  sched/core,x86: make struct thread_info arch specific again

Mark Rutland (2):
  thread_info: factor out restart_block
  thread_info: include <current.h> for THREAD_INFO_IN_TASK

 arch/x86/include/asm/thread_info.h |  9 +++++++
 include/linux/restart_block.h      | 51 ++++++++++++++++++++++++++++++++++++
 include/linux/thread_info.h        | 53 ++------------------------------------
 3 files changed, 62 insertions(+), 51 deletions(-)
 create mode 100644 include/linux/restart_block.h

-- 
1.9.1

[PATCH 1/3] sched/core,x86: make struct thread_info arch specific again

[Mark Rutland]

From: Heiko Carstens <heiko.carstens@de.ibm.com>

commit c65eacbe290b ("sched/core: Allow putting thread_info into
task_struct") made struct thread_info a generic struct with only a
single flags member if THREAD_INFO_IN_TASK_STRUCT is selected.

This change however seems to be quite x86 centric, since at least the
generic preemption code (asm-generic/preempt.h) assumes that struct
thread_info also has a preempt_count member, which apparently was not
true for x86.

We could add a bit more ifdefs to solve this problem too, but it seems
to be much simpler to make struct thread_info arch specific
again. This also makes the conversion to THREAD_INFO_IN_TASK_STRUCT a
bit easier for architectures that have a couple of arch specific stuff
in their thread_info definition.

The arch specific stuff _could_ be moved to thread_struct. However
keeping them in thread_info makes it easier: accessing thread_info
members is simple, since it is at the beginning of the task_struct,
while the thread_struct is at the end. At least on s390 the offsets
needed to access members of the thread_struct (with task_struct as
base) are too large for various asm instructions.  This is not a
problem when keeping these members within thread_info.

Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/include/asm/thread_info.h |  9 +++++++++
 include/linux/thread_info.h        | 11 -----------
 2 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
index 2aaca53..ad6f5eb0 100644
--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -52,6 +52,15 @@
 #include <asm/cpufeature.h>
 #include <linux/atomic.h>
 
+struct thread_info {
+	unsigned long		flags;		/* low level flags */
+};
+
+#define INIT_THREAD_INFO(tsk)			\
+{						\
+	.flags		= 0,			\
+}
+
 #define init_stack		(init_thread_union.stack)
 
 #else /* !__ASSEMBLY__ */
diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
index 45f004e..2873baf 100644
--- a/include/linux/thread_info.h
+++ b/include/linux/thread_info.h
@@ -14,17 +14,6 @@
 struct compat_timespec;
 
 #ifdef CONFIG_THREAD_INFO_IN_TASK
-struct thread_info {
-	unsigned long		flags;		/* low level flags */
-};
-
-#define INIT_THREAD_INFO(tsk)			\
-{						\
-	.flags		= 0,			\
-}
-#endif
-
-#ifdef CONFIG_THREAD_INFO_IN_TASK
 #define current_thread_info() ((struct thread_info *)current)
 #endif
 
-- 
1.9.1

Re: [PATCH 1/3] sched/core,x86: make struct thread_info arch specific again

[Andy Lutomirski]

On Wed, Oct 19, 2016 at 11:28 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> From: Heiko Carstens <heiko.carstens@de.ibm.com>
>
> commit c65eacbe290b ("sched/core: Allow putting thread_info into
> task_struct") made struct thread_info a generic struct with only a
> single flags member if THREAD_INFO_IN_TASK_STRUCT is selected.
>
> This change however seems to be quite x86 centric, since at least the
> generic preemption code (asm-generic/preempt.h) assumes that struct
> thread_info also has a preempt_count member, which apparently was not
> true for x86.
>
> We could add a bit more ifdefs to solve this problem too, but it seems
> to be much simpler to make struct thread_info arch specific
> again. This also makes the conversion to THREAD_INFO_IN_TASK_STRUCT a
> bit easier for architectures that have a couple of arch specific stuff
> in their thread_info definition.
>
> The arch specific stuff _could_ be moved to thread_struct. However
> keeping them in thread_info makes it easier: accessing thread_info
> members is simple, since it is at the beginning of the task_struct,
> while the thread_struct is at the end. At least on s390 the offsets
> needed to access members of the thread_struct (with task_struct as
> base) are too large for various asm instructions.  This is not a
> problem when keeping these members within thread_info.

Acked-by: Andy Lutomirski <luto@kernel.org>

Ingo, there's a (somewhat weak) argument for sending this via
tip/urgent: it doesn't change generated code at all, and I think it
will avoid a silly depedency or possible conflict for the next merge
window, since both arm64 and s390 are going to need it.

--Andy

Re: [PATCH 1/3] sched/core,x86: make struct thread_info arch specific again

[Ingo Molnar]

* Andy Lutomirski <luto@amacapital.net> wrote:

> On Wed, Oct 19, 2016 at 11:28 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > From: Heiko Carstens <heiko.carstens@de.ibm.com>
> >
> > commit c65eacbe290b ("sched/core: Allow putting thread_info into
> > task_struct") made struct thread_info a generic struct with only a
> > single flags member if THREAD_INFO_IN_TASK_STRUCT is selected.
> >
> > This change however seems to be quite x86 centric, since at least the
> > generic preemption code (asm-generic/preempt.h) assumes that struct
> > thread_info also has a preempt_count member, which apparently was not
> > true for x86.
> >
> > We could add a bit more ifdefs to solve this problem too, but it seems
> > to be much simpler to make struct thread_info arch specific
> > again. This also makes the conversion to THREAD_INFO_IN_TASK_STRUCT a
> > bit easier for architectures that have a couple of arch specific stuff
> > in their thread_info definition.
> >
> > The arch specific stuff _could_ be moved to thread_struct. However
> > keeping them in thread_info makes it easier: accessing thread_info
> > members is simple, since it is at the beginning of the task_struct,
> > while the thread_struct is at the end. At least on s390 the offsets
> > needed to access members of the thread_struct (with task_struct as
> > base) are too large for various asm instructions.  This is not a
> > problem when keeping these members within thread_info.
> 
> Acked-by: Andy Lutomirski <luto@kernel.org>
> 
> Ingo, there's a (somewhat weak) argument for sending this via
> tip/urgent: it doesn't change generated code at all, and I think it
> will avoid a silly depedency or possible conflict for the next merge
> window, since both arm64 and s390 are going to need it.

Can certainly do it if this is the final version of the patch. Mark?

Thanks,

	Ingo

Re: [PATCH 1/3] sched/core,x86: make struct thread_info arch specific again

[Mark Rutland]

On Thu, Oct 20, 2016 at 08:40:45AM +0200, Ingo Molnar wrote:
> 
> * Andy Lutomirski <luto@amacapital.net> wrote:
> 
> > On Wed, Oct 19, 2016 at 11:28 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > > From: Heiko Carstens <heiko.carstens@de.ibm.com>
> > >
> > > commit c65eacbe290b ("sched/core: Allow putting thread_info into
> > > task_struct") made struct thread_info a generic struct with only a
> > > single flags member if THREAD_INFO_IN_TASK_STRUCT is selected.
> > >
> > > This change however seems to be quite x86 centric, since at least the
> > > generic preemption code (asm-generic/preempt.h) assumes that struct
> > > thread_info also has a preempt_count member, which apparently was not
> > > true for x86.
> > >
> > > We could add a bit more ifdefs to solve this problem too, but it seems
> > > to be much simpler to make struct thread_info arch specific
> > > again. This also makes the conversion to THREAD_INFO_IN_TASK_STRUCT a
> > > bit easier for architectures that have a couple of arch specific stuff
> > > in their thread_info definition.
> > >
> > > The arch specific stuff _could_ be moved to thread_struct. However
> > > keeping them in thread_info makes it easier: accessing thread_info
> > > members is simple, since it is at the beginning of the task_struct,
> > > while the thread_struct is at the end. At least on s390 the offsets
> > > needed to access members of the thread_struct (with task_struct as
> > > base) are too large for various asm instructions.  This is not a
> > > problem when keeping these members within thread_info.
> > 
> > Acked-by: Andy Lutomirski <luto@kernel.org>
> > 
> > Ingo, there's a (somewhat weak) argument for sending this via
> > tip/urgent: it doesn't change generated code at all, and I think it
> > will avoid a silly depedency or possible conflict for the next merge
> > window, since both arm64 and s390 are going to need it.
> 
> Can certainly do it if this is the final version of the patch. Mark?

Yes; this is the final version of this patch.

I can rebase the other two core patches atop, assuming this goes in for
a v4.9-rc* tag soon.

Thanks,
Mark.

[tip:x86/urgent] sched/core, x86: Make struct thread_info arch specific again

[tip-bot for Heiko Carstens]

Commit-ID:  c8061485a0d7569a865a3cc3c63347b0f42b3765
Gitweb:     http://git.kernel.org/tip/c8061485a0d7569a865a3cc3c63347b0f42b3765
Author:     Heiko Carstens <heiko.carstens@de.ibm.com>
AuthorDate: Wed, 19 Oct 2016 19:28:11 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 20 Oct 2016 13:27:47 +0200

sched/core, x86: Make struct thread_info arch specific again

The following commit:

  c65eacbe290b ("sched/core: Allow putting thread_info into task_struct")

... made 'struct thread_info' a generic struct with only a
single ::flags member, if CONFIG_THREAD_INFO_IN_TASK_STRUCT=y is
selected.

This change however seems to be quite x86 centric, since at least the
generic preemption code (asm-generic/preempt.h) assumes that struct
thread_info also has a preempt_count member, which apparently was not
true for x86.

We could add a bit more #ifdefs to solve this problem too, but it seems
to be much simpler to make struct thread_info arch specific
again. This also makes the conversion to THREAD_INFO_IN_TASK_STRUCT a
bit easier for architectures that have a couple of arch specific stuff
in their thread_info definition.

The arch specific stuff _could_ be moved to thread_struct. However
keeping them in thread_info makes it easier: accessing thread_info
members is simple, since it is at the beginning of the task_struct,
while the thread_struct is at the end. At least on s390 the offsets
needed to access members of the thread_struct (with task_struct as
base) are too large for various asm instructions.  This is not a
problem when keeping these members within thread_info.

Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Acked-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: keescook@chromium.org
Cc: linux-arch@vger.kernel.org
Link: http://lkml.kernel.org/r/1476901693-8492-2-git-send-email-mark.rutland@arm.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/thread_info.h |  9 +++++++++
 include/linux/thread_info.h        | 11 -----------
 2 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
index 2aaca53..ad6f5eb0 100644
--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -52,6 +52,15 @@ struct task_struct;
 #include <asm/cpufeature.h>
 #include <linux/atomic.h>
 
+struct thread_info {
+	unsigned long		flags;		/* low level flags */
+};
+
+#define INIT_THREAD_INFO(tsk)			\
+{						\
+	.flags		= 0,			\
+}
+
 #define init_stack		(init_thread_union.stack)
 
 #else /* !__ASSEMBLY__ */
diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
index 45f004e..2873baf 100644
--- a/include/linux/thread_info.h
+++ b/include/linux/thread_info.h
@@ -14,17 +14,6 @@ struct timespec;
 struct compat_timespec;
 
 #ifdef CONFIG_THREAD_INFO_IN_TASK
-struct thread_info {
-	unsigned long		flags;		/* low level flags */
-};
-
-#define INIT_THREAD_INFO(tsk)			\
-{						\
-	.flags		= 0,			\
-}
-#endif
-
-#ifdef CONFIG_THREAD_INFO_IN_TASK
 #define current_thread_info() ((struct thread_info *)current)
 #endif
 

[PATCH 2/3] thread_info: factor out restart_block

[Mark Rutland]

Since commit f56141e3e2d9aabf ("all arches, signal: move restart_block
to struct task_struct"), thread_info and restart_block have been
logically distinct, yet struct restart_block is still defined in
<linux/thread_info.h>.

At least one architecture (erroneously) uses restart_block as part of
its thread_info, and thus the definition of restart_block must come
before the include of <asm/thread_info>. Subsequent patches in this
series need to shuffle the order of includes and definitions in
<linux/thread_info.h>, and will make this ordering fragile.

This patch moves the definition of restart_block out to its own header.
This serves as generic cleanup, logically separating thread_info and
restart_block, and also makes it easier to avoid fragility.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: linux-kernel@vger.kernel.org
---
 include/linux/restart_block.h | 51 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/thread_info.h   | 41 +---------------------------------
 2 files changed, 52 insertions(+), 40 deletions(-)
 create mode 100644 include/linux/restart_block.h

diff --git a/include/linux/restart_block.h b/include/linux/restart_block.h
new file mode 100644
index 0000000..0d905d8
--- /dev/null
+++ b/include/linux/restart_block.h
@@ -0,0 +1,51 @@
+/*
+ * Common syscall restarting data
+ */
+#ifndef __LINUX_RESTART_BLOCK_H
+#define __LINUX_RESTART_BLOCK_H
+
+#include <linux/compiler.h>
+#include <linux/types.h>
+
+struct timespec;
+struct compat_timespec;
+struct pollfd;
+
+/*
+ * System call restart block.
+ */
+struct restart_block {
+	long (*fn)(struct restart_block *);
+	union {
+		/* For futex_wait and futex_wait_requeue_pi */
+		struct {
+			u32 __user *uaddr;
+			u32 val;
+			u32 flags;
+			u32 bitset;
+			u64 time;
+			u32 __user *uaddr2;
+		} futex;
+		/* For nanosleep */
+		struct {
+			clockid_t clockid;
+			struct timespec __user *rmtp;
+#ifdef CONFIG_COMPAT
+			struct compat_timespec __user *compat_rmtp;
+#endif
+			u64 expires;
+		} nanosleep;
+		/* For poll */
+		struct {
+			struct pollfd __user *ufds;
+			int nfds;
+			int has_timeout;
+			unsigned long tv_sec;
+			unsigned long tv_nsec;
+		} poll;
+	};
+};
+
+extern long do_no_restart_syscall(struct restart_block *parm);
+
+#endif /* __LINUX_RESTART_BLOCK_H */
diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
index 2873baf..c75c6ab 100644
--- a/include/linux/thread_info.h
+++ b/include/linux/thread_info.h
@@ -9,51 +9,12 @@
 
 #include <linux/types.h>
 #include <linux/bug.h>
-
-struct timespec;
-struct compat_timespec;
+#include <linux/restart_block.h>
 
 #ifdef CONFIG_THREAD_INFO_IN_TASK
 #define current_thread_info() ((struct thread_info *)current)
 #endif
 
-/*
- * System call restart block.
- */
-struct restart_block {
-	long (*fn)(struct restart_block *);
-	union {
-		/* For futex_wait and futex_wait_requeue_pi */
-		struct {
-			u32 __user *uaddr;
-			u32 val;
-			u32 flags;
-			u32 bitset;
-			u64 time;
-			u32 __user *uaddr2;
-		} futex;
-		/* For nanosleep */
-		struct {
-			clockid_t clockid;
-			struct timespec __user *rmtp;
-#ifdef CONFIG_COMPAT
-			struct compat_timespec __user *compat_rmtp;
-#endif
-			u64 expires;
-		} nanosleep;
-		/* For poll */
-		struct {
-			struct pollfd __user *ufds;
-			int nfds;
-			int has_timeout;
-			unsigned long tv_sec;
-			unsigned long tv_nsec;
-		} poll;
-	};
-};
-
-extern long do_no_restart_syscall(struct restart_block *parm);
-
 #include <linux/bitops.h>
 #include <asm/thread_info.h>
 
-- 
1.9.1

Re: [PATCH 2/3] thread_info: factor out restart_block

[Andy Lutomirski]

On Wed, Oct 19, 2016 at 11:28 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> Since commit f56141e3e2d9aabf ("all arches, signal: move restart_block
> to struct task_struct"), thread_info and restart_block have been
> logically distinct, yet struct restart_block is still defined in
> <linux/thread_info.h>.
>
> At least one architecture (erroneously) uses restart_block as part of
> its thread_info, and thus the definition of restart_block must come
> before the include of <asm/thread_info>. Subsequent patches in this
> series need to shuffle the order of includes and definitions in
> <linux/thread_info.h>, and will make this ordering fragile.
>
> This patch moves the definition of restart_block out to its own header.
> This serves as generic cleanup, logically separating thread_info and
> restart_block, and also makes it easier to avoid fragility.

Looks entirely reasonable to me.

Reviewed-by: Andy Lutomirski <luto@kernel.org>

--Andy

Re: [PATCH 2/3] thread_info: factor out restart_block

[Mark Rutland]

On Wed, Oct 19, 2016 at 04:31:02PM -0700, Andy Lutomirski wrote:
> On Wed, Oct 19, 2016 at 11:28 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > Since commit f56141e3e2d9aabf ("all arches, signal: move restart_block
> > to struct task_struct"), thread_info and restart_block have been
> > logically distinct, yet struct restart_block is still defined in
> > <linux/thread_info.h>.
> >
> > At least one architecture (erroneously) uses restart_block as part of
> > its thread_info, and thus the definition of restart_block must come
> > before the include of <asm/thread_info>. Subsequent patches in this
> > series need to shuffle the order of includes and definitions in
> > <linux/thread_info.h>, and will make this ordering fragile.
> >
> > This patch moves the definition of restart_block out to its own header.
> > This serves as generic cleanup, logically separating thread_info and
> > restart_block, and also makes it easier to avoid fragility.
> 
> Looks entirely reasonable to me.
> 
> Reviewed-by: Andy Lutomirski <luto@kernel.org>

Thanks, that's much appreciated.

Now that Heiko's patch is in -rc2 I'd like to be able to put these two
patches into a stable branch.

Before I do that, would you also be happy to ack/review patch 3?

Thanks,
Mark.

[PATCH 3/3] thread_info: include <current.h> for THREAD_INFO_IN_TASK

[Mark Rutland]

When CONFIG_THREAD_INFO_IN_TASK is selected, the current_thread_info()
macro relies on current having been defined prior to its use. However,
not all users of current_thread_info() include <asm/current.h>, and thus
current is not guaranteed to be defined.

When CONFIG_THREAD_INFO_IN_TASK is not selected, it's possible that
get_current() / current are based upon current_thread_info(), and
<asm/current.h> includes <asm/thread_info.h>. Thus always including
<asm/current.h> would result in circular dependences on some platforms.

To ensure both cases work, this patch includes <asm/current.h>, but only
when CONFIG_THREAD_INFO_IN_TASK is selected.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Kees Cook <keescook@chromium.org> 
---
 include/linux/thread_info.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
index c75c6ab..ef1f4b0 100644
--- a/include/linux/thread_info.h
+++ b/include/linux/thread_info.h
@@ -12,6 +12,7 @@
 #include <linux/restart_block.h>
 
 #ifdef CONFIG_THREAD_INFO_IN_TASK
+#include <asm/current.h>
 #define current_thread_info() ((struct thread_info *)current)
 #endif
 
-- 
1.9.1

Re: [PATCH 3/3] thread_info: include <current.h> for THREAD_INFO_IN_TASK

[Heiko Carstens]

On Wed, Oct 19, 2016 at 07:28:13PM +0100, Mark Rutland wrote:
> When CONFIG_THREAD_INFO_IN_TASK is selected, the current_thread_info()
> macro relies on current having been defined prior to its use. However,
> not all users of current_thread_info() include <asm/current.h>, and thus
> current is not guaranteed to be defined.
> 
> When CONFIG_THREAD_INFO_IN_TASK is not selected, it's possible that
> get_current() / current are based upon current_thread_info(), and
> <asm/current.h> includes <asm/thread_info.h>. Thus always including
> <asm/current.h> would result in circular dependences on some platforms.
> 
> To ensure both cases work, this patch includes <asm/current.h>, but only
> when CONFIG_THREAD_INFO_IN_TASK is selected.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
> Cc: Kees Cook <keescook@chromium.org> 
> ---
>  include/linux/thread_info.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
> index c75c6ab..ef1f4b0 100644
> --- a/include/linux/thread_info.h
> +++ b/include/linux/thread_info.h
> @@ -12,6 +12,7 @@
>  #include <linux/restart_block.h>
>  
>  #ifdef CONFIG_THREAD_INFO_IN_TASK
> +#include <asm/current.h>
>  #define current_thread_info() ((struct thread_info *)current)
>  #endif

FWIW:
Acked-by: Heiko Carstens <heiko.carstens@de.ibm.com>

Re: [PATCH 3/3] thread_info: include <current.h> for THREAD_INFO_IN_TASK

[Mark Rutland]

On Thu, Oct 20, 2016 at 12:29:26PM +0200, Heiko Carstens wrote:
> On Wed, Oct 19, 2016 at 07:28:13PM +0100, Mark Rutland wrote:
> > When CONFIG_THREAD_INFO_IN_TASK is selected, the current_thread_info()
> > macro relies on current having been defined prior to its use. However,
> > not all users of current_thread_info() include <asm/current.h>, and thus
> > current is not guaranteed to be defined.
> > 
> > When CONFIG_THREAD_INFO_IN_TASK is not selected, it's possible that
> > get_current() / current are based upon current_thread_info(), and
> > <asm/current.h> includes <asm/thread_info.h>. Thus always including
> > <asm/current.h> would result in circular dependences on some platforms.
> > 
> > To ensure both cases work, this patch includes <asm/current.h>, but only
> > when CONFIG_THREAD_INFO_IN_TASK is selected.
> > 
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Andy Lutomirski <luto@kernel.org>
> > Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
> > Cc: Kees Cook <keescook@chromium.org> 
> > ---
> >  include/linux/thread_info.h | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
> > index c75c6ab..ef1f4b0 100644
> > --- a/include/linux/thread_info.h
> > +++ b/include/linux/thread_info.h
> > @@ -12,6 +12,7 @@
> >  #include <linux/restart_block.h>
> >  
> >  #ifdef CONFIG_THREAD_INFO_IN_TASK
> > +#include <asm/current.h>
> >  #define current_thread_info() ((struct thread_info *)current)
> >  #endif
> 
> FWIW:
> Acked-by: Heiko Carstens <heiko.carstens@de.ibm.com>

Cheers!

Would you be happy to do the same for patch 2?

Once these have a couple more acks I'll drop these on a (hopefully)
stable branch, tagged so that we can both use them as a base.

Thanks,
Mark.

Re: [PATCH 3/3] thread_info: include <current.h> for THREAD_INFO_IN_TASK

[Andy Lutomirski]

On Wed, Oct 19, 2016 at 11:28 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> When CONFIG_THREAD_INFO_IN_TASK is selected, the current_thread_info()
> macro relies on current having been defined prior to its use. However,
> not all users of current_thread_info() include <asm/current.h>, and thus
> current is not guaranteed to be defined.
>
> When CONFIG_THREAD_INFO_IN_TASK is not selected, it's possible that
> get_current() / current are based upon current_thread_info(), and
> <asm/current.h> includes <asm/thread_info.h>. Thus always including
> <asm/current.h> would result in circular dependences on some platforms.
>
> To ensure both cases work, this patch includes <asm/current.h>, but only
> when CONFIG_THREAD_INFO_IN_TASK is selected.

Reviewed-by: Andy Lutomirski <luto@kernel.org>

although it would be nice if you moved your description of why the
include is conditional into a comment.

--Andy

Re: [PATCH 3/3] thread_info: include <current.h> for THREAD_INFO_IN_TASK

[Mark Rutland]

On Thu, Oct 27, 2016 at 04:13:49PM -0700, Andy Lutomirski wrote:
> On Wed, Oct 19, 2016 at 11:28 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > When CONFIG_THREAD_INFO_IN_TASK is selected, the current_thread_info()
> > macro relies on current having been defined prior to its use. However,
> > not all users of current_thread_info() include <asm/current.h>, and thus
> > current is not guaranteed to be defined.
> >
> > When CONFIG_THREAD_INFO_IN_TASK is not selected, it's possible that
> > get_current() / current are based upon current_thread_info(), and
> > <asm/current.h> includes <asm/thread_info.h>. Thus always including
> > <asm/current.h> would result in circular dependences on some platforms.
> >
> > To ensure both cases work, this patch includes <asm/current.h>, but only
> > when CONFIG_THREAD_INFO_IN_TASK is selected.
> 
> Reviewed-by: Andy Lutomirski <luto@kernel.org>

Cheers!

> although it would be nice if you moved your description of why the
> include is conditional into a comment.

Agreed. I've folded in the below:

#ifdef CONFIG_THREAD_INFO_IN_TASK
/*
 * For CONFIG_THREAD_INFO_IN_TASK kernels we need <asm/current.h> for the
 * definition of current, but for !CONFIG_THREAD_INFO_IN_TASK kernels,
 * including <asm/current.h> can cause a circular dependency on some platforms.
 */
#include <asm/current.h>
#define current_thread_info() ((struct thread_info *)current)
#endif

Thanks,
Mark.

Re: [PATCH 0/3] THREAD_INFO_IN_TASK prep work for arm64+s390

[Mark Rutland]

Hi Andrew,

On Wed, Oct 19, 2016 at 07:28:10PM +0100, Mark Rutland wrote:
> Heiko and I have been working on THREAD_INFO_IN_TASK for s390 and arm64
> respectively, and we're both targetting v4.10.
> 
> These are the common core changes which we both require, which happen to
> touch x86 and some core headers. We'd either need these merged for
> v4.9-rc*, or placed on a stable branch/tag that we can both base atop
> of.
> 
> I've put together a branch [1,2] based on v4.9-rc1, but the patches themselves
> are sorely lacking in relevant acks. Are people willing to help change that? ;)

Would you be happy to ack patches 2 and 3?

Ingo took patch 1 for 4.9-rc2, and these are the only core parts s390
and arm64 need to implement THREAD_INFO_IN_TASK for v4.10.

I'd like to put them on a stable branch.

Thanks,
Mark.

> [1] git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git core/ti-stack-split
> [2] https://git.kernel.org/cgit/linux/kernel/git/mark/linux.git/log/?h=core/ti-stack-split
> 
> Heiko Carstens (1):
>   sched/core,x86: make struct thread_info arch specific again
> 
> Mark Rutland (2):
>   thread_info: factor out restart_block
>   thread_info: include <current.h> for THREAD_INFO_IN_TASK
> 
>  arch/x86/include/asm/thread_info.h |  9 +++++++
>  include/linux/restart_block.h      | 51 ++++++++++++++++++++++++++++++++++++
>  include/linux/thread_info.h        | 53 ++------------------------------------
>  3 files changed, 62 insertions(+), 51 deletions(-)
>  create mode 100644 include/linux/restart_block.h
> 
> -- 
> 1.9.1
> 

[PATCH 0/3] THREAD_INFO_IN_TASK_STRUCT vs generic preemption code

[Heiko Carstens]

Commit c65eacbe290b ("sched/core: Allow putting thread_info into
task_struct") made struct thread_info a generic struct with only a
single flags member if THREAD_INFO_IN_TASK_STRUCT is selected.

This change however seems to be quite x86 centric, since at least the
generic preemption code (asm-generic/preempt.h) assumes that struct
thread_info also has a preempt_count member, which apparently was not
true for x86.

We could add a bit more ifdefs to solve this problem too, but it seems
to be much simpler to make struct thread_info arch specific
again. This also makes the conversion to THREAD_INFO_IN_TASK_STRUCT a
bit easier for architectures that have a couple of arch specific stuff
in their thread_info definition.

The arch specific stuff _could_ be moved to thread_struct. However
keeping them in thread_info makes it easier: accessing thread_info
members is simple, since it is at the beginning of the task_struct,
while the thread_struct is at the end. At least on s390 the offsets
needed to access members of the thread_struct (with task_struct as
base) are too large for various asm instructions.  This is not a
problem when keeping these members within thread_info.

The above is actually the same as the description of the first
patch. The second patch is a simple compile fix and the third one the
s390 conversion to THREAD_INFO_IN_TASK_STRUCT.

Heiko Carstens (3):
  sched/core,x86: make struct thread_info arch specific again
  sched/preempt: include asm/current.h
  s390: move thread_info into task_struct

 arch/s390/Kconfig                   |  1 +
 arch/s390/include/asm/lowcore.h     |  2 +-
 arch/s390/include/asm/thread_info.h | 11 --------
 arch/s390/kernel/asm-offsets.c      | 17 +++++--------
 arch/s390/kernel/entry.S            | 51 ++++++++++++++++++-------------------
 arch/s390/kernel/head64.S           |  5 ++--
 arch/s390/kernel/setup.c            |  3 +--
 arch/s390/kernel/smp.c              |  1 -
 arch/x86/include/asm/thread_info.h  |  9 +++++++
 include/asm-generic/preempt.h       |  1 +
 include/linux/thread_info.h         | 11 --------
 11 files changed, 47 insertions(+), 65 deletions(-)

-- 
2.8.4

[PATCH 1/3] sched/core,x86: make struct thread_info arch specific again

[Heiko Carstens]

commit c65eacbe290b ("sched/core: Allow putting thread_info into
task_struct") made struct thread_info a generic struct with only a
single flags member if THREAD_INFO_IN_TASK_STRUCT is selected.

This change however seems to be quite x86 centric, since at least the
generic preemption code (asm-generic/preempt.h) assumes that struct
thread_info also has a preempt_count member, which apparently was not
true for x86.

We could add a bit more ifdefs to solve this problem too, but it seems
to be much simpler to make struct thread_info arch specific
again. This also makes the conversion to THREAD_INFO_IN_TASK_STRUCT a
bit easier for architectures that have a couple of arch specific stuff
in their thread_info definition.

The arch specific stuff _could_ be moved to thread_struct. However
keeping them in thread_info makes it easier: accessing thread_info
members is simple, since it is at the beginning of the task_struct,
while the thread_struct is at the end. At least on s390 the offsets
needed to access members of the thread_struct (with task_struct as
base) are too large for various asm instructions.  This is not a
problem when keeping these members within thread_info.

Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
 arch/x86/include/asm/thread_info.h |  9 +++++++++
 include/linux/thread_info.h        | 11 -----------
 2 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
index 2aaca53c0974..ad6f5eb07a95 100644
--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -52,6 +52,15 @@ struct task_struct;
 #include <asm/cpufeature.h>
 #include <linux/atomic.h>
 
+struct thread_info {
+	unsigned long		flags;		/* low level flags */
+};
+
+#define INIT_THREAD_INFO(tsk)			\
+{						\
+	.flags		= 0,			\
+}
+
 #define init_stack		(init_thread_union.stack)
 
 #else /* !__ASSEMBLY__ */
diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
index 45f004e9cc59..2873baf5372a 100644
--- a/include/linux/thread_info.h
+++ b/include/linux/thread_info.h
@@ -14,17 +14,6 @@ struct timespec;
 struct compat_timespec;
 
 #ifdef CONFIG_THREAD_INFO_IN_TASK
-struct thread_info {
-	unsigned long		flags;		/* low level flags */
-};
-
-#define INIT_THREAD_INFO(tsk)			\
-{						\
-	.flags		= 0,			\
-}
-#endif
-
-#ifdef CONFIG_THREAD_INFO_IN_TASK
 #define current_thread_info() ((struct thread_info *)current)
 #endif
 
-- 
2.8.4

Re: [PATCH 1/3] sched/core,x86: make struct thread_info arch specific again

[Mark Rutland]

Hi,

On Thu, Oct 13, 2016 at 01:57:10PM +0200, Heiko Carstens wrote:
> commit c65eacbe290b ("sched/core: Allow putting thread_info into
> task_struct") made struct thread_info a generic struct with only a
> single flags member if THREAD_INFO_IN_TASK_STRUCT is selected.
> 
> This change however seems to be quite x86 centric, since at least the
> generic preemption code (asm-generic/preempt.h) assumes that struct
> thread_info also has a preempt_count member, which apparently was not
> true for x86.
> 
> We could add a bit more ifdefs to solve this problem too, but it seems
> to be much simpler to make struct thread_info arch specific
> again. This also makes the conversion to THREAD_INFO_IN_TASK_STRUCT a
> bit easier for architectures that have a couple of arch specific stuff
> in their thread_info definition.
> 
> The arch specific stuff _could_ be moved to thread_struct. However
> keeping them in thread_info makes it easier: accessing thread_info
> members is simple, since it is at the beginning of the task_struct,
> while the thread_struct is at the end. At least on s390 the offsets
> needed to access members of the thread_struct (with task_struct as
> base) are too large for various asm instructions.  This is not a
> problem when keeping these members within thread_info.

The exact same applies for arm64 on all counts. This is also simpler than both
attempts I had at this, so FWIW:

Acked-by: Mark Rutland <mark.rutland@arm.com>

To make merging less painful, I guess we'll need a stable branch with (just)
this and whatever patch we end up with for fixing current_thread_info(), so we
can independently merge the arch-specific parts.

I guess it'd make sense for the tip tree to host that?

Thanks,
Mark.

> Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> ---
>  arch/x86/include/asm/thread_info.h |  9 +++++++++
>  include/linux/thread_info.h        | 11 -----------
>  2 files changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
> index 2aaca53c0974..ad6f5eb07a95 100644
> --- a/arch/x86/include/asm/thread_info.h
> +++ b/arch/x86/include/asm/thread_info.h
> @@ -52,6 +52,15 @@ struct task_struct;
>  #include <asm/cpufeature.h>
>  #include <linux/atomic.h>
>  
> +struct thread_info {
> +	unsigned long		flags;		/* low level flags */
> +};
> +
> +#define INIT_THREAD_INFO(tsk)			\
> +{						\
> +	.flags		= 0,			\
> +}
> +
>  #define init_stack		(init_thread_union.stack)
>  
>  #else /* !__ASSEMBLY__ */
> diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
> index 45f004e9cc59..2873baf5372a 100644
> --- a/include/linux/thread_info.h
> +++ b/include/linux/thread_info.h
> @@ -14,17 +14,6 @@ struct timespec;
>  struct compat_timespec;
>  
>  #ifdef CONFIG_THREAD_INFO_IN_TASK
> -struct thread_info {
> -	unsigned long		flags;		/* low level flags */
> -};
> -
> -#define INIT_THREAD_INFO(tsk)			\
> -{						\
> -	.flags		= 0,			\
> -}
> -#endif
> -
> -#ifdef CONFIG_THREAD_INFO_IN_TASK
>  #define current_thread_info() ((struct thread_info *)current)
>  #endif
>  
> -- 
> 2.8.4
> 

Re: [PATCH 1/3] sched/core,x86: make struct thread_info arch specific again

[Andy Lutomirski]

On Thu, Oct 13, 2016 at 4:41 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi,
>
> On Thu, Oct 13, 2016 at 01:57:10PM +0200, Heiko Carstens wrote:
>> commit c65eacbe290b ("sched/core: Allow putting thread_info into
>> task_struct") made struct thread_info a generic struct with only a
>> single flags member if THREAD_INFO_IN_TASK_STRUCT is selected.
>>
>> This change however seems to be quite x86 centric, since at least the
>> generic preemption code (asm-generic/preempt.h) assumes that struct
>> thread_info also has a preempt_count member, which apparently was not
>> true for x86.
>>
>> We could add a bit more ifdefs to solve this problem too, but it seems
>> to be much simpler to make struct thread_info arch specific
>> again. This also makes the conversion to THREAD_INFO_IN_TASK_STRUCT a
>> bit easier for architectures that have a couple of arch specific stuff
>> in their thread_info definition.
>>
>> The arch specific stuff _could_ be moved to thread_struct. However
>> keeping them in thread_info makes it easier: accessing thread_info
>> members is simple, since it is at the beginning of the task_struct,
>> while the thread_struct is at the end. At least on s390 the offsets
>> needed to access members of the thread_struct (with task_struct as
>> base) are too large for various asm instructions.  This is not a
>> problem when keeping these members within thread_info.
>
> The exact same applies for arm64 on all counts. This is also simpler than both
> attempts I had at this, so FWIW:
>
> Acked-by: Mark Rutland <mark.rutland@arm.com>
>
> To make merging less painful, I guess we'll need a stable branch with (just)
> this and whatever patch we end up with for fixing current_thread_info(), so we
> can independently merge the arch-specific parts.
>
> I guess it'd make sense for the tip tree to host that?
>

I wonder if this could even make 4.9.  It's pretty clearly a no-op.  Ingo?