Re: [RFC PATCH 5/8] sched: move CPU field back into thread_info if THREAD_INFO_IN_TASK=y
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 Acked-by: Catalin Marinas
Re: [RFC PATCH 5/8] sched: move CPU field back into thread_info if THREAD_INFO_IN_TASK=y
On Tue, 14 Sept 2021 at 17:59, Linus Torvalds wrote: > > On Tue, Sep 14, 2021 at 8:53 AM Ard Biesheuvel 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.
Re: [RFC PATCH 5/8] sched: move CPU field back into thread_info if THREAD_INFO_IN_TASK=y
On Tue, Sep 14, 2021 at 8:53 AM Ard Biesheuvel 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
Re: [RFC PATCH 5/8] sched: move CPU field back into thread_info if THREAD_INFO_IN_TASK=y
On Tue, 14 Sept 2021 at 17:49, Linus Torvalds
wrote:
>
> On Tue, Sep 14, 2021 at 5:11 AM Ard Biesheuvel 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()-> 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.
Re: [RFC PATCH 5/8] sched: move CPU field back into thread_info if THREAD_INFO_IN_TASK=y
On Tue, Sep 14, 2021 at 5:11 AM Ard Biesheuvel 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
[RFC PATCH 5/8] sched: move CPU field back into thread_info if THREAD_INFO_IN_TASK=y
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
---
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(-)
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 intcpu;
-#endif
unsigned intwakee_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
}
--
2.30.2
