| Message ID | 20210914121036.3975026-6-ardb@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Move task_struct::cpu back into thread_info | expand |
On Tue, Sep 14, 2021 at 5:11 AM Ard Biesheuvel <ardb@kernel.org> wrote: > > static inline unsigned int task_cpu(const struct task_struct *p) > { > #ifdef CONFIG_THREAD_INFO_IN_TASK > - return READ_ONCE(p->cpu); > + return READ_ONCE(p->thread_info.cpu); > #else > return READ_ONCE(task_thread_info(p)->cpu); > #endif Those two lines look different, but aren't. Please just remove the CONFIG_THREAD_INFO_IN_TASK conditional, and use return READ_ONCE(task_thread_info(p)->cpu); unconditionally, which now does the right thing regardless. Linus
On Tue, 14 Sept 2021 at 17:49, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Tue, Sep 14, 2021 at 5:11 AM Ard Biesheuvel <ardb@kernel.org> wrote: > > > > static inline unsigned int task_cpu(const struct task_struct *p) > > { > > #ifdef CONFIG_THREAD_INFO_IN_TASK > > - return READ_ONCE(p->cpu); > > + return READ_ONCE(p->thread_info.cpu); > > #else > > return READ_ONCE(task_thread_info(p)->cpu); > > #endif > > Those two lines look different, but aren't. > > Please just remove the CONFIG_THREAD_INFO_IN_TASK conditional, and use > > return READ_ONCE(task_thread_info(p)->cpu); > > unconditionally, which now does the right thing regardless. > Unfortunately not. task_cpu() takes a 'const struct task_struct *', whereas task_thread_info() takes a 'struct task_struct *'. Since task_thread_info()-><foo> is widely used as an lvalue, I would need to update task_cpu()'s prototype and fix up all the callers, some of which take the const flavor themselves. Or introduce 'const_task_thread_info()' which takes the const flavor, and cannot be used to instantiate lvalues. Suggestions welcome, but this is the cleanest I could come up with.
On Tue, Sep 14, 2021 at 8:53 AM Ard Biesheuvel <ardb@kernel.org> wrote: > > task_cpu() takes a 'const struct task_struct *', whereas > task_thread_info() takes a 'struct task_struct *'. Oh, annoying, but that's easily fixed. Just make that static inline struct thread_info *task_thread_info(struct task_struct *task) .. be a simple #define task_thread_info(tsk) (&(tsk)->thread_info) instead. That actually then matches the !THREAD_INFO_IN_TASK case anyway. Make the commit comment be about how that fixes the type problem. Because while in many cases inline functions are superior to macros, it clearly isn't the case in this case. Linus
On Tue, 14 Sept 2021 at 17:59, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Tue, Sep 14, 2021 at 8:53 AM Ard Biesheuvel <ardb@kernel.org> wrote: > > > > task_cpu() takes a 'const struct task_struct *', whereas > > task_thread_info() takes a 'struct task_struct *'. > > Oh, annoying, but that's easily fixed. Just make that > > static inline struct thread_info *task_thread_info(struct > task_struct *task) .. > > be a simple > > #define task_thread_info(tsk) (&(tsk)->thread_info) > > instead. That actually then matches the !THREAD_INFO_IN_TASK case anyway. > > Make the commit comment be about how that fixes the type problem. > > Because while in many cases inline functions are superior to macros, > it clearly isn't the case in this case. > Works for me.
On Tue, Sep 14, 2021 at 02:10:33PM +0200, Ard Biesheuvel wrote: > THREAD_INFO_IN_TASK moved the CPU field out of thread_info, but this > causes some issues on architectures that define raw_smp_processor_id() > in terms of this field, due to the fact that #include'ing linux/sched.h > to get at struct task_struct is problematic in terms of circular > dependencies. > > Given that thread_info and task_struct are the same data structure > anyway when THREAD_INFO_IN_TASK=y, let's move it back so that having > access to the type definition of struct thread_info is sufficient to > reference the CPU number of the current task. > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c index cee9f3e9f906..0bfc048221af 100644 --- a/arch/arm64/kernel/asm-offsets.c +++ b/arch/arm64/kernel/asm-offsets.c @@ -27,7 +27,6 @@ int main(void) { DEFINE(TSK_ACTIVE_MM, offsetof(struct task_struct, active_mm)); - DEFINE(TSK_CPU, offsetof(struct task_struct, cpu)); BLANK(); DEFINE(TSK_TI_CPU, offsetof(struct task_struct, thread_info.cpu)); DEFINE(TSK_TI_FLAGS, offsetof(struct task_struct, thread_info.flags)); diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S index 17962452e31d..6a98f1a38c29 100644 --- a/arch/arm64/kernel/head.S +++ b/arch/arm64/kernel/head.S @@ -412,7 +412,7 @@ SYM_FUNC_END(__create_page_tables) scs_load \tsk adr_l \tmp1, __per_cpu_offset - ldr w\tmp2, [\tsk, #TSK_CPU] + ldr w\tmp2, [\tsk, #TSK_TI_CPU] ldr \tmp1, [\tmp1, \tmp2, lsl #3] set_this_cpu_offset \tmp1 .endm diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c index e563d3222d69..e37e4546034e 100644 --- a/arch/powerpc/kernel/asm-offsets.c +++ b/arch/powerpc/kernel/asm-offsets.c @@ -93,7 +93,7 @@ int main(void) #endif /* CONFIG_PPC64 */ OFFSET(TASK_STACK, task_struct, stack); #ifdef CONFIG_SMP - OFFSET(TASK_CPU, task_struct, cpu); + OFFSET(TASK_CPU, task_struct, thread_info.cpu); #endif #ifdef CONFIG_LIVEPATCH diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c index 9cc7d3dbf439..512d875b45e0 100644 --- a/arch/powerpc/kernel/smp.c +++ b/arch/powerpc/kernel/smp.c @@ -1223,7 +1223,7 @@ static void cpu_idle_thread_init(unsigned int cpu, struct task_struct *idle) paca_ptrs[cpu]->kstack = (unsigned long)task_stack_page(idle) + THREAD_SIZE - STACK_FRAME_OVERHEAD; #endif - idle->cpu = cpu; + task_thread_info(idle)->cpu = cpu; secondary_current = current_set[cpu] = idle; } diff --git a/include/linux/sched.h b/include/linux/sched.h index e12b524426b0..37aa521078e7 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -750,10 +750,6 @@ struct task_struct { #ifdef CONFIG_SMP int on_cpu; struct __call_single_node wake_entry; -#ifdef CONFIG_THREAD_INFO_IN_TASK - /* Current CPU: */ - unsigned int cpu; -#endif unsigned int wakee_flips; unsigned long wakee_flip_decay_ts; struct task_struct *last_wakee; @@ -2114,7 +2110,7 @@ static __always_inline bool need_resched(void) static inline unsigned int task_cpu(const struct task_struct *p) { #ifdef CONFIG_THREAD_INFO_IN_TASK - return READ_ONCE(p->cpu); + return READ_ONCE(p->thread_info.cpu); #else return READ_ONCE(task_thread_info(p)->cpu); #endif diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h index 3d3e5793e117..79fcbad11450 100644 --- a/kernel/sched/sched.h +++ b/kernel/sched/sched.h @@ -1926,11 +1926,7 @@ static inline void __set_task_cpu(struct task_struct *p, unsigned int cpu) * per-task data have been completed by this moment. */ smp_wmb(); -#ifdef CONFIG_THREAD_INFO_IN_TASK - WRITE_ONCE(p->cpu, cpu); -#else WRITE_ONCE(task_thread_info(p)->cpu, cpu); -#endif p->wake_cpu = cpu; #endif }
THREAD_INFO_IN_TASK moved the CPU field out of thread_info, but this causes some issues on architectures that define raw_smp_processor_id() in terms of this field, due to the fact that #include'ing linux/sched.h to get at struct task_struct is problematic in terms of circular dependencies. Given that thread_info and task_struct are the same data structure anyway when THREAD_INFO_IN_TASK=y, let's move it back so that having access to the type definition of struct thread_info is sufficient to reference the CPU number of the current task. Signed-off-by: Ard Biesheuvel <ardb@kernel.org> --- arch/arm64/kernel/asm-offsets.c | 1 - arch/arm64/kernel/head.S | 2 +- arch/powerpc/kernel/asm-offsets.c | 2 +- arch/powerpc/kernel/smp.c | 2 +- include/linux/sched.h | 6 +----- kernel/sched/sched.h | 4 ---- 6 files changed, 4 insertions(+), 13 deletions(-)