On Thu, 4 Nov 2021 at 16:26, Helge Deller <deller@xxxxxx> wrote:
>
> On 11/4/21 09:39, Ard Biesheuvel wrote:
> > On Thu, 4 Nov 2021 at 09:31, Helge Deller <deller@xxxxxx> wrote:
> >>
> >> On 11/4/21 09:26, Ard Biesheuvel wrote:
> >>> In commit 2214c0e77259 ("parisc: Move thread_info into task struct")
> >>> PA-RISC gained support for THREAD_INFO_IN_TASK while changes were
> >>> already underway to keep the CPU field in thread_info rather than move
> >>> it into task_struct when THREAD_INFO_IN_TASK is enabled. The result is a
> >>> broken build for all PA-RISC configs that enable SMP.
> >>>
> >>> So let's partially revert that commit, and get rid of the ugly hack to
> >>> get at the offset of task_struct::cpu without having to include
> >>> linux/sched.h, and put the CPU field back where it was before.
> >>>
> >>> Reported-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> >>> Fixes: bcf9033e5449 ("sched: move CPU field back into thread_info if THREAD_INFO_IN_TASK=y")
> >>> Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
> >>
> >> Thanks Ard!
> >>
> >> I actually had the same patch finished right now too :-)
> >>
> >
> > I was wondering about that :-)
> >
> >> One small nid though. See below...
> >>
> >>
> >>> ---
> >>> arch/parisc/include/asm/smp.h | 19 ++-----------------
> >>> arch/parisc/include/asm/thread_info.h | 2 ++
> >>> arch/parisc/kernel/asm-offsets.c | 4 +---
> >>> arch/parisc/kernel/smp.c | 2 +-
> >>> 4 files changed, 6 insertions(+), 21 deletions(-)
> >>>
> >>> diff --git a/arch/parisc/include/asm/smp.h b/arch/parisc/include/asm/smp.h
> >>> index 16d41127500e..cba55abf7bac 100644
> >>> --- a/arch/parisc/include/asm/smp.h
> >>> +++ b/arch/parisc/include/asm/smp.h
> >>> @@ -34,23 +34,8 @@ extern void arch_send_call_function_ipi_mask(const struct cpumask *mask);
> >>>
> >>> #endif /* !ASSEMBLY */
> >>>
> >>> -/*
> >>> - * 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
> >>> -#include <asm/asm-offsets.h>
> >>> -#define raw_smp_processor_id() (*(unsigned int *)((void *)current + TASK_CPU))
> >>> -#endif
> >>> +#define raw_smp_processor_id() (current_thread_info()->cpu)
> >>> +
> >>> #else /* CONFIG_SMP */
> >>>
> >>> static inline void smp_send_all_nop(void) { return; }
> >>> diff --git a/arch/parisc/include/asm/thread_info.h b/arch/parisc/include/asm/thread_info.h
> >>> index 75657c2c54e1..d6268834bfa5 100644
> >>> --- a/arch/parisc/include/asm/thread_info.h
> >>> +++ b/arch/parisc/include/asm/thread_info.h
> >>> @@ -8,12 +8,14 @@
> >>>
> >>> struct thread_info {
> >>> unsigned long flags; /* thread_info flags (see TIF_*) */
> >>> + __u32 cpu; /* current CPU */
> >>> int preempt_count; /* 0=premptable, <0=BUG; will also serve as bh-counter */
> >>> };
> >>>
> >>> #define INIT_THREAD_INFO(tsk) \
> >>> { \
> >>> .flags = 0, \
> >>> + .cpu = 0, \
> >>> .preempt_count = INIT_PREEMPT_COUNT, \
> >>> }
> >>>
> >>> diff --git a/arch/parisc/kernel/asm-offsets.c b/arch/parisc/kernel/asm-offsets.c
> >>> index e35154035441..629d38e36e22 100644
> >>> --- a/arch/parisc/kernel/asm-offsets.c
> >>> +++ b/arch/parisc/kernel/asm-offsets.c
> >>> @@ -14,8 +14,6 @@
> >>> * Copyright (C) 2003 James Bottomley <jejb at parisc-linux.org>
> >>> */
> >>>
> >>> -#define GENERATING_ASM_OFFSETS /* asm/smp.h */
> >>> -
> >>> #include <linux/types.h>
> >>> #include <linux/sched.h>
> >>> #include <linux/thread_info.h>
> >>> @@ -40,7 +38,7 @@ int main(void)
> >>> DEFINE(TASK_TI_FLAGS, offsetof(struct task_struct, thread_info.flags));
> >>> DEFINE(TASK_STACK, offsetof(struct task_struct, stack));
> >>> #ifdef CONFIG_SMP
> >>> - DEFINE(TASK_CPU, offsetof(struct task_struct, cpu));
> >>> + DEFINE(TASK_CPU, offsetof(struct task_struct, thread_info.cpu));
> >>> #endif
> >>
> >> The TASK_CPU define isn't needed any longer.
> >> So, the whole part inside #ifdef..#endif can go.
> >>
> >
> > OK let's just drop it then.
>
> Ok
>
> >
> >>> BLANK();
> >>> DEFINE(TASK_REGS, offsetof(struct task_struct, thread.regs));
> >>> diff --git a/arch/parisc/kernel/smp.c b/arch/parisc/kernel/smp.c
> >>> index 171925285f3e..0cd97fa004c5 100644
> >>> --- a/arch/parisc/kernel/smp.c
> >>> +++ b/arch/parisc/kernel/smp.c
> >>> @@ -339,7 +339,7 @@ int smp_boot_one_cpu(int cpuid, struct task_struct *idle)
> >>> const struct cpuinfo_parisc *p = &per_cpu(cpu_data, cpuid);
> >>> long timeout;
> >>>
> >>> - idle->cpu = cpuid;
> >>> + task_thread_info(idle)->cpu = cpuid;
> >
> > I was wondering if this could simply be dropped altogether? I am
> > pretty sure it is redundant, as the core code sets this field as well,
> > but I don't have access to a SMP PA-RISC machine to test it.
>
>
> Good point.
> I tested and it can be dropped.
>
>
> >>> /* Let _start know what logical CPU we're booting
> >>> ** (offset into init_tasks[],cpu_data[])
> >>>
> >
> >> If it's Ok for you I'll clean up your patch and submit
> >> all with the next pull request to Linus later today.
> >>
> >
> > Whatever works for you is fine with me.
>
> Ok. I've cleaned it up, applied it to my for-next tree and will push later today to Linus
> with other patches. Here is the current version:
> https://git.kernel.org/pub/scm/linux/kernel/git/deller/parisc-linux.git/commit/?h=for-next
>
Looks fine
Thanks,
