* [PATCH] parisc: move CPU field back into thread_info
@ 2021-11-04 8:26 Ard Biesheuvel
2021-11-04 8:30 ` Helge Deller
0 siblings, 1 reply; 6+ messages in thread
From: Ard Biesheuvel @ 2021-11-04 8:26 UTC (permalink / raw)
To: linux-parisc; +Cc: James.Bottomley, deller, linux, torvalds, Ard Biesheuvel
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@roeck-us.net>
Fixes: bcf9033e5449 ("sched: move CPU field back into thread_info if THREAD_INFO_IN_TASK=y")
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
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
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;
/* Let _start know what logical CPU we're booting
** (offset into init_tasks[],cpu_data[])
--
2.30.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] parisc: move CPU field back into thread_info
2021-11-04 8:26 [PATCH] parisc: move CPU field back into thread_info Ard Biesheuvel
@ 2021-11-04 8:30 ` Helge Deller
2021-11-04 8:39 ` Ard Biesheuvel
0 siblings, 1 reply; 6+ messages in thread
From: Helge Deller @ 2021-11-04 8:30 UTC (permalink / raw)
To: Ard Biesheuvel, linux-parisc; +Cc: James.Bottomley, linux, torvalds
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@roeck-us.net>
> Fixes: bcf9033e5449 ("sched: move CPU field back into thread_info if THREAD_INFO_IN_TASK=y")
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Thanks Ard!
I actually had the same patch finished right now too :-)
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.
If it's Ok for you I'll clean up your patch and submit
all with the next pull request to Linus later today.
Helge
> 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;
>
> /* Let _start know what logical CPU we're booting
> ** (offset into init_tasks[],cpu_data[])
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] parisc: move CPU field back into thread_info
2021-11-04 8:30 ` Helge Deller
@ 2021-11-04 8:39 ` Ard Biesheuvel
2021-11-04 15:25 ` Helge Deller
0 siblings, 1 reply; 6+ messages in thread
From: Ard Biesheuvel @ 2021-11-04 8:39 UTC (permalink / raw)
To: Helge Deller
Cc: open list:PARISC ARCHITECTURE, James E.J. Bottomley,
Guenter Roeck, Linus Torvalds
On Thu, 4 Nov 2021 at 09:31, Helge Deller <deller@gmx.de> 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@roeck-us.net>
> > Fixes: bcf9033e5449 ("sched: move CPU field back into thread_info if THREAD_INFO_IN_TASK=y")
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>
> 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.
> > 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.
> >
> > /* 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] parisc: move CPU field back into thread_info
2021-11-04 8:39 ` Ard Biesheuvel
@ 2021-11-04 15:25 ` Helge Deller
2021-11-04 15:35 ` Ard Biesheuvel
2021-11-04 16:51 ` Guenter Roeck
0 siblings, 2 replies; 6+ messages in thread
From: Helge Deller @ 2021-11-04 15:25 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: open list:PARISC ARCHITECTURE, James E.J. Bottomley,
Guenter Roeck, Linus Torvalds
On 11/4/21 09:39, Ard Biesheuvel wrote:
> On Thu, 4 Nov 2021 at 09:31, Helge Deller <deller@gmx.de> 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@roeck-us.net>
>>> Fixes: bcf9033e5449 ("sched: move CPU field back into thread_info if THREAD_INFO_IN_TASK=y")
>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>
>> 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
Thanks!
Helge
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] parisc: move CPU field back into thread_info
2021-11-04 15:25 ` Helge Deller
@ 2021-11-04 15:35 ` Ard Biesheuvel
2021-11-04 16:51 ` Guenter Roeck
1 sibling, 0 replies; 6+ messages in thread
From: Ard Biesheuvel @ 2021-11-04 15:35 UTC (permalink / raw)
To: Helge Deller
Cc: open list:PARISC ARCHITECTURE, James E.J. Bottomley,
Guenter Roeck, Linus Torvalds
On Thu, 4 Nov 2021 at 16:26, Helge Deller <deller@gmx.de> wrote:
>
> On 11/4/21 09:39, Ard Biesheuvel wrote:
> > On Thu, 4 Nov 2021 at 09:31, Helge Deller <deller@gmx.de> 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@roeck-us.net>
> >>> Fixes: bcf9033e5449 ("sched: move CPU field back into thread_info if THREAD_INFO_IN_TASK=y")
> >>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> >>
> >> 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,
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] parisc: move CPU field back into thread_info
2021-11-04 15:25 ` Helge Deller
2021-11-04 15:35 ` Ard Biesheuvel
@ 2021-11-04 16:51 ` Guenter Roeck
1 sibling, 0 replies; 6+ messages in thread
From: Guenter Roeck @ 2021-11-04 16:51 UTC (permalink / raw)
To: Helge Deller, Ard Biesheuvel
Cc: open list:PARISC ARCHITECTURE, James E.J. Bottomley,
Linus Torvalds
On 11/4/21 8:25 AM, Helge Deller wrote:
> On 11/4/21 09:39, Ard Biesheuvel wrote:
>> On Thu, 4 Nov 2021 at 09:31, Helge Deller <deller@gmx.de> 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@roeck-us.net>
>>>> Fixes: bcf9033e5449 ("sched: move CPU field back into thread_info if THREAD_INFO_IN_TASK=y")
>>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>>
>>> 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
>
Works for me.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-11-04 16:51 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-11-04 8:26 [PATCH] parisc: move CPU field back into thread_info Ard Biesheuvel
2021-11-04 8:30 ` Helge Deller
2021-11-04 8:39 ` Ard Biesheuvel
2021-11-04 15:25 ` Helge Deller
2021-11-04 15:35 ` Ard Biesheuvel
2021-11-04 16:51 ` Guenter Roeck
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.