| Message ID | 1473947349-14521-3-git-send-email-mark.rutland@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 15, 2016 at 6:49 AM, Mark Rutland <mark.rutland@arm.com> wrote: > Currently, task_struct is defined in <linux/sched.h>, which (indirectly) > pulls in a number of low-level arch headers such as <asm/preempt.h> > through a number of other headers. Thus, code and structures in these > headers can't rely on the definition of task_struct. Some of these > headers are necessary for the definition of task_struct, so moving > task_struct into its own header is insufficient tio avoid circular > includes. The flippant answer is to fix the headers, but I tried that myself and gave up :( But how about this slightly less duplicative alternative: struct thread_info { #ifdef arch_thread_info struct arch_thread_info arch_ti; #endif };
On Thu, Sep 15, 2016 at 11:37:47AM -0700, Andy Lutomirski wrote: > On Thu, Sep 15, 2016 at 6:49 AM, Mark Rutland <mark.rutland@arm.com> wrote: > > Currently, task_struct is defined in <linux/sched.h>, which (indirectly) > > pulls in a number of low-level arch headers such as <asm/preempt.h> > > through a number of other headers. Thus, code and structures in these > > headers can't rely on the definition of task_struct. Some of these > > headers are necessary for the definition of task_struct, so moving > > task_struct into its own header is insufficient tio avoid circular > > includes. > > The flippant answer is to fix the headers, but I tried that myself and > gave up :( Agreed; likewise (though I gave up quicker, I suspect). :( Longer-term I'd still hope that we can do this. > But how about this slightly less duplicative alternative: > > struct thread_info { > #ifdef arch_thread_info > struct arch_thread_info arch_ti; > #endif > }; I'm happy to have an arch_thread_info. Just to check, what do you mean to happen with the flags field? Should that always be in the generic thread_info? e.g. struct thread_info { u32 flags; #ifdef arch_thread_info struct arch_thread_info arch_ti; #endif }; Thanks, Mark,
> On Thu, Sep 15, 2016 at 11:37:47AM -0700, Andy Lutomirski wrote: > > On Thu, Sep 15, 2016 at 6:49 AM, Mark Rutland <mark.rutland@arm.com> wrote: > > > Currently, task_struct is defined in <linux/sched.h>, which (indirectly) > > > pulls in a number of low-level arch headers such as <asm/preempt.h> > > > through a number of other headers. Thus, code and structures in these > > > headers can't rely on the definition of task_struct. Some of these > > > headers are necessary for the definition of task_struct, so moving > > > task_struct into its own header is insufficient tio avoid circular > > > includes. > > > > The flippant answer is to fix the headers, but I tried that myself and > > gave up :( > > Agreed; likewise (though I gave up quicker, I suspect). :( > > Longer-term I'd still hope that we can do this. > > > But how about this slightly less duplicative alternative: > > > > struct thread_info { > > #ifdef arch_thread_info > > struct arch_thread_info arch_ti; > > #endif > > }; > > I'm happy to have an arch_thread_info. > > Just to check, what do you mean to happen with the flags field? Should > that always be in the generic thread_info? e.g. > > struct thread_info { > u32 flags; > #ifdef arch_thread_info > struct arch_thread_info arch_ti; > #endif > }; Exactly. Possibly with a comment that using thread_struct should be preferred and that arch_thread_info should be used only if some header file requires access via current_thread_info() or task_thread_info(). --Andy > > Thanks, > Mark,
Hi Andy, On Fri, Sep 16, 2016 at 08:11:14AM -0700, Andy Lutomirski wrote: > > On Thu, Sep 15, 2016 at 11:37:47AM -0700, Andy Lutomirski wrote: > > Just to check, what do you mean to happen with the flags field? Should > > that always be in the generic thread_info? e.g. > > > > struct thread_info { > > u32 flags; > > #ifdef arch_thread_info > > struct arch_thread_info arch_ti; > > #endif > > }; > > Exactly. Possibly with a comment that using thread_struct should be > preferred and that arch_thread_info should be used only if some header > file requires access via current_thread_info() or task_thread_info(). While fixing up these patches, I realised that I'm somewhat concerned by flags becoming a u32 (where it was previously an unsigned long for arm64). The generic {test,set,*}_ti_thread_flag() helpers use the usual bitops, which perform accesses of sizeof(unsigned long) at a time, and for arm64 these need to be naturally-aligned. We happen to get that alignment from subsequent fields in task_struct and/or thread_info, and for arm64 we don't seem to have a problem with tearing, but it feels somewhat fragile, and leaves me uneasy. Looking at the git log, it seems that x86 also use unsigned long until commit affa219b60a11b32 ("x86: change thread_info's flag field back to 32 bits"), where if I'm reading correctly, this was done to get rid of unnecessary padding. With THREAD_INFO_IN_STACK, thread_info::flags is immediately followed by a long on x86, so we save no padding. Given all that, can we make the generic thread_info::flags an unsigned long, matching what the thread flag helpers implicitly assume? Thanks, Mark.
On Sep 21, 2016 12:28 AM, "Mark Rutland" <mark.rutland@arm.com> wrote: > > Hi Andy, > > On Fri, Sep 16, 2016 at 08:11:14AM -0700, Andy Lutomirski wrote: > > > On Thu, Sep 15, 2016 at 11:37:47AM -0700, Andy Lutomirski wrote: > > > Just to check, what do you mean to happen with the flags field? Should > > > that always be in the generic thread_info? e.g. > > > > > > struct thread_info { > > > u32 flags; > > > #ifdef arch_thread_info > > > struct arch_thread_info arch_ti; > > > #endif > > > }; > > > > Exactly. Possibly with a comment that using thread_struct should be > > preferred and that arch_thread_info should be used only if some header > > file requires access via current_thread_info() or task_thread_info(). > > While fixing up these patches, I realised that I'm somewhat concerned by > flags becoming a u32 (where it was previously an unsigned long for > arm64). > > The generic {test,set,*}_ti_thread_flag() helpers use the usual bitops, > which perform accesses of sizeof(unsigned long) at a time, and for arm64 > these need to be naturally-aligned. > > We happen to get that alignment from subsequent fields in task_struct > and/or thread_info, and for arm64 we don't seem to have a problem with > tearing, but it feels somewhat fragile, and leaves me uneasy. > > Looking at the git log, it seems that x86 also use unsigned long until > commit affa219b60a11b32 ("x86: change thread_info's flag field back to > 32 bits"), where if I'm reading correctly, this was done to get rid of > unnecessary padding. With THREAD_INFO_IN_STACK, thread_info::flags is > immediately followed by a long on x86, so we save no padding. > > Given all that, can we make the generic thread_info::flags an unsigned > long, matching what the thread flag helpers implicitly assume? > Yes. Want to send the patch or should I? --Andy
On Thu, Sep 22, 2016 at 03:23:59PM -0700, Andy Lutomirski wrote: > On Sep 21, 2016 12:28 AM, "Mark Rutland" <mark.rutland@arm.com> wrote: > > Given all that, can we make the generic thread_info::flags an unsigned > > long, matching what the thread flag helpers implicitly assume? > > Yes. Want to send the patch or should I? I've sent a patch out to LKML [1], though if that doesn't look right I'm more than happy for you to send a correct one. ;) Thanks, Mark. [1] http://lkml.kernel.org/r/1474651447-30447-1-git-send-email-mark.rutland@arm.com
diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h index d9622f7..7984f87 100644 --- a/include/linux/thread_info.h +++ b/include/linux/thread_info.h @@ -13,7 +13,8 @@ struct timespec; struct compat_timespec; -#ifdef CONFIG_THREAD_INFO_IN_TASK +#if defined(CONFIG_THREAD_INFO_IN_TASK) && \ + !defined(CONFIG_ARCH_HAS_OWN_THREAD_INFO) struct thread_info { u32 flags; /* low level flags */ }; diff --git a/init/Kconfig b/init/Kconfig index 3b9a47f..f812098 100644 --- a/init/Kconfig +++ b/init/Kconfig @@ -26,6 +26,9 @@ config IRQ_WORK config BUILDTIME_EXTABLE_SORT bool +config ARCH_HAS_OWN_THREAD_INFO + bool + config THREAD_INFO_IN_TASK bool help
Currently, task_struct is defined in <linux/sched.h>, which (indirectly) pulls in a number of low-level arch headers such as <asm/preempt.h> through a number of other headers. Thus, code and structures in these headers can't rely on the definition of task_struct. Some of these headers are necessary for the definition of task_struct, so moving task_struct into its own header is insufficient tio avoid circular includes. With CONFIG_THREAD_INFO_IN_TASK, arch code needs to implement its own get_current() for the generic get_thread_info(). To avoid header dependency issues, this relies on thread_info being the first member of task_struct. This can be used by low-level arch code, as it doesn't depend on the definition of task_struct. For architectures without preempt-safe this_cpu ops, some data required by low-level arch code (e.g. preempt_count) has to be stored per-thread, and for the reasons above, we cannot place this in task_struct (or thread_struct, since we cannot know its offset from within task_struct). The only practical location for these is thread_info. This patch allows architectures with CONFIG_ARCH_HAS_OWN_THREAD_INFO to define their own thread_info, avoiding the problems described above. Signed-off-by: Mark Rutland <mark.rutland@arm.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Andy Lutomirski <luto@kernel.org> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Kees Cook <keescook@chromium.org> Cc: Will Deacon <will.deacon@arm.com> Cc: linux-kernel@vger.kernel.org --- include/linux/thread_info.h | 3 ++- init/Kconfig | 3 +++ 2 files changed, 5 insertions(+), 1 deletion(-)