Commit c65eacbe290b ("sched/core: Allow putting thread_info into
task_struct") mentions that, along with moving thread_info into
task_struct, the cpu field is moved out of the former into the latter,
but does not explain why.
While collaborating with Keith on adding THREAD_INFO_IN_TASK support to
ARM, we noticed that keeping CPU in task_struct is problematic for
architectures that define raw_smp_processor_id() in terms of this field,
as it requires linux/sched.h to be included, which causes a lot of pain
in terms of circular dependencies (or 'header soup', as the original
commit refers to it).
For examples of how existing architectures work around this, please
refer to patches #6 or #7. In the former case, it uses an awful
asm-offsets hack to index thread_info/current without using its type
definition. The latter approach simply keeps a copy of the task_struct
CPU field in thread_info, and keeps it in sync at context switch time.
Patch #8 reverts this latter approach for ARM, but this code is still
under review so it does not currently apply to mainline.
We also discussed introducing yet another Kconfig symbol to indicate
that the arch has THREAD_INFO_IN_TASK enabled but still prefers to keep
its CPU field in thread_info, but simply keeping it in thread_info in
all cases seems to be the cleanest approach here.
Cc: Keith Packard <[email protected]>
Cc: Russell King <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Michael Ellerman <[email protected]>
Cc: Benjamin Herrenschmidt <[email protected]>
Cc: Christophe Leroy <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Paul Walmsley <[email protected]>
Cc: Palmer Dabbelt <[email protected]>
Cc: Albert Ou <[email protected]>
Cc: Heiko Carstens <[email protected]>
Cc: Vasily Gorbik <[email protected]>
Cc: Christian Borntraeger <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Ard Biesheuvel (8):
arm64: add CPU field to struct thread_info
x86: add CPU field to struct thread_info
s390: add CPU field to struct thread_info
powerpc: add CPU field to struct thread_info
sched: move CPU field back into thread_info if THREAD_INFO_IN_TASK=y
powerpc: smp: remove hack to obtain offset of task_struct::cpu
riscv: rely on core code to keep thread_info::cpu updated
ARM: rely on core code to keep thread_info::cpu updated
arch/arm/include/asm/switch_to.h | 14 --------------
arch/arm/kernel/smp.c | 3 ---
arch/arm64/include/asm/thread_info.h | 1 +
arch/arm64/kernel/asm-offsets.c | 2 +-
arch/arm64/kernel/head.S | 2 +-
arch/powerpc/Makefile | 11 -----------
arch/powerpc/include/asm/smp.h | 17 +----------------
arch/powerpc/include/asm/thread_info.h | 3 +++
arch/powerpc/kernel/asm-offsets.c | 4 +---
arch/powerpc/kernel/smp.c | 2 +-
arch/riscv/kernel/asm-offsets.c | 1 -
arch/riscv/kernel/entry.S | 5 -----
arch/riscv/kernel/head.S | 1 -
arch/s390/include/asm/thread_info.h | 1 +
arch/x86/include/asm/thread_info.h | 3 +++
include/linux/sched.h | 6 +-----
kernel/sched/sched.h | 4 ----
17 files changed, 14 insertions(+), 66 deletions(-)
--
2.30.2
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 <[email protected]>
---
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 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
}
--
2.30.2
The CPU field will be moved back into thread_info even when
THREAD_INFO_IN_TASK is enabled, so add it back to arm64's definition of
struct thread_info.
Signed-off-by: Ard Biesheuvel <[email protected]>
---
arch/arm64/include/asm/thread_info.h | 1 +
arch/arm64/kernel/asm-offsets.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
index 6623c99f0984..c02bc8c183c3 100644
--- a/arch/arm64/include/asm/thread_info.h
+++ b/arch/arm64/include/asm/thread_info.h
@@ -42,6 +42,7 @@ struct thread_info {
void *scs_base;
void *scs_sp;
#endif
+ u32 cpu;
};
#define thread_saved_pc(tsk) \
diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
index 551427ae8cc5..cee9f3e9f906 100644
--- a/arch/arm64/kernel/asm-offsets.c
+++ b/arch/arm64/kernel/asm-offsets.c
@@ -29,6 +29,7 @@ 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));
DEFINE(TSK_TI_PREEMPT, offsetof(struct task_struct, thread_info.preempt_count));
#ifdef CONFIG_ARM64_SW_TTBR0_PAN
--
2.30.2
Now that the core code switched back to using thread_info::cpu to keep
a task's CPU number, we no longer need to keep it in sync explicitly. So
just drop the code that does this.
Signed-off-by: Ard Biesheuvel <[email protected]>
---
arch/riscv/kernel/asm-offsets.c | 1 -
arch/riscv/kernel/entry.S | 5 -----
arch/riscv/kernel/head.S | 1 -
3 files changed, 7 deletions(-)
diff --git a/arch/riscv/kernel/asm-offsets.c b/arch/riscv/kernel/asm-offsets.c
index 90f8ce64fa6f..478d9f02dab5 100644
--- a/arch/riscv/kernel/asm-offsets.c
+++ b/arch/riscv/kernel/asm-offsets.c
@@ -33,7 +33,6 @@ void asm_offsets(void)
OFFSET(TASK_TI_PREEMPT_COUNT, task_struct, thread_info.preempt_count);
OFFSET(TASK_TI_KERNEL_SP, task_struct, thread_info.kernel_sp);
OFFSET(TASK_TI_USER_SP, task_struct, thread_info.user_sp);
- OFFSET(TASK_TI_CPU, task_struct, thread_info.cpu);
OFFSET(TASK_THREAD_F0, task_struct, thread.fstate.f[0]);
OFFSET(TASK_THREAD_F1, task_struct, thread.fstate.f[1]);
diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
index 98f502654edd..459eb1714353 100644
--- a/arch/riscv/kernel/entry.S
+++ b/arch/riscv/kernel/entry.S
@@ -544,11 +544,6 @@ ENTRY(__switch_to)
REG_L s9, TASK_THREAD_S9_RA(a4)
REG_L s10, TASK_THREAD_S10_RA(a4)
REG_L s11, TASK_THREAD_S11_RA(a4)
- /* Swap the CPU entry around. */
- lw a3, TASK_TI_CPU(a0)
- lw a4, TASK_TI_CPU(a1)
- sw a3, TASK_TI_CPU(a1)
- sw a4, TASK_TI_CPU(a0)
/* The offset of thread_info in task_struct is zero. */
move tp, a1
ret
diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
index fce5184b22c3..d5ec30ef6f5d 100644
--- a/arch/riscv/kernel/head.S
+++ b/arch/riscv/kernel/head.S
@@ -317,7 +317,6 @@ clear_bss_done:
call setup_trap_vector
/* Restore C environment */
la tp, init_task
- sw zero, TASK_TI_CPU(tp)
la sp, init_thread_union + THREAD_SIZE
#ifdef CONFIG_KASAN
--
2.30.2
Now that the core code switched back to using thread_info::cpu to keep
a task's CPU number, we no longer need to keep it in sync explicitly. So
just drop the code that does this.
Signed-off-by: Ard Biesheuvel <[email protected]>
---
This patch applies onto [0], which we hope to get merged for v5.16
[0] https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=arm32-ti-in-task-v5
arch/arm/include/asm/switch_to.h | 14 --------------
arch/arm/kernel/smp.c | 3 ---
2 files changed, 17 deletions(-)
diff --git a/arch/arm/include/asm/switch_to.h b/arch/arm/include/asm/switch_to.h
index db2be1f6550d..61e4a3c4ca6e 100644
--- a/arch/arm/include/asm/switch_to.h
+++ b/arch/arm/include/asm/switch_to.h
@@ -23,23 +23,9 @@
*/
extern struct task_struct *__switch_to(struct task_struct *, struct thread_info *, struct thread_info *);
-static inline void set_ti_cpu(struct task_struct *p)
-{
-#ifdef CONFIG_THREAD_INFO_IN_TASK
- /*
- * The core code no longer maintains the thread_info::cpu field once
- * CONFIG_THREAD_INFO_IN_TASK is in effect, but we rely on it for
- * raw_smp_processor_id(), which cannot access struct task_struct*
- * directly for reasons of circular #inclusion hell.
- */
- task_thread_info(p)->cpu = p->cpu;
-#endif
-}
-
#define switch_to(prev,next,last) \
do { \
__complete_pending_tlbi(); \
- set_ti_cpu(next); \
if (IS_ENABLED(CONFIG_CURRENT_POINTER_IN_TPIDRURO)) \
__this_cpu_write(__entry_task, next); \
last = __switch_to(prev,task_thread_info(prev), task_thread_info(next)); \
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index cde5b6d8bac5..97ee6b1567e9 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -154,9 +154,6 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle)
secondary_data.swapper_pg_dir = get_arch_pgd(swapper_pg_dir);
#endif
secondary_data.task = idle;
- if (IS_ENABLED(CONFIG_THREAD_INFO_IN_TASK))
- task_thread_info(idle)->cpu = cpu;
-
sync_cache_w(&secondary_data);
/*
--
2.30.2
Instead of relying on awful hacks to obtain the offset of the cpu field
in struct task_struct, move it back into struct thread_info, which does
not create the same level of circular dependency hell when trying to
include the header file that defines it.
Signed-off-by: Ard Biesheuvel <[email protected]>
---
arch/powerpc/Makefile | 11 -----------
arch/powerpc/include/asm/smp.h | 17 +----------------
arch/powerpc/kernel/asm-offsets.c | 2 --
3 files changed, 1 insertion(+), 29 deletions(-)
diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
index aa6808e70647..54cad1faa5d0 100644
--- a/arch/powerpc/Makefile
+++ b/arch/powerpc/Makefile
@@ -446,17 +446,6 @@ else
endif
endif
-ifdef CONFIG_SMP
-ifdef CONFIG_PPC32
-prepare: task_cpu_prepare
-
-PHONY += task_cpu_prepare
-task_cpu_prepare: prepare0
- $(eval KBUILD_CFLAGS += -D_TASK_CPU=$(shell awk '{if ($$2 == "TASK_CPU") print $$3;}' include/generated/asm-offsets.h))
-
-endif # CONFIG_PPC32
-endif # CONFIG_SMP
-
PHONY += checkbin
# Check toolchain versions:
# - gcc-4.6 is the minimum kernel-wide version so nothing required.
diff --git a/arch/powerpc/include/asm/smp.h b/arch/powerpc/include/asm/smp.h
index 7ef1cd8168a0..007332a4a732 100644
--- a/arch/powerpc/include/asm/smp.h
+++ b/arch/powerpc/include/asm/smp.h
@@ -87,22 +87,7 @@ int is_cpu_dead(unsigned int cpu);
/* 32-bit */
extern int smp_hw_index[];
-/*
- * This is particularly ugly: it appears we can't actually get the definition
- * of task_struct here, but we need access to the CPU this task is running on.
- * Instead of using task_struct we're using _TASK_CPU which is extracted from
- * asm-offsets.h by kbuild to get the current processor ID.
- *
- * This also needs to be safeguarded when building asm-offsets.s because at
- * that time _TASK_CPU is not defined yet. It could have been guarded by
- * _TASK_CPU itself, but we want the build to fail if _TASK_CPU is missing
- * when building something else than asm-offsets.s
- */
-#ifdef GENERATING_ASM_OFFSETS
-#define raw_smp_processor_id() (0)
-#else
-#define raw_smp_processor_id() (*(unsigned int *)((void *)current + _TASK_CPU))
-#endif
+#define raw_smp_processor_id() (current_thread_info()->cpu)
#define hard_smp_processor_id() (smp_hw_index[smp_processor_id()])
static inline int get_hard_smp_processor_id(int cpu)
diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
index e37e4546034e..cc05522f50bf 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c
@@ -9,8 +9,6 @@
* #defines from the assembly-language output.
*/
-#define GENERATING_ASM_OFFSETS /* asm/smp.h */
-
#include <linux/compat.h>
#include <linux/signal.h>
#include <linux/sched.h>
--
2.30.2
Le 14/09/2021 à 14:10, Ard Biesheuvel a écrit :
> Commit c65eacbe290b ("sched/core: Allow putting thread_info into
> task_struct") mentions that, along with moving thread_info into
> task_struct, the cpu field is moved out of the former into the latter,
> but does not explain why.
I think it does explain why (init/Kconfig): "an arch will need to remove
all thread_info fields except flags".
IIUC initially the intention with THREAD_INFO_IN_TASK was to remove
everything from thread_info, but at the end it didn't happen it seems.
>
> While collaborating with Keith on adding THREAD_INFO_IN_TASK support to
> ARM, we noticed that keeping CPU in task_struct is problematic for
> architectures that define raw_smp_processor_id() in terms of this field,
> as it requires linux/sched.h to be included, which causes a lot of pain
> in terms of circular dependencies (or 'header soup', as the original
> commit refers to it).
>
> For examples of how existing architectures work around this, please
> refer to patches #6 or #7. In the former case, it uses an awful
> asm-offsets hack to index thread_info/current without using its type
> definition. The latter approach simply keeps a copy of the task_struct
> CPU field in thread_info, and keeps it in sync at context switch time.
It was a pain when implementing that on powerpc, so I really like your
idea, the series looks good to me.
>
> Patch #8 reverts this latter approach for ARM, but this code is still
> under review so it does not currently apply to mainline.
>
> We also discussed introducing yet another Kconfig symbol to indicate
> that the arch has THREAD_INFO_IN_TASK enabled but still prefers to keep
> its CPU field in thread_info, but simply keeping it in thread_info in
> all cases seems to be the cleanest approach here.
Yes, if we can avoid yet another config, that's better. We already have
so many configs that are supposed to be temporary and have lasted for
years if not decades.
Christophe
On Tue, Sep 14, 2021 at 02:10:28PM +0200, Ard Biesheuvel wrote:
> Commit c65eacbe290b ("sched/core: Allow putting thread_info into
> task_struct") mentions that, along with moving thread_info into
> task_struct, the cpu field is moved out of the former into the latter,
> but does not explain why.
From what I recall of talking to Andy around that time, when converting
arm64 over, the theory was that over time we'd move more and more out of
thread_info and into task_struct or thread_struct, until task_struct
supplanted thread_info entirely, and that all became generic.
I think the key gain there was making things more *generic*, and there
are other ways we could do that in future without moving more into
task_struct (e.g. with a geenric thread_info and arch_thread_info inside
that).
With that in mind, and given the diffstat, I think this is worthwhile.
FWIW, for the series:
Acked-by: Mark Rutland <[email protected]>
Mark.
> While collaborating with Keith on adding THREAD_INFO_IN_TASK support to
> ARM, we noticed that keeping CPU in task_struct is problematic for
> architectures that define raw_smp_processor_id() in terms of this field,
> as it requires linux/sched.h to be included, which causes a lot of pain
> in terms of circular dependencies (or 'header soup', as the original
> commit refers to it).
>
> For examples of how existing architectures work around this, please
> refer to patches #6 or #7. In the former case, it uses an awful
> asm-offsets hack to index thread_info/current without using its type
> definition. The latter approach simply keeps a copy of the task_struct
> CPU field in thread_info, and keeps it in sync at context switch time.
>
> Patch #8 reverts this latter approach for ARM, but this code is still
> under review so it does not currently apply to mainline.
>
> We also discussed introducing yet another Kconfig symbol to indicate
> that the arch has THREAD_INFO_IN_TASK enabled but still prefers to keep
> its CPU field in thread_info, but simply keeping it in thread_info in
> all cases seems to be the cleanest approach here.
>
> Cc: Keith Packard <[email protected]>
> Cc: Russell King <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Michael Ellerman <[email protected]>
> Cc: Benjamin Herrenschmidt <[email protected]>
> Cc: Christophe Leroy <[email protected]>
> Cc: Paul Mackerras <[email protected]>
> Cc: Paul Walmsley <[email protected]>
> Cc: Palmer Dabbelt <[email protected]>
> Cc: Albert Ou <[email protected]>
> Cc: Heiko Carstens <[email protected]>
> Cc: Vasily Gorbik <[email protected]>
> Cc: Christian Borntraeger <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: Andy Lutomirski <[email protected]>
> Cc: Linus Torvalds <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
>
> Ard Biesheuvel (8):
> arm64: add CPU field to struct thread_info
> x86: add CPU field to struct thread_info
> s390: add CPU field to struct thread_info
> powerpc: add CPU field to struct thread_info
> sched: move CPU field back into thread_info if THREAD_INFO_IN_TASK=y
> powerpc: smp: remove hack to obtain offset of task_struct::cpu
> riscv: rely on core code to keep thread_info::cpu updated
> ARM: rely on core code to keep thread_info::cpu updated
>
> arch/arm/include/asm/switch_to.h | 14 --------------
> arch/arm/kernel/smp.c | 3 ---
> arch/arm64/include/asm/thread_info.h | 1 +
> arch/arm64/kernel/asm-offsets.c | 2 +-
> arch/arm64/kernel/head.S | 2 +-
> arch/powerpc/Makefile | 11 -----------
> arch/powerpc/include/asm/smp.h | 17 +----------------
> arch/powerpc/include/asm/thread_info.h | 3 +++
> arch/powerpc/kernel/asm-offsets.c | 4 +---
> arch/powerpc/kernel/smp.c | 2 +-
> arch/riscv/kernel/asm-offsets.c | 1 -
> arch/riscv/kernel/entry.S | 5 -----
> arch/riscv/kernel/head.S | 1 -
> arch/s390/include/asm/thread_info.h | 1 +
> arch/x86/include/asm/thread_info.h | 3 +++
> include/linux/sched.h | 6 +-----
> kernel/sched/sched.h | 4 ----
> 17 files changed, 14 insertions(+), 66 deletions(-)
>
> --
> 2.30.2
>
On Tue, Sep 14, 2021 at 5:10 AM Ard Biesheuvel <[email protected]> wrote:
>
> The CPU field will be moved back into thread_info even when
> THREAD_INFO_IN_TASK is enabled, so add it back to arm64's definition of
> struct thread_info.
The series looks sane to me, but it strikes me that it's inconsistent
- here for arm64, you make it unconditional, but for the other
architectures you end up putting it inside a #ifdef CONFIG_SMP.
Was there some reason for this odd behavior?
Linus
On Tue, 14 Sept 2021 at 17:41, Linus Torvalds
<[email protected]> wrote:
>
> On Tue, Sep 14, 2021 at 5:10 AM Ard Biesheuvel <[email protected]> wrote:
> >
> > The CPU field will be moved back into thread_info even when
> > THREAD_INFO_IN_TASK is enabled, so add it back to arm64's definition of
> > struct thread_info.
>
> The series looks sane to me, but it strikes me that it's inconsistent
> - here for arm64, you make it unconditional, but for the other
> architectures you end up putting it inside a #ifdef CONFIG_SMP.
>
> Was there some reason for this odd behavior?
>
Yes. CONFIG_SMP is a 'def_bool y' on arm64.
On Tue, Sep 14, 2021 at 5:11 AM Ard Biesheuvel <[email protected]> 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
<[email protected]> wrote:
>
> On Tue, Sep 14, 2021 at 5:11 AM Ard Biesheuvel <[email protected]> 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 <[email protected]> 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
<[email protected]> wrote:
>
> On Tue, Sep 14, 2021 at 8:53 AM Ard Biesheuvel <[email protected]> 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:29PM +0200, Ard Biesheuvel wrote:
> The CPU field will be moved back into thread_info even when
> THREAD_INFO_IN_TASK is enabled, so add it back to arm64's definition of
> struct thread_info.
>
> Signed-off-by: Ard Biesheuvel <[email protected]>
Acked-by: Catalin Marinas <[email protected]>
On Thu, 16 Sept 2021 at 16:41, Catalin Marinas <[email protected]> wrote:
>
> On Tue, Sep 14, 2021 at 02:10:29PM +0200, Ard Biesheuvel wrote:
> > The CPU field will be moved back into thread_info even when
> > THREAD_INFO_IN_TASK is enabled, so add it back to arm64's definition of
> > struct thread_info.
> >
> > Signed-off-by: Ard Biesheuvel <[email protected]>
>
> Acked-by: Catalin Marinas <[email protected]>
Thanks. I take it this applies to patch #5 as well?
On Tue, 14 Sept 2021 at 15:55, Mark Rutland <[email protected]> wrote:
>
> On Tue, Sep 14, 2021 at 02:10:28PM +0200, Ard Biesheuvel wrote:
> > Commit c65eacbe290b ("sched/core: Allow putting thread_info into
> > task_struct") mentions that, along with moving thread_info into
> > task_struct, the cpu field is moved out of the former into the latter,
> > but does not explain why.
>
> From what I recall of talking to Andy around that time, when converting
> arm64 over, the theory was that over time we'd move more and more out of
> thread_info and into task_struct or thread_struct, until task_struct
> supplanted thread_info entirely, and that all became generic.
>
> I think the key gain there was making things more *generic*, and there
> are other ways we could do that in future without moving more into
> task_struct (e.g. with a geenric thread_info and arch_thread_info inside
> that).
>
> With that in mind, and given the diffstat, I think this is worthwhile.
>
> FWIW, for the series:
>
> Acked-by: Mark Rutland <[email protected]>
>
Thanks.
Any comments on this from the various arch maintainers? Especially
power, as Christophe seems happy with this but there are 3 different
patches affecting power that need a maintainer ack.
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 <[email protected]>
Acked-by: Catalin Marinas <[email protected]>
On Tue, 14 Sep 2021 05:10:35 PDT (-0700), [email protected] wrote:
> Now that the core code switched back to using thread_info::cpu to keep
> a task's CPU number, we no longer need to keep it in sync explicitly. So
> just drop the code that does this.
>
> Signed-off-by: Ard Biesheuvel <[email protected]>
> ---
> arch/riscv/kernel/asm-offsets.c | 1 -
> arch/riscv/kernel/entry.S | 5 -----
> arch/riscv/kernel/head.S | 1 -
> 3 files changed, 7 deletions(-)
>
> diff --git a/arch/riscv/kernel/asm-offsets.c b/arch/riscv/kernel/asm-offsets.c
> index 90f8ce64fa6f..478d9f02dab5 100644
> --- a/arch/riscv/kernel/asm-offsets.c
> +++ b/arch/riscv/kernel/asm-offsets.c
> @@ -33,7 +33,6 @@ void asm_offsets(void)
> OFFSET(TASK_TI_PREEMPT_COUNT, task_struct, thread_info.preempt_count);
> OFFSET(TASK_TI_KERNEL_SP, task_struct, thread_info.kernel_sp);
> OFFSET(TASK_TI_USER_SP, task_struct, thread_info.user_sp);
> - OFFSET(TASK_TI_CPU, task_struct, thread_info.cpu);
>
> OFFSET(TASK_THREAD_F0, task_struct, thread.fstate.f[0]);
> OFFSET(TASK_THREAD_F1, task_struct, thread.fstate.f[1]);
> diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
> index 98f502654edd..459eb1714353 100644
> --- a/arch/riscv/kernel/entry.S
> +++ b/arch/riscv/kernel/entry.S
> @@ -544,11 +544,6 @@ ENTRY(__switch_to)
> REG_L s9, TASK_THREAD_S9_RA(a4)
> REG_L s10, TASK_THREAD_S10_RA(a4)
> REG_L s11, TASK_THREAD_S11_RA(a4)
> - /* Swap the CPU entry around. */
> - lw a3, TASK_TI_CPU(a0)
> - lw a4, TASK_TI_CPU(a1)
> - sw a3, TASK_TI_CPU(a1)
> - sw a4, TASK_TI_CPU(a0)
> /* The offset of thread_info in task_struct is zero. */
> move tp, a1
> ret
> diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
> index fce5184b22c3..d5ec30ef6f5d 100644
> --- a/arch/riscv/kernel/head.S
> +++ b/arch/riscv/kernel/head.S
> @@ -317,7 +317,6 @@ clear_bss_done:
> call setup_trap_vector
> /* Restore C environment */
> la tp, init_task
> - sw zero, TASK_TI_CPU(tp)
> la sp, init_thread_union + THREAD_SIZE
>
> #ifdef CONFIG_KASAN
Acked-by: Palmer Dabbelt <[email protected]>
Thanks!