| Message ID | 20210914121036.3975026-1-ardb@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Move task_struct::cpu back into thread_info | expand |
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 <mark.rutland@arm.com> 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 <keithpac@amazon.com> > Cc: Russell King <linux@armlinux.org.uk> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will@kernel.org> > Cc: Michael Ellerman <mpe@ellerman.id.au> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Cc: Christophe Leroy <christophe.leroy@csgroup.eu> > Cc: Paul Mackerras <paulus@samba.org> > Cc: Paul Walmsley <paul.walmsley@sifive.com> > Cc: Palmer Dabbelt <palmer@dabbelt.com> > Cc: Albert Ou <aou@eecs.berkeley.edu> > Cc: Heiko Carstens <hca@linux.ibm.com> > Cc: Vasily Gorbik <gor@linux.ibm.com> > Cc: Christian Borntraeger <borntraeger@de.ibm.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Borislav Petkov <bp@alien8.de> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Kees Cook <keescook@chromium.org> > Cc: Andy Lutomirski <luto@kernel.org> > Cc: Linus Torvalds <torvalds@linux-foundation.org> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: linux-arm-kernel@lists.infradead.org > Cc: linuxppc-dev@lists.ozlabs.org > Cc: linux-riscv@lists.infradead.org > Cc: linux-s390@vger.kernel.org > > 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, 14 Sept 2021 at 15:55, Mark Rutland <mark.rutland@arm.com> 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 <mark.rutland@arm.com> > 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.
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 <keithpac@amazon.com> Cc: Russell King <linux@armlinux.org.uk> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will@kernel.org> Cc: Michael Ellerman <mpe@ellerman.id.au> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> Cc: Christophe Leroy <christophe.leroy@csgroup.eu> Cc: Paul Mackerras <paulus@samba.org> Cc: Paul Walmsley <paul.walmsley@sifive.com> Cc: Palmer Dabbelt <palmer@dabbelt.com> Cc: Albert Ou <aou@eecs.berkeley.edu> Cc: Heiko Carstens <hca@linux.ibm.com> Cc: Vasily Gorbik <gor@linux.ibm.com> Cc: Christian Borntraeger <borntraeger@de.ibm.com> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Ingo Molnar <mingo@redhat.com> Cc: Borislav Petkov <bp@alien8.de> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Kees Cook <keescook@chromium.org> Cc: Andy Lutomirski <luto@kernel.org> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Arnd Bergmann <arnd@arndb.de> Cc: linux-arm-kernel@lists.infradead.org Cc: linuxppc-dev@lists.ozlabs.org Cc: linux-riscv@lists.infradead.org Cc: linux-s390@vger.kernel.org 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(-)