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

Message ID 1476901693-8492-2-git-send-email-mark.rutland@arm.com
State New
Headers show

Commit Message

Mark Rutland Oct. 19, 2016, 6:28 p.m.
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(-)

-- 
1.9.1

Comments

Andy Lutomirski Oct. 19, 2016, 11:19 p.m. | #1
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
Ingo Molnar Oct. 20, 2016, 6:40 a.m. | #2
* 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
Mark Rutland Oct. 20, 2016, 9:33 a.m. | #3
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.

Patch

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