| Message ID | cca346b5c87693499e630291d78fb0bf12c24290.1676358308.git.jpoimboe@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | cpu,sched: Mark arch_cpu_idle_dead() __noreturn | expand |
Hi Josh, On 14/2/23 08:05, Josh Poimboeuf wrote: > cpu_die() doesn't return. Make that more explicit with a BUG(). > > BUG() is preferable to unreachable() because BUG() is a more explicit > failure mode and avoids undefined behavior like falling off the edge of > the function into whatever code happens to be next. > > Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org> > --- > arch/xtensa/kernel/smp.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/arch/xtensa/kernel/smp.c b/arch/xtensa/kernel/smp.c > index 4dc109dd6214..7bad78495536 100644 > --- a/arch/xtensa/kernel/smp.c > +++ b/arch/xtensa/kernel/smp.c Can you update the documentation along? Currently we have: /* * Called from the idle thread for the CPU which has been shutdown. * * Note that we disable IRQs here, but do not re-enable them * before returning to the caller. This is also the behaviour * of the other hotplug-cpu capable cores, so presumably coming * out of idle fixes this. */ > @@ -341,6 +341,8 @@ void __ref cpu_die(void) > __asm__ __volatile__( > " movi a2, cpu_restart\n" > " jx a2\n"); > + > + BUG(); > } > > #endif /* CONFIG_HOTPLUG_CPU */
On Tue, Feb 14, 2023 at 08:55:32AM +0100, Philippe Mathieu-Daudé wrote: > Hi Josh, > > On 14/2/23 08:05, Josh Poimboeuf wrote: > > cpu_die() doesn't return. Make that more explicit with a BUG(). > > > > BUG() is preferable to unreachable() because BUG() is a more explicit > > failure mode and avoids undefined behavior like falling off the edge of > > the function into whatever code happens to be next. > > > > Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org> > > --- > > arch/xtensa/kernel/smp.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/arch/xtensa/kernel/smp.c b/arch/xtensa/kernel/smp.c > > index 4dc109dd6214..7bad78495536 100644 > > --- a/arch/xtensa/kernel/smp.c > > +++ b/arch/xtensa/kernel/smp.c > > Can you update the documentation along? Currently we have: > > /* > * Called from the idle thread for the CPU which has been shutdown. > * > * Note that we disable IRQs here, but do not re-enable them > * before returning to the caller. This is also the behaviour > * of the other hotplug-cpu capable cores, so presumably coming > * out of idle fixes this. > */ void __ref cpu_die(void) { idle_task_exit(); local_irq_disable(); __asm__ __volatile__( " movi a2, cpu_restart\n" " jx a2\n"); BUG(); } Hm, not only is the comment wrong, but it seems to be branching to cpu_restart? That doesn't seem right at all. Max/Chris?
On Tue, 14 Feb 2023 10:23:22 -0800 Josh Poimboeuf <jpoimboe@kernel.org> wrote: > void __ref cpu_die(void) > { > idle_task_exit(); > local_irq_disable(); > __asm__ __volatile__( > " movi a2, cpu_restart\n" > " jx a2\n"); > > BUG(); > } > > Hm, not only is the comment wrong, but it seems to be branching to > cpu_restart? That doesn't seem right at all. Agreed, that does not look right at all. -- Steve > > Max/Chris? >
On Tue, Feb 14, 2023 at 10:23 AM Josh Poimboeuf <jpoimboe@kernel.org> wrote: > On Tue, Feb 14, 2023 at 08:55:32AM +0100, Philippe Mathieu-Daudé wrote: > > Can you update the documentation along? Currently we have: > > > > /* > > * Called from the idle thread for the CPU which has been shutdown. > > * > > * Note that we disable IRQs here, but do not re-enable them > > * before returning to the caller. This is also the behaviour > > * of the other hotplug-cpu capable cores, so presumably coming > > * out of idle fixes this. > > */ > > void __ref cpu_die(void) > { > idle_task_exit(); > local_irq_disable(); > __asm__ __volatile__( > " movi a2, cpu_restart\n" > " jx a2\n"); > > BUG(); > } > > Hm, not only is the comment wrong, but it seems to be branching to > cpu_restart? That doesn't seem right at all. Perhaps the name is a bit misleading. The CPU that enters 'cpu_restart' loops there until a call to 'boot_secondary' releases it, after which it goes to '_startup'. So it is a restart, but not immediate.
On Tue, Feb 14, 2023 at 11:48:41AM -0800, Max Filippov wrote: > On Tue, Feb 14, 2023 at 10:23 AM Josh Poimboeuf <jpoimboe@kernel.org> wrote: > > On Tue, Feb 14, 2023 at 08:55:32AM +0100, Philippe Mathieu-Daudé wrote: > > > Can you update the documentation along? Currently we have: > > > > > > /* > > > * Called from the idle thread for the CPU which has been shutdown. > > > * > > > * Note that we disable IRQs here, but do not re-enable them > > > * before returning to the caller. This is also the behaviour > > > * of the other hotplug-cpu capable cores, so presumably coming > > > * out of idle fixes this. > > > */ > > > > void __ref cpu_die(void) > > { > > idle_task_exit(); > > local_irq_disable(); > > __asm__ __volatile__( > > " movi a2, cpu_restart\n" > > " jx a2\n"); > > > > BUG(); > > } > > > > Hm, not only is the comment wrong, but it seems to be branching to > > cpu_restart? That doesn't seem right at all. > > Perhaps the name is a bit misleading. The CPU that enters 'cpu_restart' > loops there until a call to 'boot_secondary' releases it, after which it goes > to '_startup'. So it is a restart, but not immediate. Ah, I see. That sounds similar to what Xen does.
diff --git a/arch/xtensa/kernel/smp.c b/arch/xtensa/kernel/smp.c index 4dc109dd6214..7bad78495536 100644 --- a/arch/xtensa/kernel/smp.c +++ b/arch/xtensa/kernel/smp.c @@ -341,6 +341,8 @@ void __ref cpu_die(void) __asm__ __volatile__( " movi a2, cpu_restart\n" " jx a2\n"); + + BUG(); } #endif /* CONFIG_HOTPLUG_CPU */
cpu_die() doesn't return. Make that more explicit with a BUG(). BUG() is preferable to unreachable() because BUG() is a more explicit failure mode and avoids undefined behavior like falling off the edge of the function into whatever code happens to be next. Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org> --- arch/xtensa/kernel/smp.c | 2 ++ 1 file changed, 2 insertions(+)