[RFC] Make SMP secondary CPU up more resilient to failure.

[RFC] Make SMP secondary CPU up more resilient to failure.

[Andrei Warkentin]

Hi,

This is my first time on linux-arm-kernel, and while I've read the
FAQ, hopefully I don't screw up too badly :).

Anyway, we're on a dual-core ARMv7 running 2.6.36, and during
stability stress testing saw the following:
1) After a number hotplug iterations, CPU1 fails to set its online bit
quickly enough and __cpu_up() times-out.
2) CPU1 eventually completes its startup and sets the bit, however,
since _cpu_up() failed, CPU1's active bit is never set.
3) On the next call to cpu_down(), the function checks that the online
bit is set and proceeds.
4) The workqueue receives a CPU_DOWN_PENDING notification and creates
a trustee_thread to run on CPU1.
5) Since CPU1's active bit is not set, the scheduler runs the thread
on CPU0 instead and the BUG_ON at kernel/workqueue.c:3111 fires (check
that CPU run on is the desired one)

I have a patch that resolves this in two ways -
1) I only wait with timeouts to make sure the CPU registers the SIPI
and enters secondary_start_kernel. After that we're in "safe"
territory and it doesn't matter how long the rest of initialization
takes. So that puts the waited time into deterministic land.

2) Additionally I ensure that if the CPU comes up later than it were
supposed to (shouldn't, but...), then it will not start initializing
behind cpu_up's back (which is not really undoable). This solves the
problem with both cpu_up+secondary_start_kernel races and with
platform_cpu_kill+secondary_start_kernel races.

Tested this by injecting mdelays into secondary_start_kernel (before
clearing the booting bit and after), and by putting a while(1) into
secondary_start_kernel at different points.

There were concerns brought up that this patch might conflict with all
the ARM SMP work going into .38, so I wanted to check with the list
first.

Thank You,
A
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-ARM-Make-SMP-init-more-resilient-to-failures.patch
Type: text/x-patch
Size: 4678 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20101215/ec26c93b/attachment.bin>

[RFC] Make SMP secondary CPU up more resilient to failure.

[Russell King - ARM Linux]

On Wed, Dec 15, 2010 at 05:45:13PM -0600, Andrei Warkentin wrote:
> This is my first time on linux-arm-kernel, and while I've read the
> FAQ, hopefully I don't screw up too badly :).
> 
> Anyway, we're on a dual-core ARMv7 running 2.6.36, and during
> stability stress testing saw the following:
> 1) After a number hotplug iterations, CPU1 fails to set its online bit
> quickly enough and __cpu_up() times-out.
> 2) CPU1 eventually completes its startup and sets the bit, however,
> since _cpu_up() failed, CPU1's active bit is never set.

Why is your CPU taking soo long to come up?  We wait one second in the
generic code, which is the time taken from the platform code being happy
that it has successfully started the CPU.  Normally, platforms wait an
additional second to detect the CPU entering the kernel.

> 2) Additionally I ensure that if the CPU comes up later than it were
> supposed to (shouldn't, but...), then it will not start initializing
> behind cpu_up's back (which is not really undoable). This solves the
> problem with both cpu_up+secondary_start_kernel races and with
> platform_cpu_kill+secondary_start_kernel races.

Why would you have platform_cpu_kill() running at the same time - firstly,
hotplug events are serialized, and secondly the platform_cpu_kill() path
should wait up to five seconds for the CPU to go offline.  If it doesn't
go offline within five seconds it's dead (and maybe we should mark it
not present.)

[RFC] Make SMP secondary CPU up more resilient to failure.

[Andrei Warkentin]

On Thu, Dec 16, 2010 at 5:34 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
>
> On Wed, Dec 15, 2010 at 05:45:13PM -0600, Andrei Warkentin wrote:
> > This is my first time on linux-arm-kernel, and while I've read the
> > FAQ, hopefully I don't screw up too badly :).
> >
> > Anyway, we're on a dual-core ARMv7 running 2.6.36, and during
> > stability stress testing saw the following:
> > 1) After a number hotplug iterations, CPU1 fails to set its online bit
> > quickly enough and __cpu_up() times-out.
> > 2) CPU1 eventually completes its startup and sets the bit, however,
> > since _cpu_up() failed, CPU1's active bit is never set.
>
> Why is your CPU taking soo long to come up? ?We wait one second in the
> generic code, which is the time taken from the platform code being happy
> that it has successfully started the CPU. ?Normally, platforms wait an
> additional second to detect the CPU entering the kernel.

It seems twd_calibrate_rate is the culprit (although in our case,
since the clock is the same to both CPUs, there is no point in
calibrating).
We've seen this only when the device was under stress test load.

>
> > 2) Additionally I ensure that if the CPU comes up later than it were
> > supposed to (shouldn't, but...), then it will not start initializing
> > behind cpu_up's back (which is not really undoable). This solves the
> > problem with both cpu_up+secondary_start_kernel races and with
> > platform_cpu_kill+secondary_start_kernel races.
>
> Why would you have platform_cpu_kill() running at the same time - firstly,
> hotplug events are serialized, and secondly the platform_cpu_kill() path
> should wait up to five seconds for the CPU to go offline. ?If it doesn't
> go offline within five seconds it's dead (and maybe we should mark it
> not present.)
>

That's the platform_cpu_kill I invoke when I time out waiting for the
online bit. Sorry, wasn't being clear. Just trying
to show I didn't introduce any races :).

See, the SMP logic is sensitive to system load at the moment. Since
boot_secondary is supposed to return failure on failing
to up the secondary, maybe there is no point doing a timed wait for
the online bit, since you are guaranteed to get there.
But right now, you end up in a situation where there is a timeout, but
the CPU is up and running and registered.
And this causes bad behavior later when you try to down it.

> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC] Make SMP secondary CPU up more resilient to failure.

[Russell King - ARM Linux]

On Thu, Dec 16, 2010 at 05:09:48PM -0600, Andrei Warkentin wrote:
> On Thu, Dec 16, 2010 at 5:34 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> >
> > On Wed, Dec 15, 2010 at 05:45:13PM -0600, Andrei Warkentin wrote:
> > > This is my first time on linux-arm-kernel, and while I've read the
> > > FAQ, hopefully I don't screw up too badly :).
> > >
> > > Anyway, we're on a dual-core ARMv7 running 2.6.36, and during
> > > stability stress testing saw the following:
> > > 1) After a number hotplug iterations, CPU1 fails to set its online bit
> > > quickly enough and __cpu_up() times-out.
> > > 2) CPU1 eventually completes its startup and sets the bit, however,
> > > since _cpu_up() failed, CPU1's active bit is never set.
> >
> > Why is your CPU taking soo long to come up? ?We wait one second in the
> > generic code, which is the time taken from the platform code being happy
> > that it has successfully started the CPU. ?Normally, platforms wait an
> > additional second to detect the CPU entering the kernel.
> 
> It seems twd_calibrate_rate is the culprit (although in our case,
> since the clock is the same to both CPUs, there is no point in
> calibrating).

twd_calibrate_rate() should only run once at boot.  Once it's run,
taking CPUs offline and back online should not cause the rate to be
recalibrated.

> See, the SMP logic is sensitive to system load at the moment.

I don't think it is - it sounds like you're explicitly causing the twd
rate to be recalculated every time you're bringing a CPU online, which
is not supposed to happen.

> Since boot_secondary is supposed to return failure on failing
> to up the secondary, maybe there is no point doing a timed wait for
> the online bit, since you are guaranteed to get there.

If you're starving the secondary CPU of soo much bus bandwidth that it's
taking more than one second for it to be marked online, the delay loop
calibration is going to fail too.  If you can starve it of bus bandwidth
from the primary CPU, then you have badly designed hardware too - you'll
gain very little benefit from a SMP system if you can't sensibly run both
CPUs at the same time without starving one or other.

What I'm saying is that if it's taking more than one second to setup
the local timer (which should be a few register writes) and calibrate
the delay loop, you're going to have bigger problems and your system is
already in an unstable situation.

Please post your SMP support code so it can be reviewed, which'll help
eliminate it from being the cause.

[RFC] Make SMP secondary CPU up more resilient to failure.

[murali at embeddedwireless.com]

Sent by Maxis from my BlackBerry? smartphone

-----Original Message-----
From: Andrei Warkentin <andreiw@motorola.com>
Sender: linux-arm-kernel-bounces at lists.infradead.org
Date: Thu, 16 Dec 2010 17:09:48 
To: Russell King - ARM Linux<linux@arm.linux.org.uk>
Cc: <linux-arm-kernel@lists.infradead.org>
Subject: Re: [RFC] Make SMP secondary CPU up more resilient to failure.

On Thu, Dec 16, 2010 at 5:34 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
>
> On Wed, Dec 15, 2010 at 05:45:13PM -0600, Andrei Warkentin wrote:
> > This is my first time on linux-arm-kernel, and while I've read the
> > FAQ, hopefully I don't screw up too badly :).
> >
> > Anyway, we're on a dual-core ARMv7 running 2.6.36, and during
> > stability stress testing saw the following:
> > 1) After a number hotplug iterations, CPU1 fails to set its online bit
> > quickly enough and__cpu_up() times-out.
> > 2) CPU1 eventually completes its startup and sets the bit, however,
> > since_cpu_up() failed, CPU1's active bit is never set.
>
> Why is your CPU taking soo long to come up? ?We wait one second in the
> generic code, which is the time taken from the platform code being happy
> that it has successfully started the CPU. ?Normally, platforms wait an
> additional second to detect the CPU entering the kernel.

It seems twd_calibrate_rate is the culprit (although in our case,
since the clock is the same to both CPUs, there is no point in
calibrating).
We've seen this only when the device was under stress test load.

>
> > 2) Additionally I ensure that if the CPU comes up later than it were
> > supposed to (shouldn't, but...), then it will not start initializing
> > behind cpu_up's back (which is not really undoable). This solves the
> > problem with both cpu_up+secondary_start_kernel races and with
> > platform_cpu_kill+secondary_start_kernel races.
>
> Why would you have platform_cpu_kill() running at the same time - firstly,
> hotplug events are serialized, and secondly the platform_cpu_kill() path
> should wait up to five seconds for the CPU to go offline. ?If it doesn't
> go offline within five seconds it's dead (and maybe we should mark it
> not present.)
>

That's the platform_cpu_kill I invoke when I time out waiting for the
online bit. Sorry, wasn't being clear. Just trying
to show I didn't introduce any races :).

See, the SMP logic is sensitive to system load at the moment. Since
boot_secondary is supposed to return failure on failing
to up the secondary, maybe there is no point doing a timed wait for
the online bit, since you are guaranteed to get there.
But right now, you end up in a situation where there is a timeout, but
the CPU is up and running and registered.
And this causes bad behavior later when you try to down it.

>_______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC] Make SMP secondary CPU up more resilient to failure.

[Andrei Warkentin]

On Thu, Dec 16, 2010 at 5:28 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Dec 16, 2010 at 05:09:48PM -0600, Andrei Warkentin wrote:
>> On Thu, Dec 16, 2010 at 5:34 AM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> >
>> > On Wed, Dec 15, 2010 at 05:45:13PM -0600, Andrei Warkentin wrote:
>> > > This is my first time on linux-arm-kernel, and while I've read the
>> > > FAQ, hopefully I don't screw up too badly :).
>> > >
>> > > Anyway, we're on a dual-core ARMv7 running 2.6.36, and during
>> > > stability stress testing saw the following:
>> > > 1) After a number hotplug iterations, CPU1 fails to set its online bit
>> > > quickly enough and __cpu_up() times-out.
>> > > 2) CPU1 eventually completes its startup and sets the bit, however,
>> > > since _cpu_up() failed, CPU1's active bit is never set.
>> >
>> > Why is your CPU taking soo long to come up? ?We wait one second in the
>> > generic code, which is the time taken from the platform code being happy
>> > that it has successfully started the CPU. ?Normally, platforms wait an
>> > additional second to detect the CPU entering the kernel.
>>
>> It seems twd_calibrate_rate is the culprit (although in our case,
>> since the clock is the same to both CPUs, there is no point in
>> calibrating).
>
> twd_calibrate_rate() should only run once at boot. ?Once it's run,
> taking CPUs offline and back online should not cause the rate to be
> recalibrated.

Let's me just see if I understand things correctly for the hotplug case.

1) cpu_down calls take_down_cpu
2) Idle thread on secondary notices cpu_is_offline, and calls cpu_die()
3) cpu_die calls platform_cpu_die, at which point the cpu is dead. If
it ever wakes up (because of a cpu_up), it will continue to run in
cpu_die.
4) cpu_die jump to secondary_start_kernel.
5) secondary_start_kernel calls percpu_timer_setup
6)  percpu_timer_setup calls platform local_timer_setup
7) local_timer_setup calls twd_timer_setup_scalable

...which calls __twd_timer_setup, which does twd_calibrate_rate among
other things.
It also does clockevents_register_device.

I didn't try, but looking at kernel/time/clockevents.c,
clockevents_exchange_device is called from tick_shutdown in the
CPU_DEAD notify path, so as is, twd_timer_setup does need to be called
in UP path, even on hotplug.

>
>> See, the SMP logic is sensitive to system load at the moment.
>
> I don't think it is - it sounds like you're explicitly causing the twd
> rate to be recalculated every time you're bringing a CPU online, which
> is not supposed to happen.
>
>> Since boot_secondary is supposed to return failure on failing
>> to up the secondary, maybe there is no point doing a timed wait for
>> the online bit, since you are guaranteed to get there.
>
> If you're starving the secondary CPU of soo much bus bandwidth that it's
> taking more than one second for it to be marked online, the delay loop
> calibration is going to fail too. ?If you can starve it of bus bandwidth
> from the primary CPU, then you have badly designed hardware too - you'll
> gain very little benefit from a SMP system if you can't sensibly run both
> CPUs at the same time without starving one or other.

I can't argue with you there :). But I didn't design the hardware, and
I'm not the only poor sod dealing with it ;). I wish I had another
MPCore Cortex A9 platform nearby so I could replicate the test
results. Unfortunately I don't. This particular issue only happens
about once every four hours while running stress tests that
effectively wedge the entire system, starving it both computationally
and memory/IO-wise.

Anyway, the point I want to make, is that whatever happens in
__cpu_up, it shouldn't ever cause the system to be put into an
inconsistent state, where subsequent oopses will result from from the
secondary having booted up after __cpu_up timed out. At the very
least, it makes looking for these strange up issues (whether they are
caused by HW issues or bad programming) non-trivial.

>
> What I'm saying is that if it's taking more than one second to setup
> the local timer (which should be a few register writes) and calibrate
> the delay loop, you're going to have bigger problems and your system is
> already in an unstable situation.
>
> Please post your SMP support code so it can be reviewed, which'll help
> eliminate it from being the cause.
>

Sure. It's mach-tegra/platsmp.c.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: platsmp.c
Type: text/x-csrc
Size: 6189 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20101217/b85cf3d2/attachment.bin>

[RFC] Make SMP secondary CPU up more resilient to failure.

[Russell King - ARM Linux]

On Fri, Dec 17, 2010 at 02:52:29PM -0600, Andrei Warkentin wrote:
> On Thu, Dec 16, 2010 at 5:28 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Thu, Dec 16, 2010 at 05:09:48PM -0600, Andrei Warkentin wrote:
> >> On Thu, Dec 16, 2010 at 5:34 AM, Russell King - ARM Linux
> >> <linux@arm.linux.org.uk> wrote:
> >> >
> >> > On Wed, Dec 15, 2010 at 05:45:13PM -0600, Andrei Warkentin wrote:
> >> > > This is my first time on linux-arm-kernel, and while I've read the
> >> > > FAQ, hopefully I don't screw up too badly :).
> >> > >
> >> > > Anyway, we're on a dual-core ARMv7 running 2.6.36, and during
> >> > > stability stress testing saw the following:
> >> > > 1) After a number hotplug iterations, CPU1 fails to set its online bit
> >> > > quickly enough and __cpu_up() times-out.
> >> > > 2) CPU1 eventually completes its startup and sets the bit, however,
> >> > > since _cpu_up() failed, CPU1's active bit is never set.
> >> >
> >> > Why is your CPU taking soo long to come up? ?We wait one second in the
> >> > generic code, which is the time taken from the platform code being happy
> >> > that it has successfully started the CPU. ?Normally, platforms wait an
> >> > additional second to detect the CPU entering the kernel.
> >>
> >> It seems twd_calibrate_rate is the culprit (although in our case,
> >> since the clock is the same to both CPUs, there is no point in
> >> calibrating).
> >
> > twd_calibrate_rate() should only run once at boot. ?Once it's run,
> > taking CPUs offline and back online should not cause the rate to be
> > recalibrated.
> 
> Let's me just see if I understand things correctly for the hotplug case.
> 
> 1) cpu_down calls take_down_cpu
> 2) Idle thread on secondary notices cpu_is_offline, and calls cpu_die()
> 3) cpu_die calls platform_cpu_die, at which point the cpu is dead.

Correct so far.

> If it ever wakes up (because of a cpu_up), it will continue to run in
> cpu_die.

That depends what you have in your cpu_die().  If you return from
cpu_die() then we assume that is because you have been woken up, and
so we re-initialize the CPU.

That's because existing hotplug implementations don't have the necessary
support hardware to reset just one CPU, and the only way to bring a CPU
back online is to jump back to the secondary startup.

> 4) cpu_die jump to secondary_start_kernel.

This is for dumb implementations.  If your code takes the CPU offline via
hardware means, then you must _never_ return from cpu_die(), even if the
hardware fails to take the CPU offline.  I suspect this is where your
problem lies.

> 5) secondary_start_kernel calls percpu_timer_setup
> 6)  percpu_timer_setup calls platform local_timer_setup
> 7) local_timer_setup calls twd_timer_setup_scalable
> 
> ...which calls __twd_timer_setup, which does twd_calibrate_rate among
> other things.

Again, correct, and this is where you claimed that excessive time was
being spent.  Looking at twd_calibrate_rate(), it has this:

        if (twd_timer_rate == 0) {
                printk(KERN_INFO "Calibrating local timer... ");
...
                twd_timer_rate = (0xFFFFFFFFU - count) * (HZ / 5);

                printk("%lu.%02luMHz.\n", twd_timer_rate / 1000000,
                        (twd_timer_rate / 100000) % 100);
        }

which, on the first and _only_ first pass through, initializes
twd_timer_rate.  Because on hotplug cpu-up the timer rate has already
been calibrated, we won't re-run the calibration, and we won't spend
lots of time here.

> #ifdef CONFIG_HOTPLUG_CPU
> static DEFINE_PER_CPU(struct completion, cpu_killed);
> extern void tegra_hotplug_startup(void);
> #endif

You do not need cpu_killed to be a per-cpu thing.  The locking in
cpu_up() and cpu_down() ensures that you can not take more than one
CPU up or down concurrently.  See cpu_maps_update_begin() which
acquires the lock, and cpu_maps_update_done() which drops the lock.

> static DECLARE_BITMAP(cpu_init_bits, CONFIG_NR_CPUS) __read_mostly;
> const struct cpumask *const cpu_init_mask = to_cpumask(cpu_init_bits);
> #define cpu_init_map (*(cpumask_t *)cpu_init_mask)
> 
> #define EVP_CPU_RESET_VECTOR \
> 	(IO_ADDRESS(TEGRA_EXCEPTION_VECTORS_BASE) + 0x100)
> #define CLK_RST_CONTROLLER_CLK_CPU_CMPLX \
> 	(IO_ADDRESS(TEGRA_CLK_RESET_BASE) + 0x4c)
> #define CLK_RST_CONTROLLER_RST_CPU_CMPLX_SET \
> 	(IO_ADDRESS(TEGRA_CLK_RESET_BASE) + 0x340)
> #define CLK_RST_CONTROLLER_RST_CPU_CMPLX_CLR \
> 	(IO_ADDRESS(TEGRA_CLK_RESET_BASE) + 0x344)
> 
> void __cpuinit platform_secondary_init(unsigned int cpu)
> {
> 	trace_hardirqs_off();
> 	gic_cpu_init(0, IO_ADDRESS(TEGRA_ARM_PERIF_BASE) + 0x100);
> 	/*
> 	 * Synchronise with the boot thread.
> 	 */
> 	spin_lock(&boot_lock);
> #ifdef CONFIG_HOTPLUG_CPU
> 	cpu_set(cpu, cpu_init_map);
> 	INIT_COMPLETION(per_cpu(cpu_killed, cpu));

This means you don't need to re-initialize the completion.  Just define
it once using DEFINE_COMPLETION().  Note that this is something that will
be taken care of in core SMP hotplug code during the next merge window.

> #endif
> 	spin_unlock(&boot_lock);
> }
> 
> int __cpuinit boot_secondary(unsigned int cpu, struct task_struct *idle)
> {
> 	unsigned long old_boot_vector;
> 	unsigned long boot_vector;
> 	unsigned long timeout;
> 	u32 reg;
> 
> 	/*
> 	 * set synchronisation state between this boot processor
> 	 * and the secondary one
> 	 */
> 	spin_lock(&boot_lock);
> 
> 	/* set the reset vector to point to the secondary_startup routine */
> #ifdef CONFIG_HOTPLUG_CPU
> 	if (cpumask_test_cpu(cpu, cpu_init_mask))
> 		boot_vector = virt_to_phys(tegra_hotplug_startup);
> 	else
> #endif
> 		boot_vector = virt_to_phys(tegra_secondary_startup);

I didn't see the code for these startup functions.

> 
> 	smp_wmb();
> 
> 	old_boot_vector = readl(EVP_CPU_RESET_VECTOR);
> 	writel(boot_vector, EVP_CPU_RESET_VECTOR);
> 
> 	/* enable cpu clock on cpu */
> 	reg = readl(CLK_RST_CONTROLLER_CLK_CPU_CMPLX);
> 	writel(reg & ~(1<<(8+cpu)), CLK_RST_CONTROLLER_CLK_CPU_CMPLX);
> 
> 	reg = 0x1111<<cpu;
> 	writel(reg, CLK_RST_CONTROLLER_RST_CPU_CMPLX_CLR);
> 
> 	/* unhalt the cpu */
> 	writel(0, IO_ADDRESS(TEGRA_FLOW_CTRL_BASE) + 0x14 + 0x8*(cpu-1));
> 
> 	timeout = jiffies + HZ;
> 	while (time_before(jiffies, timeout)) {
> 		if (readl(EVP_CPU_RESET_VECTOR) != boot_vector)

At what point does the hardware change the reset vector?

> 			break;
> 		udelay(10);
> 	}
> 
> 	/* put the old boot vector back */
> 	writel(old_boot_vector, EVP_CPU_RESET_VECTOR);
> 
> 	/*
> 	 * now the secondary core is starting up let it run its
> 	 * calibrations, then wait for it to finish
> 	 */
> 	spin_unlock(&boot_lock);
> 
> 	return 0;
> }
> 
> /*
>  * Initialise the CPU possible map early - this describes the CPUs
>  * which may be present or become present in the system.
>  */
> void __init smp_init_cpus(void)
> {
> 	unsigned int i, ncores = scu_get_core_count(scu_base);
> 
> 	for (i = 0; i < ncores; i++)
> 		cpu_set(i, cpu_possible_map);
> }
> 
> void __init smp_prepare_cpus(unsigned int max_cpus)
> {
> 	unsigned int ncores = scu_get_core_count(scu_base);
> 	unsigned int cpu = smp_processor_id();
> 	int i;
> 
> 	smp_store_cpu_info(cpu);
> 
> 	/*
> 	 * are we trying to boot more cores than exist?
> 	 */
> 	if (max_cpus > ncores)
> 		max_cpus = ncores;
> 
> 	/*
> 	 * Initialise the present map, which describes the set of CPUs
> 	 * actually populated at the present time.
> 	 */
> 	for (i = 0; i < max_cpus; i++)
> 		set_cpu_present(i, true);
> 
> #ifdef CONFIG_HOTPLUG_CPU
> 	for_each_present_cpu(i) {
> 		init_completion(&per_cpu(cpu_killed, i));
> 	}

Again, no need for this with the comments I've made above wrt it.

> #endif
> 
> 	/*
> 	 * Initialise the SCU if there are more than one CPU and let
> 	 * them know where to start. Note that, on modern versions of
> 	 * MILO, the "poke" doesn't actually do anything until each
> 	 * individual core is sent a soft interrupt to get it out of
> 	 * WFI
> 	 */
> 	if (max_cpus > 1) {
> 		percpu_timer_setup();
> 		scu_enable(scu_base);
> 	}
> }
> 
> #ifdef CONFIG_HOTPLUG_CPU
> 
> extern void vfp_sync_state(struct thread_info *thread);
> 
> void __cpuinit secondary_start_kernel(void);
> 
> int platform_cpu_kill(unsigned int cpu)
> {
> 	unsigned int reg;
> 	int e;
> 
> 	e = wait_for_completion_timeout(&per_cpu(cpu_killed, cpu), 100);
> 	printk(KERN_NOTICE "CPU%u: %s shutdown\n", cpu, (e) ? "clean":"forced");

So, this timeout '100' - how long exactly is that?  It's dependent on
the jiffy tick interval rather than being defined in ms.  (Oops, that's
something which should also be fixed in version in the generic code.)

> 
> 	if (e) {
> 		do {
> 			reg = readl(CLK_RST_CONTROLLER_RST_CPU_CMPLX_SET);
> 			cpu_relax();
> 		} while (!(reg & (1<<cpu)));
> 	} else {
> 		writel(0x1111<<cpu, CLK_RST_CONTROLLER_RST_CPU_CMPLX_SET);
> 		/* put flow controller in WAIT_EVENT mode */
> 		writel(2<<29, IO_ADDRESS(TEGRA_FLOW_CTRL_BASE)+0x14 + 0x8*(cpu-1));
> 	}
> 	spin_lock(&boot_lock);
> 	reg = readl(CLK_RST_CONTROLLER_CLK_CPU_CMPLX);
> 	writel(reg | (1<<(8+cpu)), CLK_RST_CONTROLLER_CLK_CPU_CMPLX);
> 	spin_unlock(&boot_lock);
> 	return e;
> }
> 
> void platform_cpu_die(unsigned int cpu)
> {
> #ifdef DEBUG
> 	unsigned int this_cpu = hard_smp_processor_id();
> 
> 	if (cpu != this_cpu) {
> 		printk(KERN_CRIT "Eek! platform_cpu_die running on %u, should be %u\n",
> 			   this_cpu, cpu);
> 		BUG();
> 	}
> #endif
> 
> 	gic_cpu_exit(0);
> 	barrier();
> 	complete(&per_cpu(cpu_killed, cpu));
> 	flush_cache_all();
> 	barrier();
> 	__cortex_a9_save(0);
> 
> 	/* return happens from __cortex_a9_restore */
> 	barrier();
> 	writel(smp_processor_id(), EVP_CPU_RESET_VECTOR);

And here, if the CPU is not _immediately_ stopped, it will return and
restart the bring-up.  If you are using hardware to reset the CPU (which
it seems you are), you should ensure that this function never returns.
You may also wish to add a pr_crit() here to indicate when this failure
happens.

> }
> 
> int platform_cpu_disable(unsigned int cpu)
> {
> 	/*
> 	 * we don't allow CPU 0 to be shutdown (it is still too special
> 	 * e.g. clock tick interrupts)
> 	 */
> 	if (unlikely(!tegra_context_area))
> 		return -ENXIO;
> 
> 	return cpu == 0 ? -EPERM : 0;
> }
> #endif

[RFC] Make SMP secondary CPU up more resilient to failure.

[Andrei Warkentin]

On Fri, Dec 17, 2010 at 5:14 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Dec 17, 2010 at 02:52:29PM -0600, Andrei Warkentin wrote:
>> On Thu, Dec 16, 2010 at 5:28 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > On Thu, Dec 16, 2010 at 05:09:48PM -0600, Andrei Warkentin wrote:
>> >> On Thu, Dec 16, 2010 at 5:34 AM, Russell King - ARM Linux
>> >> <linux@arm.linux.org.uk> wrote:
>> >> >
>> >> > On Wed, Dec 15, 2010 at 05:45:13PM -0600, Andrei Warkentin wrote:
>> >> > > This is my first time on linux-arm-kernel, and while I've read the
>> >> > > FAQ, hopefully I don't screw up too badly :).
>> >> > >
>> >> > > Anyway, we're on a dual-core ARMv7 running 2.6.36, and during
>> >> > > stability stress testing saw the following:
>> >> > > 1) After a number hotplug iterations, CPU1 fails to set its online bit
>> >> > > quickly enough and __cpu_up() times-out.
>> >> > > 2) CPU1 eventually completes its startup and sets the bit, however,
>> >> > > since _cpu_up() failed, CPU1's active bit is never set.
>> >> >
>> >> > Why is your CPU taking soo long to come up? ?We wait one second in the
>> >> > generic code, which is the time taken from the platform code being happy
>> >> > that it has successfully started the CPU. ?Normally, platforms wait an
>> >> > additional second to detect the CPU entering the kernel.
>> >>
>> >> It seems twd_calibrate_rate is the culprit (although in our case,
>> >> since the clock is the same to both CPUs, there is no point in
>> >> calibrating).
>> >
>> > twd_calibrate_rate() should only run once at boot. ?Once it's run,
>> > taking CPUs offline and back online should not cause the rate to be
>> > recalibrated.
>>
>> Let's me just see if I understand things correctly for the hotplug case.
>>
>> 1) cpu_down calls take_down_cpu
>> 2) Idle thread on secondary notices cpu_is_offline, and calls cpu_die()
>> 3) cpu_die calls platform_cpu_die, at which point the cpu is dead.
>
> Correct so far.
>
>> If it ever wakes up (because of a cpu_up), it will continue to run in
>> cpu_die.
>
> That depends what you have in your cpu_die(). ?If you return from
> cpu_die() then we assume that is because you have been woken up, and
> so we re-initialize the CPU.
>

That's correct. Same applies to us. When we wake up a CPU, it is going
to return back to cpu_die, and then call secondary startup to join the
rest.

> That's because existing hotplug implementations don't have the necessary
> support hardware to reset just one CPU, and the only way to bring a CPU
> back online is to jump back to the secondary startup.
>
>> 4) cpu_die jump to secondary_start_kernel.
>
> This is for dumb implementations. ?If your code takes the CPU offline via
> hardware means, then you must _never_ return from cpu_die(), even if the
> hardware fails to take the CPU offline. ?I suspect this is where your
> problem lies.
>

No, we don't have a problem shutting cores down. We gate the clock and
set the reset line, but when it comes back up all state will be
restored as it were prior to the getting turned off, so it can be
resumed back properly.


>> 5) secondary_start_kernel calls percpu_timer_setup
>> 6) ?percpu_timer_setup calls platform local_timer_setup
>> 7) local_timer_setup calls twd_timer_setup_scalable
>>
>> ...which calls __twd_timer_setup, which does twd_calibrate_rate among
>> other things.
>
> Again, correct, and this is where you claimed that excessive time was
> being spent. ?Looking at twd_calibrate_rate(), it has this:
>
> ? ? ? ?if (twd_timer_rate == 0) {
> ? ? ? ? ? ? ? ?printk(KERN_INFO "Calibrating local timer... ");
> ...
> ? ? ? ? ? ? ? ?twd_timer_rate = (0xFFFFFFFFU - count) * (HZ / 5);
>
> ? ? ? ? ? ? ? ?printk("%lu.%02luMHz.\n", twd_timer_rate / 1000000,
> ? ? ? ? ? ? ? ? ? ? ? ?(twd_timer_rate / 100000) % 100);
> ? ? ? ?}
>
> which, on the first and _only_ first pass through, initializes
> twd_timer_rate. ?Because on hotplug cpu-up the timer rate has already
> been calibrated, we won't re-run the calibration, and we won't spend
> lots of time here.
>

Sorry, I feel like a moron. Didn't realize that in our codebase there
is a change explicitly always doing the calibration.
I'll see if this is necessary.

This still doesn't explain why the ARM SMP code shouldn't be more
resilient (i.e. secondary startup doesn't do anything unless __cpu_up
wants it to)

>> #ifdef CONFIG_HOTPLUG_CPU
>> static DEFINE_PER_CPU(struct completion, cpu_killed);
>> extern void tegra_hotplug_startup(void);
>> #endif
>
> You do not need cpu_killed to be a per-cpu thing. ?The locking in
> cpu_up() and cpu_down() ensures that you can not take more than one
> CPU up or down concurrently. ?See cpu_maps_update_begin() which
> acquires the lock, and cpu_maps_update_done() which drops the lock.
>
>> static DECLARE_BITMAP(cpu_init_bits, CONFIG_NR_CPUS) __read_mostly;
>> const struct cpumask *const cpu_init_mask = to_cpumask(cpu_init_bits);
>> #define cpu_init_map (*(cpumask_t *)cpu_init_mask)
>>
>> #define EVP_CPU_RESET_VECTOR \
>> ? ? ? (IO_ADDRESS(TEGRA_EXCEPTION_VECTORS_BASE) + 0x100)
>> #define CLK_RST_CONTROLLER_CLK_CPU_CMPLX \
>> ? ? ? (IO_ADDRESS(TEGRA_CLK_RESET_BASE) + 0x4c)
>> #define CLK_RST_CONTROLLER_RST_CPU_CMPLX_SET \
>> ? ? ? (IO_ADDRESS(TEGRA_CLK_RESET_BASE) + 0x340)
>> #define CLK_RST_CONTROLLER_RST_CPU_CMPLX_CLR \
>> ? ? ? (IO_ADDRESS(TEGRA_CLK_RESET_BASE) + 0x344)
>>
>> void __cpuinit platform_secondary_init(unsigned int cpu)
>> {
>> ? ? ? trace_hardirqs_off();
>> ? ? ? gic_cpu_init(0, IO_ADDRESS(TEGRA_ARM_PERIF_BASE) + 0x100);
>> ? ? ? /*
>> ? ? ? ?* Synchronise with the boot thread.
>> ? ? ? ?*/
>> ? ? ? spin_lock(&boot_lock);
>> #ifdef CONFIG_HOTPLUG_CPU
>> ? ? ? cpu_set(cpu, cpu_init_map);
>> ? ? ? INIT_COMPLETION(per_cpu(cpu_killed, cpu));
>
> This means you don't need to re-initialize the completion. ?Just define
> it once using DEFINE_COMPLETION(). ?Note that this is something that will
> be taken care of in core SMP hotplug code during the next merge window.
>

Ah ok, thanks!

>> #endif
>> ? ? ? spin_unlock(&boot_lock);
>> }
>>
>> int __cpuinit boot_secondary(unsigned int cpu, struct task_struct *idle)
>> {
>> ? ? ? unsigned long old_boot_vector;
>> ? ? ? unsigned long boot_vector;
>> ? ? ? unsigned long timeout;
>> ? ? ? u32 reg;
>>
>> ? ? ? /*
>> ? ? ? ?* set synchronisation state between this boot processor
>> ? ? ? ?* and the secondary one
>> ? ? ? ?*/
>> ? ? ? spin_lock(&boot_lock);
>>
>> ? ? ? /* set the reset vector to point to the secondary_startup routine */
>> #ifdef CONFIG_HOTPLUG_CPU
>> ? ? ? if (cpumask_test_cpu(cpu, cpu_init_mask))
>> ? ? ? ? ? ? ? boot_vector = virt_to_phys(tegra_hotplug_startup);
>> ? ? ? else
>> #endif
>> ? ? ? ? ? ? ? boot_vector = virt_to_phys(tegra_secondary_startup);
>
> I didn't see the code for these startup functions.
>

ENTRY(tegra_secondary_startup)
        msr     cpsr_fsxc, #0xd3
        bl      __invalidate_cpu_state
        cpu_id  r0
        enable_coresite r1
        poke_ev r0, r1                        <--------- updates reset
vector with up'ped CPU
        b       secondary_startup

ENTRY(tegra_hotplug_startup)
        setmode PSR_F_BIT | PSR_I_BIT | SVC_MODE, r9
        bl      __invalidate_cpu_state
        enable_coresite r1

        /* most of the below is a retread of what happens in __v7_setup and
         * secondary_startup, to get the MMU re-enabled and to branch
         * to secondary_kernel_startup */
        mrc     p15, 0, r0, c1, c0, 1
        orr     r0, r0, #(1 << 6) | (1 << 0)    @ re-enable coherency
        mcr     p15, 0, r0, c1, c0, 1

        adr     r4, __tegra_hotplug_data
        ldmia   r4, {r5, r7, r12}
        mov     r1, r12                 @ ctx_restore = __cortex_a9_restore
        sub     r4, r4, r5
        ldr     r0, [r7, r4]            @ pgdir = secondary_data.pgdir
        b       __return_to_virtual
ENDPROC(tegra_hotplug_startup)

Effectively this restores what __cortex_a9_save did before it shut down the CPU.

>>
>> ? ? ? smp_wmb();
>>
>> ? ? ? old_boot_vector = readl(EVP_CPU_RESET_VECTOR);
>> ? ? ? writel(boot_vector, EVP_CPU_RESET_VECTOR);
>>
>> ? ? ? /* enable cpu clock on cpu */
>> ? ? ? reg = readl(CLK_RST_CONTROLLER_CLK_CPU_CMPLX);
>> ? ? ? writel(reg & ~(1<<(8+cpu)), CLK_RST_CONTROLLER_CLK_CPU_CMPLX);
>>
>> ? ? ? reg = 0x1111<<cpu;
>> ? ? ? writel(reg, CLK_RST_CONTROLLER_RST_CPU_CMPLX_CLR);
>>
>> ? ? ? /* unhalt the cpu */
>> ? ? ? writel(0, IO_ADDRESS(TEGRA_FLOW_CTRL_BASE) + 0x14 + 0x8*(cpu-1));
>>
>> ? ? ? timeout = jiffies + HZ;
>> ? ? ? while (time_before(jiffies, timeout)) {
>> ? ? ? ? ? ? ? if (readl(EVP_CPU_RESET_VECTOR) != boot_vector)
>
> At what point does the hardware change the reset vector?

Not hardware. it's either done inside tegra_secondary_startup or
inside platform_cpu_die after the dead CPU comes back to life on an
cpu_up.

>
>> ? ? ? ? ? ? ? ? ? ? ? break;
>> ? ? ? ? ? ? ? udelay(10);
>> ? ? ? }
>>
>> ? ? ? /* put the old boot vector back */
>> ? ? ? writel(old_boot_vector, EVP_CPU_RESET_VECTOR);
>>
>> ? ? ? /*
>> ? ? ? ?* now the secondary core is starting up let it run its
>> ? ? ? ?* calibrations, then wait for it to finish
>> ? ? ? ?*/
>> ? ? ? spin_unlock(&boot_lock);
>>
>> ? ? ? return 0;
>> }
>>
>> /*
>> ?* Initialise the CPU possible map early - this describes the CPUs
>> ?* which may be present or become present in the system.
>> ?*/
>> void __init smp_init_cpus(void)
>> {
>> ? ? ? unsigned int i, ncores = scu_get_core_count(scu_base);
>>
>> ? ? ? for (i = 0; i < ncores; i++)
>> ? ? ? ? ? ? ? cpu_set(i, cpu_possible_map);
>> }
>>
>> void __init smp_prepare_cpus(unsigned int max_cpus)
>> {
>> ? ? ? unsigned int ncores = scu_get_core_count(scu_base);
>> ? ? ? unsigned int cpu = smp_processor_id();
>> ? ? ? int i;
>>
>> ? ? ? smp_store_cpu_info(cpu);
>>
>> ? ? ? /*
>> ? ? ? ?* are we trying to boot more cores than exist?
>> ? ? ? ?*/
>> ? ? ? if (max_cpus > ncores)
>> ? ? ? ? ? ? ? max_cpus = ncores;
>>
>> ? ? ? /*
>> ? ? ? ?* Initialise the present map, which describes the set of CPUs
>> ? ? ? ?* actually populated at the present time.
>> ? ? ? ?*/
>> ? ? ? for (i = 0; i < max_cpus; i++)
>> ? ? ? ? ? ? ? set_cpu_present(i, true);
>>
>> #ifdef CONFIG_HOTPLUG_CPU
>> ? ? ? for_each_present_cpu(i) {
>> ? ? ? ? ? ? ? init_completion(&per_cpu(cpu_killed, i));
>> ? ? ? }
>
> Again, no need for this with the comments I've made above wrt it.
>
>> #endif
>>
>> ? ? ? /*
>> ? ? ? ?* Initialise the SCU if there are more than one CPU and let
>> ? ? ? ?* them know where to start. Note that, on modern versions of
>> ? ? ? ?* MILO, the "poke" doesn't actually do anything until each
>> ? ? ? ?* individual core is sent a soft interrupt to get it out of
>> ? ? ? ?* WFI
>> ? ? ? ?*/
>> ? ? ? if (max_cpus > 1) {
>> ? ? ? ? ? ? ? percpu_timer_setup();
>> ? ? ? ? ? ? ? scu_enable(scu_base);
>> ? ? ? }
>> }
>>
>> #ifdef CONFIG_HOTPLUG_CPU
>>
>> extern void vfp_sync_state(struct thread_info *thread);
>>
>> void __cpuinit secondary_start_kernel(void);
>>
>> int platform_cpu_kill(unsigned int cpu)
>> {
>> ? ? ? unsigned int reg;
>> ? ? ? int e;
>>
>> ? ? ? e = wait_for_completion_timeout(&per_cpu(cpu_killed, cpu), 100);
>> ? ? ? printk(KERN_NOTICE "CPU%u: %s shutdown\n", cpu, (e) ? "clean":"forced");
>
> So, this timeout '100' - how long exactly is that? ?It's dependent on
> the jiffy tick interval rather than being defined in ms. ?(Oops, that's
> something which should also be fixed in version in the generic code.)
>
>>
>> ? ? ? if (e) {
>> ? ? ? ? ? ? ? do {
>> ? ? ? ? ? ? ? ? ? ? ? reg = readl(CLK_RST_CONTROLLER_RST_CPU_CMPLX_SET);
>> ? ? ? ? ? ? ? ? ? ? ? cpu_relax();
>> ? ? ? ? ? ? ? } while (!(reg & (1<<cpu)));
>> ? ? ? } else {
>> ? ? ? ? ? ? ? writel(0x1111<<cpu, CLK_RST_CONTROLLER_RST_CPU_CMPLX_SET);
>> ? ? ? ? ? ? ? /* put flow controller in WAIT_EVENT mode */
>> ? ? ? ? ? ? ? writel(2<<29, IO_ADDRESS(TEGRA_FLOW_CTRL_BASE)+0x14 + 0x8*(cpu-1));
>> ? ? ? }
>> ? ? ? spin_lock(&boot_lock);
>> ? ? ? reg = readl(CLK_RST_CONTROLLER_CLK_CPU_CMPLX);
>> ? ? ? writel(reg | (1<<(8+cpu)), CLK_RST_CONTROLLER_CLK_CPU_CMPLX);
>> ? ? ? spin_unlock(&boot_lock);
>> ? ? ? return e;
>> }
>>
>> void platform_cpu_die(unsigned int cpu)
>> {
>> #ifdef DEBUG
>> ? ? ? unsigned int this_cpu = hard_smp_processor_id();
>>
>> ? ? ? if (cpu != this_cpu) {
>> ? ? ? ? ? ? ? printk(KERN_CRIT "Eek! platform_cpu_die running on %u, should be %u\n",
>> ? ? ? ? ? ? ? ? ? ? ? ? ?this_cpu, cpu);
>> ? ? ? ? ? ? ? BUG();
>> ? ? ? }
>> #endif
>>
>> ? ? ? gic_cpu_exit(0);
>> ? ? ? barrier();
>> ? ? ? complete(&per_cpu(cpu_killed, cpu));
>> ? ? ? flush_cache_all();
>> ? ? ? barrier();
>> ? ? ? __cortex_a9_save(0);
>>
>> ? ? ? /* return happens from __cortex_a9_restore */
>> ? ? ? barrier();
>> ? ? ? writel(smp_processor_id(), EVP_CPU_RESET_VECTOR);
>
> And here, if the CPU is not _immediately_ stopped, it will return and
> restart the bring-up. ?If you are using hardware to reset the CPU (which
> it seems you are), you should ensure that this function never returns.
> You may also wish to add a pr_crit() here to indicate when this failure
> happens.
>

It is immediately stopped. Turned off really. But this is where it
will resume on when it gets brought back up (__cortex_a9_restore takes
care of that)

>> }
>>
>> int platform_cpu_disable(unsigned int cpu)
>> {
>> ? ? ? /*
>> ? ? ? ?* we don't allow CPU 0 to be shutdown (it is still too special
>> ? ? ? ?* e.g. clock tick interrupts)
>> ? ? ? ?*/
>> ? ? ? if (unlikely(!tegra_context_area))
>> ? ? ? ? ? ? ? return -ENXIO;
>>
>> ? ? ? return cpu == 0 ? -EPERM : 0;
>> }
>> #endif
>

[RFC] Make SMP secondary CPU up more resilient to failure.

[Russell King - ARM Linux]

On Fri, Dec 17, 2010 at 05:45:09PM -0600, Andrei Warkentin wrote:
> On Fri, Dec 17, 2010 at 5:14 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Fri, Dec 17, 2010 at 02:52:29PM -0600, Andrei Warkentin wrote:
> >> On Thu, Dec 16, 2010 at 5:28 PM, Russell King - ARM Linux
> >> <linux@arm.linux.org.uk> wrote:
> >> > On Thu, Dec 16, 2010 at 05:09:48PM -0600, Andrei Warkentin wrote:
> >> >> On Thu, Dec 16, 2010 at 5:34 AM, Russell King - ARM Linux
> >> >> <linux@arm.linux.org.uk> wrote:
> >> >> >
> >> >> > On Wed, Dec 15, 2010 at 05:45:13PM -0600, Andrei Warkentin wrote:
> >> >> > > This is my first time on linux-arm-kernel, and while I've read the
> >> >> > > FAQ, hopefully I don't screw up too badly :).
> >> >> > >
> >> >> > > Anyway, we're on a dual-core ARMv7 running 2.6.36, and during
> >> >> > > stability stress testing saw the following:
> >> >> > > 1) After a number hotplug iterations, CPU1 fails to set its online bit
> >> >> > > quickly enough and __cpu_up() times-out.
> >> >> > > 2) CPU1 eventually completes its startup and sets the bit, however,
> >> >> > > since _cpu_up() failed, CPU1's active bit is never set.
> >> >> >
> >> >> > Why is your CPU taking soo long to come up? ?We wait one second in the
> >> >> > generic code, which is the time taken from the platform code being happy
> >> >> > that it has successfully started the CPU. ?Normally, platforms wait an
> >> >> > additional second to detect the CPU entering the kernel.
> >> >>
> >> >> It seems twd_calibrate_rate is the culprit (although in our case,
> >> >> since the clock is the same to both CPUs, there is no point in
> >> >> calibrating).
> >> >
> >> > twd_calibrate_rate() should only run once at boot. ?Once it's run,
> >> > taking CPUs offline and back online should not cause the rate to be
> >> > recalibrated.
> >>
> >> Let's me just see if I understand things correctly for the hotplug case.
> >>
> >> 1) cpu_down calls take_down_cpu
> >> 2) Idle thread on secondary notices cpu_is_offline, and calls cpu_die()
> >> 3) cpu_die calls platform_cpu_die, at which point the cpu is dead.
> >
> > Correct so far.
> >
> >> If it ever wakes up (because of a cpu_up), it will continue to run in
> >> cpu_die.
> >
> > That depends what you have in your cpu_die(). ?If you return from
> > cpu_die() then we assume that is because you have been woken up, and
> > so we re-initialize the CPU.
> >
> 
> That's correct. Same applies to us. When we wake up a CPU, it is going
> to return back to cpu_die, and then call secondary startup to join the
> rest.

However, your writel() is not guaranteed to take effect at the point in
time that you issue it.  You're programming a weakly ordered architecture,
you need to be aware that things don't happen at the point in time that
they appear in the program, or sometimes even in the order that you put
them in the program.  (See below.)

> > That's because existing hotplug implementations don't have the necessary
> > support hardware to reset just one CPU, and the only way to bring a CPU
> > back online is to jump back to the secondary startup.
> >
> >> 4) cpu_die jump to secondary_start_kernel.
> >
> > This is for dumb implementations. ?If your code takes the CPU offline via
> > hardware means, then you must _never_ return from cpu_die(), even if the
> > hardware fails to take the CPU offline. ?I suspect this is where your
> > problem lies.
> >
> 
> No, we don't have a problem shutting cores down. We gate the clock and
> set the reset line, but when it comes back up all state will be
> restored as it were prior to the getting turned off, so it can be
> resumed back properly.
> 
> 
> >> 5) secondary_start_kernel calls percpu_timer_setup
> >> 6) ?percpu_timer_setup calls platform local_timer_setup
> >> 7) local_timer_setup calls twd_timer_setup_scalable
> >>
> >> ...which calls __twd_timer_setup, which does twd_calibrate_rate among
> >> other things.
> >
> > Again, correct, and this is where you claimed that excessive time was
> > being spent. ?Looking at twd_calibrate_rate(), it has this:
> >
> > ? ? ? ?if (twd_timer_rate == 0) {
> > ? ? ? ? ? ? ? ?printk(KERN_INFO "Calibrating local timer... ");
> > ...
> > ? ? ? ? ? ? ? ?twd_timer_rate = (0xFFFFFFFFU - count) * (HZ / 5);
> >
> > ? ? ? ? ? ? ? ?printk("%lu.%02luMHz.\n", twd_timer_rate / 1000000,
> > ? ? ? ? ? ? ? ? ? ? ? ?(twd_timer_rate / 100000) % 100);
> > ? ? ? ?}
> >
> > which, on the first and _only_ first pass through, initializes
> > twd_timer_rate. ?Because on hotplug cpu-up the timer rate has already
> > been calibrated, we won't re-run the calibration, and we won't spend
> > lots of time here.
> >
> 
> Sorry, I feel like a moron. Didn't realize that in our codebase there
> is a change explicitly always doing the calibration.
> I'll see if this is necessary.
> 
> This still doesn't explain why the ARM SMP code shouldn't be more
> resilient (i.e. secondary startup doesn't do anything unless __cpu_up
> wants it to)
> 
> >> #ifdef CONFIG_HOTPLUG_CPU
> >> static DEFINE_PER_CPU(struct completion, cpu_killed);
> >> extern void tegra_hotplug_startup(void);
> >> #endif
> >
> > You do not need cpu_killed to be a per-cpu thing. ?The locking in
> > cpu_up() and cpu_down() ensures that you can not take more than one
> > CPU up or down concurrently. ?See cpu_maps_update_begin() which
> > acquires the lock, and cpu_maps_update_done() which drops the lock.
> >
> >> static DECLARE_BITMAP(cpu_init_bits, CONFIG_NR_CPUS) __read_mostly;
> >> const struct cpumask *const cpu_init_mask = to_cpumask(cpu_init_bits);
> >> #define cpu_init_map (*(cpumask_t *)cpu_init_mask)
> >>
> >> #define EVP_CPU_RESET_VECTOR \
> >> ? ? ? (IO_ADDRESS(TEGRA_EXCEPTION_VECTORS_BASE) + 0x100)
> >> #define CLK_RST_CONTROLLER_CLK_CPU_CMPLX \
> >> ? ? ? (IO_ADDRESS(TEGRA_CLK_RESET_BASE) + 0x4c)
> >> #define CLK_RST_CONTROLLER_RST_CPU_CMPLX_SET \
> >> ? ? ? (IO_ADDRESS(TEGRA_CLK_RESET_BASE) + 0x340)
> >> #define CLK_RST_CONTROLLER_RST_CPU_CMPLX_CLR \
> >> ? ? ? (IO_ADDRESS(TEGRA_CLK_RESET_BASE) + 0x344)
> >>
> >> void __cpuinit platform_secondary_init(unsigned int cpu)
> >> {
> >> ? ? ? trace_hardirqs_off();
> >> ? ? ? gic_cpu_init(0, IO_ADDRESS(TEGRA_ARM_PERIF_BASE) + 0x100);
> >> ? ? ? /*
> >> ? ? ? ?* Synchronise with the boot thread.
> >> ? ? ? ?*/
> >> ? ? ? spin_lock(&boot_lock);
> >> #ifdef CONFIG_HOTPLUG_CPU
> >> ? ? ? cpu_set(cpu, cpu_init_map);
> >> ? ? ? INIT_COMPLETION(per_cpu(cpu_killed, cpu));
> >
> > This means you don't need to re-initialize the completion. ?Just define
> > it once using DEFINE_COMPLETION(). ?Note that this is something that will
> > be taken care of in core SMP hotplug code during the next merge window.
> >
> 
> Ah ok, thanks!
> 
> >> #endif
> >> ? ? ? spin_unlock(&boot_lock);
> >> }
> >>
> >> int __cpuinit boot_secondary(unsigned int cpu, struct task_struct *idle)
> >> {
> >> ? ? ? unsigned long old_boot_vector;
> >> ? ? ? unsigned long boot_vector;
> >> ? ? ? unsigned long timeout;
> >> ? ? ? u32 reg;
> >>
> >> ? ? ? /*
> >> ? ? ? ?* set synchronisation state between this boot processor
> >> ? ? ? ?* and the secondary one
> >> ? ? ? ?*/
> >> ? ? ? spin_lock(&boot_lock);
> >>
> >> ? ? ? /* set the reset vector to point to the secondary_startup routine */
> >> #ifdef CONFIG_HOTPLUG_CPU
> >> ? ? ? if (cpumask_test_cpu(cpu, cpu_init_mask))
> >> ? ? ? ? ? ? ? boot_vector = virt_to_phys(tegra_hotplug_startup);
> >> ? ? ? else
> >> #endif
> >> ? ? ? ? ? ? ? boot_vector = virt_to_phys(tegra_secondary_startup);
> >
> > I didn't see the code for these startup functions.
> >
> 
> ENTRY(tegra_secondary_startup)
>         msr     cpsr_fsxc, #0xd3
>         bl      __invalidate_cpu_state
>         cpu_id  r0
>         enable_coresite r1
>         poke_ev r0, r1                        <--------- updates reset
> vector with up'ped CPU
>         b       secondary_startup
> 
> ENTRY(tegra_hotplug_startup)
>         setmode PSR_F_BIT | PSR_I_BIT | SVC_MODE, r9
>         bl      __invalidate_cpu_state
>         enable_coresite r1
> 
>         /* most of the below is a retread of what happens in __v7_setup and
>          * secondary_startup, to get the MMU re-enabled and to branch
>          * to secondary_kernel_startup */
>         mrc     p15, 0, r0, c1, c0, 1
>         orr     r0, r0, #(1 << 6) | (1 << 0)    @ re-enable coherency
>         mcr     p15, 0, r0, c1, c0, 1
> 
>         adr     r4, __tegra_hotplug_data
>         ldmia   r4, {r5, r7, r12}
>         mov     r1, r12                 @ ctx_restore = __cortex_a9_restore
>         sub     r4, r4, r5
>         ldr     r0, [r7, r4]            @ pgdir = secondary_data.pgdir
>         b       __return_to_virtual
> ENDPROC(tegra_hotplug_startup)
> 
> Effectively this restores what __cortex_a9_save did before it shut down the CPU.

Err.  You _really_ do not need all this complexity.  My guess is that
you've looked at what Realview is doing, and are trying to copy the
way it does stuff.  That's really over the top if you have the ability
to place CPU cores back into reset, which it seems you do.

All you need to do when killing a CPU is to put it back into reset.
When you bring the CPU back up, just the same steps you'd run on the
initial CPU bring-up.  In other words, point the reset vector at
tegra_secondary_startup and let it run through the same startup again.

There's (should be) no need to preserve any state for the CPU across
a hot-unplug.  If there is state to be preserved, that's a bug which
needs fixing (because something hasn't been properly re-initialized.)
The first bug I've spotted in that area would be VFP - which is not
hotplug aware.  VFP needs to register into the hotplug notifier system
so that vfp_enable() can be called.  I'll have a patch for that before
this weekend is out.

> >> void platform_cpu_die(unsigned int cpu)
> >> {
> >> #ifdef DEBUG
> >> ? ? ? unsigned int this_cpu = hard_smp_processor_id();
> >>
> >> ? ? ? if (cpu != this_cpu) {
> >> ? ? ? ? ? ? ? printk(KERN_CRIT "Eek! platform_cpu_die running on %u, should be %u\n",
> >> ? ? ? ? ? ? ? ? ? ? ? ? ?this_cpu, cpu);
> >> ? ? ? ? ? ? ? BUG();
> >> ? ? ? }
> >> #endif
> >>
> >> ? ? ? gic_cpu_exit(0);
> >> ? ? ? barrier();
> >> ? ? ? complete(&per_cpu(cpu_killed, cpu));
> >> ? ? ? flush_cache_all();
> >> ? ? ? barrier();
> >> ? ? ? __cortex_a9_save(0);
> >>
> >> ? ? ? /* return happens from __cortex_a9_restore */
> >> ? ? ? barrier();
> >> ? ? ? writel(smp_processor_id(), EVP_CPU_RESET_VECTOR);
> >
> > And here, if the CPU is not _immediately_ stopped, it will return and
> > restart the bring-up. ?If you are using hardware to reset the CPU (which
> > it seems you are), you should ensure that this function never returns.
> > You may also wish to add a pr_crit() here to indicate when this failure
> > happens.
> >
> 
> It is immediately stopped. Turned off really. But this is where it
> will resume on when it gets brought back up (__cortex_a9_restore takes
> care of that)

As I explained above, writel() is not guaranteed to take immediate effect.
Writes to device memory can be buffered, which allows the CPU to continue
execution having posted the write.  At some point later, that write gets
committed to hardware.

For instance, this code is buggy:

	writel(0x00, base);
	udelay(10);
	writel(0xff, base);

It may or may not write these two values 10us apart.  As far as the
external hardware can see, the writes could be:

	udelay(10);
	writel(0x00, base);
	writel(0xff, base);

or:

	writel(0x00, base);
	writel(0xff, base);
	udelay(10);

or even:

	writel(0x00, base);
	udelay(1000);
	writel(0xff, base);

There are ways to ensure that writes take effect before the program
continues, and one of the most reliable is to read back the register
you've just written (which is the preferred method when writing PCI
device drivers - because there could be many levels of bridges with
write posting buffers between the CPU and the device.)

As far as ordering:

	writel(0x00, base);
	*(int *)memory = value;

there is no guarantee that the writel() will be seen by hardware before
the memory location is written and that write is visible to hardware.
The two operations could occur in the reverse order.  This is because
accesses to device memory can pass memory accesses.  This iss why
barriers are necessary - and there's a big document to describe barriers
in the kernel Documentation subdirectory.

[RFC] Make SMP secondary CPU up more resilient to failure.

[Russell King - ARM Linux]

On Sat, Dec 18, 2010 at 12:08:28AM +0000, Russell King - ARM Linux wrote:
> The first bug I've spotted in that area would be VFP - which is not
> hotplug aware.  VFP needs to register into the hotplug notifier system
> so that vfp_enable() can be called.  I'll have a patch for that before
> this weekend is out.

This patch ensures that VFP is correctly re-initialized when the CPU
restarts.

 arch/arm/vfp/vfpmodule.c |   21 ++++++++++++++++++++-
 1 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
index 8063a32..b865a53 100644
--- a/arch/arm/vfp/vfpmodule.c
+++ b/arch/arm/vfp/vfpmodule.c
@@ -10,9 +10,12 @@
  */
 #include <linux/module.h>
 #include <linux/types.h>
+#include <linux/cpu.h>
 #include <linux/kernel.h>
+#include <linux/notifier.h>
 #include <linux/signal.h>
 #include <linux/sched.h>
+#include <linux/smp.h>
 #include <linux/init.h>
 
 #include <asm/cputype.h>
@@ -484,7 +487,19 @@ void vfp_flush_hwstate(struct thread_info *thread)
 	put_cpu();
 }
 
-#include <linux/smp.h>
+#ifdef CONFIG_HOTPLUG_CPU
+static int vfp_hotplug_notifier(struct notifier_block *b, unsigned long action,
+				void *data)
+{
+	if (action == CPU_STARTING)
+		vfp_enable(NULL);
+	return NOTIFY_OK;
+}
+
+static struct notifier_block vfp_hotplug_nb = {
+	.notifier_call = vfp_hotplug_notifier,
+};
+#endif
 
 /*
  * VFP support code initialisation.
@@ -514,6 +529,10 @@ static int __init vfp_init(void)
 	else if (vfpsid & FPSID_NODOUBLE) {
 		printk("no double precision support\n");
 	} else {
+#ifdef CONFIG_HOTPLUG_CPU
+		register_cpu_notifier(&vfp_hotplug_nb);
+#endif
+
 		smp_call_function(vfp_enable, NULL, 1);
 
 		VFP_arch = (vfpsid & FPSID_ARCH_MASK) >> FPSID_ARCH_BIT;  /* Extract the architecture version */

[RFC] Make SMP secondary CPU up more resilient to failure.

[Andrei Warkentin]

On Fri, Dec 17, 2010 at 6:08 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Dec 17, 2010 at 05:45:09PM -0600, Andrei Warkentin wrote:
>> On Fri, Dec 17, 2010 at 5:14 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > On Fri, Dec 17, 2010 at 02:52:29PM -0600, Andrei Warkentin wrote:
>> >> On Thu, Dec 16, 2010 at 5:28 PM, Russell King - ARM Linux
>> >> <linux@arm.linux.org.uk> wrote:
>> >> > On Thu, Dec 16, 2010 at 05:09:48PM -0600, Andrei Warkentin wrote:
>> >> >> On Thu, Dec 16, 2010 at 5:34 AM, Russell King - ARM Linux
>> >> >> <linux@arm.linux.org.uk> wrote:
>> >> >> >
>> >> >> > On Wed, Dec 15, 2010 at 05:45:13PM -0600, Andrei Warkentin wrote:
>> >> >> > > This is my first time on linux-arm-kernel, and while I've read the
>> >> >> > > FAQ, hopefully I don't screw up too badly :).
>> >> >> > >
>> >> >> > > Anyway, we're on a dual-core ARMv7 running 2.6.36, and during
>> >> >> > > stability stress testing saw the following:
>> >> >> > > 1) After a number hotplug iterations, CPU1 fails to set its online bit
>> >> >> > > quickly enough and __cpu_up() times-out.
>> >> >> > > 2) CPU1 eventually completes its startup and sets the bit, however,
>> >> >> > > since _cpu_up() failed, CPU1's active bit is never set.
>> >> >> >
>> >> >> > Why is your CPU taking soo long to come up? ?We wait one second in the
>> >> >> > generic code, which is the time taken from the platform code being happy
>> >> >> > that it has successfully started the CPU. ?Normally, platforms wait an
>> >> >> > additional second to detect the CPU entering the kernel.
>> >> >>
>> >> >> It seems twd_calibrate_rate is the culprit (although in our case,
>> >> >> since the clock is the same to both CPUs, there is no point in
>> >> >> calibrating).
>> >> >
>> >> > twd_calibrate_rate() should only run once at boot. ?Once it's run,
>> >> > taking CPUs offline and back online should not cause the rate to be
>> >> > recalibrated.
>> >>
>> >> Let's me just see if I understand things correctly for the hotplug case.
>> >>
>> >> 1) cpu_down calls take_down_cpu
>> >> 2) Idle thread on secondary notices cpu_is_offline, and calls cpu_die()
>> >> 3) cpu_die calls platform_cpu_die, at which point the cpu is dead.
>> >
>> > Correct so far.
>> >
>> >> If it ever wakes up (because of a cpu_up), it will continue to run in
>> >> cpu_die.
>> >
>> > That depends what you have in your cpu_die(). ?If you return from
>> > cpu_die() then we assume that is because you have been woken up, and
>> > so we re-initialize the CPU.
>> >
>>
>> That's correct. Same applies to us. When we wake up a CPU, it is going
>> to return back to cpu_die, and then call secondary startup to join the
>> rest.
>
> However, your writel() is not guaranteed to take effect at the point in
> time that you issue it. ?You're programming a weakly ordered architecture,
> you need to be aware that things don't happen at the point in time that
> they appear in the program, or sometimes even in the order that you put
> them in the program. ?(See below.)
>
>> > That's because existing hotplug implementations don't have the necessary
>> > support hardware to reset just one CPU, and the only way to bring a CPU
>> > back online is to jump back to the secondary startup.
>> >
>> >> 4) cpu_die jump to secondary_start_kernel.
>> >
>> > This is for dumb implementations. ?If your code takes the CPU offline via
>> > hardware means, then you must _never_ return from cpu_die(), even if the
>> > hardware fails to take the CPU offline. ?I suspect this is where your
>> > problem lies.
>> >
>>
>> No, we don't have a problem shutting cores down. We gate the clock and
>> set the reset line, but when it comes back up all state will be
>> restored as it were prior to the getting turned off, so it can be
>> resumed back properly.
>>
>>
>> >> 5) secondary_start_kernel calls percpu_timer_setup
>> >> 6) ?percpu_timer_setup calls platform local_timer_setup
>> >> 7) local_timer_setup calls twd_timer_setup_scalable
>> >>
>> >> ...which calls __twd_timer_setup, which does twd_calibrate_rate among
>> >> other things.
>> >
>> > Again, correct, and this is where you claimed that excessive time was
>> > being spent. ?Looking at twd_calibrate_rate(), it has this:
>> >
>> > ? ? ? ?if (twd_timer_rate == 0) {
>> > ? ? ? ? ? ? ? ?printk(KERN_INFO "Calibrating local timer... ");
>> > ...
>> > ? ? ? ? ? ? ? ?twd_timer_rate = (0xFFFFFFFFU - count) * (HZ / 5);
>> >
>> > ? ? ? ? ? ? ? ?printk("%lu.%02luMHz.\n", twd_timer_rate / 1000000,
>> > ? ? ? ? ? ? ? ? ? ? ? ?(twd_timer_rate / 100000) % 100);
>> > ? ? ? ?}
>> >
>> > which, on the first and _only_ first pass through, initializes
>> > twd_timer_rate. ?Because on hotplug cpu-up the timer rate has already
>> > been calibrated, we won't re-run the calibration, and we won't spend
>> > lots of time here.
>> >
>>
>> Sorry, I feel like a moron. Didn't realize that in our codebase there
>> is a change explicitly always doing the calibration.
>> I'll see if this is necessary.
>>
>> This still doesn't explain why the ARM SMP code shouldn't be more
>> resilient (i.e. secondary startup doesn't do anything unless __cpu_up
>> wants it to)
>>
>> >> #ifdef CONFIG_HOTPLUG_CPU
>> >> static DEFINE_PER_CPU(struct completion, cpu_killed);
>> >> extern void tegra_hotplug_startup(void);
>> >> #endif
>> >
>> > You do not need cpu_killed to be a per-cpu thing. ?The locking in
>> > cpu_up() and cpu_down() ensures that you can not take more than one
>> > CPU up or down concurrently. ?See cpu_maps_update_begin() which
>> > acquires the lock, and cpu_maps_update_done() which drops the lock.
>> >
>> >> static DECLARE_BITMAP(cpu_init_bits, CONFIG_NR_CPUS) __read_mostly;
>> >> const struct cpumask *const cpu_init_mask = to_cpumask(cpu_init_bits);
>> >> #define cpu_init_map (*(cpumask_t *)cpu_init_mask)
>> >>
>> >> #define EVP_CPU_RESET_VECTOR \
>> >> ? ? ? (IO_ADDRESS(TEGRA_EXCEPTION_VECTORS_BASE) + 0x100)
>> >> #define CLK_RST_CONTROLLER_CLK_CPU_CMPLX \
>> >> ? ? ? (IO_ADDRESS(TEGRA_CLK_RESET_BASE) + 0x4c)
>> >> #define CLK_RST_CONTROLLER_RST_CPU_CMPLX_SET \
>> >> ? ? ? (IO_ADDRESS(TEGRA_CLK_RESET_BASE) + 0x340)
>> >> #define CLK_RST_CONTROLLER_RST_CPU_CMPLX_CLR \
>> >> ? ? ? (IO_ADDRESS(TEGRA_CLK_RESET_BASE) + 0x344)
>> >>
>> >> void __cpuinit platform_secondary_init(unsigned int cpu)
>> >> {
>> >> ? ? ? trace_hardirqs_off();
>> >> ? ? ? gic_cpu_init(0, IO_ADDRESS(TEGRA_ARM_PERIF_BASE) + 0x100);
>> >> ? ? ? /*
>> >> ? ? ? ?* Synchronise with the boot thread.
>> >> ? ? ? ?*/
>> >> ? ? ? spin_lock(&boot_lock);
>> >> #ifdef CONFIG_HOTPLUG_CPU
>> >> ? ? ? cpu_set(cpu, cpu_init_map);
>> >> ? ? ? INIT_COMPLETION(per_cpu(cpu_killed, cpu));
>> >
>> > This means you don't need to re-initialize the completion. ?Just define
>> > it once using DEFINE_COMPLETION(). ?Note that this is something that will
>> > be taken care of in core SMP hotplug code during the next merge window.
>> >
>>
>> Ah ok, thanks!
>>
>> >> #endif
>> >> ? ? ? spin_unlock(&boot_lock);
>> >> }
>> >>
>> >> int __cpuinit boot_secondary(unsigned int cpu, struct task_struct *idle)
>> >> {
>> >> ? ? ? unsigned long old_boot_vector;
>> >> ? ? ? unsigned long boot_vector;
>> >> ? ? ? unsigned long timeout;
>> >> ? ? ? u32 reg;
>> >>
>> >> ? ? ? /*
>> >> ? ? ? ?* set synchronisation state between this boot processor
>> >> ? ? ? ?* and the secondary one
>> >> ? ? ? ?*/
>> >> ? ? ? spin_lock(&boot_lock);
>> >>
>> >> ? ? ? /* set the reset vector to point to the secondary_startup routine */
>> >> #ifdef CONFIG_HOTPLUG_CPU
>> >> ? ? ? if (cpumask_test_cpu(cpu, cpu_init_mask))
>> >> ? ? ? ? ? ? ? boot_vector = virt_to_phys(tegra_hotplug_startup);
>> >> ? ? ? else
>> >> #endif
>> >> ? ? ? ? ? ? ? boot_vector = virt_to_phys(tegra_secondary_startup);
>> >
>> > I didn't see the code for these startup functions.
>> >
>>
>> ENTRY(tegra_secondary_startup)
>> ? ? ? ? msr ? ? cpsr_fsxc, #0xd3
>> ? ? ? ? bl ? ? ?__invalidate_cpu_state
>> ? ? ? ? cpu_id ?r0
>> ? ? ? ? enable_coresite r1
>> ? ? ? ? poke_ev r0, r1 ? ? ? ? ? ? ? ? ? ? ? ?<--------- updates reset
>> vector with up'ped CPU
>> ? ? ? ? b ? ? ? secondary_startup
>>
>> ENTRY(tegra_hotplug_startup)
>> ? ? ? ? setmode PSR_F_BIT | PSR_I_BIT | SVC_MODE, r9
>> ? ? ? ? bl ? ? ?__invalidate_cpu_state
>> ? ? ? ? enable_coresite r1
>>
>> ? ? ? ? /* most of the below is a retread of what happens in __v7_setup and
>> ? ? ? ? ?* secondary_startup, to get the MMU re-enabled and to branch
>> ? ? ? ? ?* to secondary_kernel_startup */
>> ? ? ? ? mrc ? ? p15, 0, r0, c1, c0, 1
>> ? ? ? ? orr ? ? r0, r0, #(1 << 6) | (1 << 0) ? ?@ re-enable coherency
>> ? ? ? ? mcr ? ? p15, 0, r0, c1, c0, 1
>>
>> ? ? ? ? adr ? ? r4, __tegra_hotplug_data
>> ? ? ? ? ldmia ? r4, {r5, r7, r12}
>> ? ? ? ? mov ? ? r1, r12 ? ? ? ? ? ? ? ? @ ctx_restore = __cortex_a9_restore
>> ? ? ? ? sub ? ? r4, r4, r5
>> ? ? ? ? ldr ? ? r0, [r7, r4] ? ? ? ? ? ?@ pgdir = secondary_data.pgdir
>> ? ? ? ? b ? ? ? __return_to_virtual
>> ENDPROC(tegra_hotplug_startup)
>>
>> Effectively this restores what __cortex_a9_save did before it shut down the CPU.
>
> Err. ?You _really_ do not need all this complexity. ?My guess is that
> you've looked at what Realview is doing, and are trying to copy the
> way it does stuff. ?That's really over the top if you have the ability
> to place CPU cores back into reset, which it seems you do.
>
> All you need to do when killing a CPU is to put it back into reset.
> When you bring the CPU back up, just the same steps you'd run on the
> initial CPU bring-up. ?In other words, point the reset vector at
> tegra_secondary_startup and let it run through the same startup again.
>
> There's (should be) no need to preserve any state for the CPU across
> a hot-unplug. ?If there is state to be preserved, that's a bug which
> needs fixing (because something hasn't been properly re-initialized.)
> The first bug I've spotted in that area would be VFP - which is not
> hotplug aware. ?VFP needs to register into the hotplug notifier system
> so that vfp_enable() can be called. ?I'll have a patch for that before
> this weekend is out.

I fully agree with you. There should be no need for that. it looked
strange to me as well.
The worst part, is that tegra isn't the only cortex a9 based machine
arch, and all this code
is going to do is get duplicated all over the place, unnecessarily.
Thank you for the patch,
I think re-enabling debug monitor (if necessary) might be another
thing if it's not done already.

I do want to bring to your attention something I found while making
the patch I mailed here. It's that
secondary_startup (along with MMU-enabling friends) are located in the
.init.text section and get nuked after
startup. Which means that the secondary startup right now can't be
done without moving this code out of .init.text.
Moving that stuff is kind of iffy too, because it's used immediately
in head.S. I didn't try very hard :) (and it didn't work, but that's
my fault). Now I have a good reason to try again :).

>
>> >> void platform_cpu_die(unsigned int cpu)
>> >> {
>> >> #ifdef DEBUG
>> >> ? ? ? unsigned int this_cpu = hard_smp_processor_id();
>> >>
>> >> ? ? ? if (cpu != this_cpu) {
>> >> ? ? ? ? ? ? ? printk(KERN_CRIT "Eek! platform_cpu_die running on %u, should be %u\n",
>> >> ? ? ? ? ? ? ? ? ? ? ? ? ?this_cpu, cpu);
>> >> ? ? ? ? ? ? ? BUG();
>> >> ? ? ? }
>> >> #endif
>> >>
>> >> ? ? ? gic_cpu_exit(0);
>> >> ? ? ? barrier();
>> >> ? ? ? complete(&per_cpu(cpu_killed, cpu));
>> >> ? ? ? flush_cache_all();
>> >> ? ? ? barrier();
>> >> ? ? ? __cortex_a9_save(0);
>> >>
>> >> ? ? ? /* return happens from __cortex_a9_restore */
>> >> ? ? ? barrier();
>> >> ? ? ? writel(smp_processor_id(), EVP_CPU_RESET_VECTOR);
>> >
>> > And here, if the CPU is not _immediately_ stopped, it will return and
>> > restart the bring-up. ?If you are using hardware to reset the CPU (which
>> > it seems you are), you should ensure that this function never returns.
>> > You may also wish to add a pr_crit() here to indicate when this failure
>> > happens.
>> >
>>
>> It is immediately stopped. Turned off really. But this is where it
>> will resume on when it gets brought back up (__cortex_a9_restore takes
>> care of that)
>

But this is a writel to memory. And CONFIG_ARM_DMA_MEM_BUFFERABLE is defined, so
there is a __wmb() after the write, and in our case
#define wmb()           do { dsb(); outer_sync(); } while (0)

The dsb will flush the write buffer, unless I misunderstand.

> As I explained above, writel() is not guaranteed to take immediate effect.
> Writes to device memory can be buffered, which allows the CPU to continue
> execution having posted the write. ?At some point later, that write gets
> committed to hardware.
>
> For instance, this code is buggy:
>
> ? ? ? ?writel(0x00, base);
> ? ? ? ?udelay(10);
> ? ? ? ?writel(0xff, base);
>
> It may or may not write these two values 10us apart. ?As far as the
> external hardware can see, the writes could be:
>
> ? ? ? ?udelay(10);
> ? ? ? ?writel(0x00, base);
> ? ? ? ?writel(0xff, base);
>
> or:
>
> ? ? ? ?writel(0x00, base);
> ? ? ? ?writel(0xff, base);
> ? ? ? ?udelay(10);
>
> or even:
>
> ? ? ? ?writel(0x00, base);
> ? ? ? ?udelay(1000);
> ? ? ? ?writel(0xff, base);
>
> There are ways to ensure that writes take effect before the program
> continues, and one of the most reliable is to read back the register
> you've just written (which is the preferred method when writing PCI
> device drivers - because there could be many levels of bridges with
> write posting buffers between the CPU and the device.)
>
> As far as ordering:
>
> ? ? ? ?writel(0x00, base);
> ? ? ? ?*(int *)memory = value;
>
> there is no guarantee that the writel() will be seen by hardware before
> the memory location is written and that write is visible to hardware.
> The two operations could occur in the reverse order. ?This is because
> accesses to device memory can pass memory accesses. ?This iss why
> barriers are necessary - and there's a big document to describe barriers
> in the kernel Documentation subdirectory.
>

Keep in mind that this is not why we were seeing that failure. If it
failed here, secondary_boot spinning on this value wouldn't have
succeeded, and it wouldn't even try spinning with a timeout on the
cpu_online bit. And while I agree with you that the hotplug code code
be severely less byzantine (and help everybody all the other mach SMP
efforts in a generic fashion), it's not the problem either - it works
- boot_secondary returns success and cpu_up enters the timeout wait
for the online bit.

So what about the original issue at hand? Can we make the logic in
__cpu_up basically either bring the CPU up successfully or ensure it's
not going to come up behind the scenes? Because the current logic
allows for that.

Right now, in __cpu_up, if the online bit doesn't come up in some time
frame, there is nothing there that guarantees that the secondary
doesn't just come up late (and ruin things because it's an unexpected
guest at the party). This is why I sent the original patch.

I debugged through what was happening, and boot_secondary was
succeeding (this means that the secondary IS now about to jump to
secondary_start_kernel). But the wait to set the online bit timed out.
Not by much (judging from kernel messages), but timed out. Now, this
was on a IO/memory/compute overloaded system, this isn't supposed to
happen in real life, but it happened, and it's not helping our stress
tests ;-). Basically there is no safe guards to prevent something like
this from happening, hence the patch.

The patch waits to make sure secondary_start_kernel is entered, but
before all the non-easily revertable stuff starts happening. If that
wait succeeds nothing else should go wrong, and we do a non-timeout
wait for the online bit.

The second bit is paranoid, and extremely unlikely, but is there for
completeness sakes. I have never seen it in our mach. If it enters
too late (impossible really, barring terrible hw bugs) into
secondary_start_kernel, it won't continue with the init, because
__cpu_up timed out and didn't release the lock necessary to continue
secondary_start_kernel.

Thanks again,
A

[RFC] Make SMP secondary CPU up more resilient to failure.

[Russell King - ARM Linux]

On Thu, Dec 16, 2010 at 05:09:48PM -0600, Andrei Warkentin wrote:
> It seems twd_calibrate_rate is the culprit (although in our case,
> since the clock is the same to both CPUs, there is no point in
> calibrating).
> We've seen this only when the device was under stress test load.

Right, I can reproduce it here on Versatile Express.  With some debugging
printk's added, what's going on is revealed.  This is what a normal cpu
up looks like:

Booting CPU3
Writng pen_release: 4294967295 -> 3
CPU3: Booted secondary processor
CPU3: Unknown IPI message 0x1
CPU3: calibrating delay
Switched to NOHz mode on CPU #3
CPU3: calibrating done
CPU3: now online
CPU3: online mask = 0000000f

However, when things go bad:

CPU3: Booted secondary processor
CPU3: calibrating delay
Booting CPU3
Switched to NOHz mode on CPU #3
Writng pen_release: 4294967295 -> 3
CPU3: Unknown IPI message 0x1
CPU3: calibrating done
CPU3: now online
CPU3: online mask = 0000000f
CPU3: processor failed to boot: -38
online mask = 0000000f

Notice that CPU3 booted before the requesting processor requested it to
boot - pen_release was -1 when CPU3 exited its hotplug lowpower function.
However, for CPU3 to get out of that, it must have seen pen_release = 3.

What I think is going on here is that the write to set pen_release back
to -1 is being cached.  When CPU3 dies, although we call flush_cache_all(),
this doesn't touch the L2x0 controller, so the update never makes it to
memory.  Then CPU3 disables its caches, and its access to pen_release
bypasses both all caches, resulting in the value in physical memory being
seen - which was the value written during the previous plug/unplug
iteration.

Reading the pen_release value in various places in CPU3's startup path
(which changes the MOESI cache conditions) prevents this 'speculative
starting' effect, as does flushing the pen_release cache line back to
memory after we set pen_release to -1 in platform_secondary_startup.

So, I can fix the underlying bug causing early CPU start in the existing
CPU hotplug implementations by ensuring that pen_release is always visibly
written.

We don't add code to patch around behaviour we don't immediately
understand - we try to understand what is going on, and fix the real
underlying problem.

So, the question now is: where is your underlying bug.  There's certainly
a few holes in your code which I've pointed out - I suggest fixing those
and re-testing, and if the problem persists, try looking at the order in
which the kernel messages appear to get a better clue what's going on.

[RFC] Make SMP secondary CPU up more resilient to failure.

[Andrei Warkentin]

On Sat, Dec 18, 2010 at 3:58 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Dec 16, 2010 at 05:09:48PM -0600, Andrei Warkentin wrote:
>> It seems twd_calibrate_rate is the culprit (although in our case,
>> since the clock is the same to both CPUs, there is no point in
>> calibrating).
>> We've seen this only when the device was under stress test load.
>
> Right, I can reproduce it here on Versatile Express. ?With some debugging
> printk's added, what's going on is revealed. ?This is what a normal cpu
> up looks like:
>
> Booting CPU3
> Writng pen_release: 4294967295 -> 3
> CPU3: Booted secondary processor
> CPU3: Unknown IPI message 0x1
> CPU3: calibrating delay
> Switched to NOHz mode on CPU #3
> CPU3: calibrating done
> CPU3: now online
> CPU3: online mask = 0000000f
>
> However, when things go bad:
>
> CPU3: Booted secondary processor
> CPU3: calibrating delay
> Booting CPU3
> Switched to NOHz mode on CPU #3
> Writng pen_release: 4294967295 -> 3
> CPU3: Unknown IPI message 0x1
> CPU3: calibrating done
> CPU3: now online
> CPU3: online mask = 0000000f
> CPU3: processor failed to boot: -38
> online mask = 0000000f
>
> Notice that CPU3 booted before the requesting processor requested it to
> boot - pen_release was -1 when CPU3 exited its hotplug lowpower function.
> However, for CPU3 to get out of that, it must have seen pen_release = 3.
>
> What I think is going on here is that the write to set pen_release back
> to -1 is being cached. ?When CPU3 dies, although we call flush_cache_all(),
> this doesn't touch the L2x0 controller, so the update never makes it to
> memory. ?Then CPU3 disables its caches, and its access to pen_release
> bypasses both all caches, resulting in the value in physical memory being
> seen - which was the value written during the previous plug/unplug
> iteration.

This is an interesting bug, but it is a different bug. We don't have a
holding pen, the CPUs
are very much dead, and there is no early start going on - we have to
very much twiddle some HW
regs to ungate the clock and lower the reset line on the Tegra to get
the secondary started.

>
> Reading the pen_release value in various places in CPU3's startup path
> (which changes the MOESI cache conditions) prevents this 'speculative
> starting' effect, as does flushing the pen_release cache line back to
> memory after we set pen_release to -1 in platform_secondary_startup.
>
> So, I can fix the underlying bug causing early CPU start in the existing
> CPU hotplug implementations by ensuring that pen_release is always visibly
> written.
>

This should fix hot plug implementations which rely on a holding pen +
WFI while they are "dead", but
will not resolve the issue we saw.

> We don't add code to patch around behaviour we don't immediately
> understand - we try to understand what is going on, and fix the real
> underlying problem.
>

I agree. The patch does give you a safety net. For example, for this
particular bug you have just described, you would have
seen a message reporting the early start, but the offending CPU would
have never gotten past the arch_spin_lock, so it couldn't
touch kernel structures unexpectedly.

> So, the question now is: where is your underlying bug. ?There's certainly
> a few holes in your code which I've pointed out - I suggest fixing those
> and re-testing, and if the problem persists, try looking at the order in
> which the kernel messages appear to get a better clue what's going on.
>

That's a good question. I think the best answer I can come up right
now is to find another dual-core ARM platform and run our stress tests
to get it to fail in the same fashion. The issue you pointed out -
namely, the byzantine design of the hotplug code, does not affect the
up path. Proof: boot_secondary always succeeds. I am not convinced
with your writel argument either. Proof: boot_secondary always
succeeded, plus the writel expands to a write followed by a dsb and
outer cache sync. And the writel location looked polled at
boot_secondary is in memory. Doesn't the dsb drain the write buffer?
In all the failure cases the secondary was inside kernel code by the
time __cpu_up started waiting on the online bit to appear. That means
that whatever was taking it's sweet time - it was taking it from the
moment secondary_start_kernel entered to the moment the online bit was
set. I have printk logs showing that.

So the question we want to collectively answer is really - should the
stuffing in secondary_start_kernel really be taking so long, assuming
an unrealistic load? I don't have a good answer for that, yet. I'm
going to get some timings for that code.

Btw, I would guess that one of the reasons the hotplug logic is done
the way it was is because secondary_startup is garbage after init,
having been freed along with the rest of init memory. Is anybody
addressing that atm?

[RFC] Make SMP secondary CPU up more resilient to failure.

[Russell King - ARM Linux]

On Sat, Dec 18, 2010 at 01:17:34AM -0600, Andrei Warkentin wrote:
> I do want to bring to your attention something I found while making
> the patch I mailed here. It's that
> secondary_startup (along with MMU-enabling friends) are located in the
> .init.text section and get nuked after
> startup. Which means that the secondary startup right now can't be
> done without moving this code out of .init.text.
> Moving that stuff is kind of iffy too, because it's used immediately
> in head.S. I didn't try very hard :) (and it didn't work, but that's
> my fault). Now I have a good reason to try again :).

I have a series of 10 patches which does that, posted previously to this
mailing list on 4th October which addresses that.

There's another series of 22 patches which clean up the SMP support which
you also probably aren't aware of (Daniel Walker is though.)  That now
has the VFP and pen_release fixes I just posted added to it, and it's
also going to make the failure-to-online message a little more verbose
as to why the failure happened.

> >> >> ? ? ? /* return happens from __cortex_a9_restore */
> >> >> ? ? ? barrier();
> >> >> ? ? ? writel(smp_processor_id(), EVP_CPU_RESET_VECTOR);
> >> >
> >> > And here, if the CPU is not _immediately_ stopped, it will return and
> >> > restart the bring-up. ?If you are using hardware to reset the CPU (which
> >> > it seems you are), you should ensure that this function never returns.
> >> > You may also wish to add a pr_crit() here to indicate when this failure
> >> > happens.
> >> >
> >>
> >> It is immediately stopped. Turned off really. But this is where it
> >> will resume on when it gets brought back up (__cortex_a9_restore takes
> >> care of that)
> >
> 
> But this is a writel to memory. And CONFIG_ARM_DMA_MEM_BUFFERABLE is
> defined, so there is a __wmb() after the write, and in our case
> #define wmb()           do { dsb(); outer_sync(); } while (0)
> 
> The dsb will flush the write buffer, unless I misunderstand.

writel() is defined as follows:

#define writel(v,c)             ({ __iowmb(); writel_relaxed(v,c); })

Notice that the wmb barrier is performed before the actual write to device,
not after it.  That is to ensure that if this device write is telling
something to start DMA, any previous writes to DMA coherent memory (eg,
for ring descriptors) will be visible to the DMA agent.

There are no ordering guarantees between the device write vs accesses
after the writel().

> Keep in mind that this is not why we were seeing that failure. If it
> failed here, secondary_boot spinning on this value wouldn't have
> succeeded, and it wouldn't even try spinning with a timeout on the
> cpu_online bit. And while I agree with you that the hotplug code code
> be severely less byzantine (and help everybody all the other mach SMP
> efforts in a generic fashion), it's not the problem either - it works
> - boot_secondary returns success and cpu_up enters the timeout wait
> for the online bit.

Well, the code has issues as it currently stands, and I'd like to see
some of those issues resolved first, and then re-checking where we stand
with this.

> I debugged through what was happening, and boot_secondary was
> succeeding (this means that the secondary IS now about to jump to
> secondary_start_kernel).

I assume you are aware that your boot_secondary() _always_ succeeds, even
if this times out?

        timeout = jiffies + HZ;
        while (time_before(jiffies, timeout)) {
                if (readl(EVP_CPU_RESET_VECTOR) != boot_vector)
                        break;
                udelay(10);
        }
...
        spin_unlock(&boot_lock);

        return 0;

It would be good if you fixed that and re-tested - what could be
happening is exactly the same thing you're accusing the generic code
of doing:

	CPU0			CPU1
	writes RESET_VECTOR
	prods CPU
	starts wait
				cpu starts slowly
	times out
	restores RESET_VECTOR
				writes RESET_VECTOR
	returns success
	starts wait for CPU online
				...
	times out
	returns -EIO
				cpu comes online

You'll never know if your reset vector has been corrupted in this way
because you always return success.

> But the wait to set the online bit timed out.
> Not by much (judging from kernel messages), but timed out. Now, this
> was on a IO/memory/compute overloaded system, this isn't supposed to
> happen in real life, but it happened, and it's not helping our stress
> tests ;-). Basically there is no safe guards to prevent something like
> this from happening, hence the patch.

We assume in __cpu_up() is that if boot_secondary() returns success, the
CPU is well on its way to coming online - iow, platform_secondary_init()
has been reached, and we have relatively little stuff to do before we see
the CPU coming online.

I can't see that your code allows that assumption to be made, as
boot_secondary() always returns success.

Maybe you need your wait-for-reset-vector to be longer, and on failure
place the CPU back into reset to ensure that it doesn't corrupt the
reset vector if it starts late?

[RFC] Make SMP secondary CPU up more resilient to failure.

[Andrei Warkentin]

On Sat, Dec 18, 2010 at 6:01 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sat, Dec 18, 2010 at 01:17:34AM -0600, Andrei Warkentin wrote:
>> I do want to bring to your attention something I found while making
>> the patch I mailed here. It's that
>> secondary_startup (along with MMU-enabling friends) are located in the
>> .init.text section and get nuked after
>> startup. Which means that the secondary startup right now can't be
>> done without moving this code out of .init.text.
>> Moving that stuff is kind of iffy too, because it's used immediately
>> in head.S. I didn't try very hard :) (and it didn't work, but that's
>> my fault). Now I have a good reason to try again :).
>
> I have a series of 10 patches which does that, posted previously to this
> mailing list on 4th October which addresses that.
>
> There's another series of 22 patches which clean up the SMP support which
> you also probably aren't aware of (Daniel Walker is though.) ?That now
> has the VFP and pen_release fixes I just posted added to it, and it's
> also going to make the failure-to-online message a little more verbose
> as to why the failure happened.
>
>> >> >> ? ? ? /* return happens from __cortex_a9_restore */
>> >> >> ? ? ? barrier();
>> >> >> ? ? ? writel(smp_processor_id(), EVP_CPU_RESET_VECTOR);
>> >> >
>> >> > And here, if the CPU is not _immediately_ stopped, it will return and
>> >> > restart the bring-up. ?If you are using hardware to reset the CPU (which
>> >> > it seems you are), you should ensure that this function never returns.
>> >> > You may also wish to add a pr_crit() here to indicate when this failure
>> >> > happens.
>> >> >
>> >>
>> >> It is immediately stopped. Turned off really. But this is where it
>> >> will resume on when it gets brought back up (__cortex_a9_restore takes
>> >> care of that)
>> >
>>
>> But this is a writel to memory. And CONFIG_ARM_DMA_MEM_BUFFERABLE is
>> defined, so there is a __wmb() after the write, and in our case
>> #define wmb() ? ? ? ? ? do { dsb(); outer_sync(); } while (0)
>>
>> The dsb will flush the write buffer, unless I misunderstand.
>
> writel() is defined as follows:
>
> #define writel(v,c) ? ? ? ? ? ? ({ __iowmb(); writel_relaxed(v,c); })
>
> Notice that the wmb barrier is performed before the actual write to device,
> not after it. ?That is to ensure that if this device write is telling
> something to start DMA, any previous writes to DMA coherent memory (eg,
> for ring descriptors) will be visible to the DMA agent.
>

...and in that last case there are no writels following the write of
SMP id to reset vector. Oops :/.

> There are no ordering guarantees between the device write vs accesses
> after the writel().
>
>> Keep in mind that this is not why we were seeing that failure. If it
>> failed here, secondary_boot spinning on this value wouldn't have
>> succeeded, and it wouldn't even try spinning with a timeout on the
>> cpu_online bit. And while I agree with you that the hotplug code code
>> be severely less byzantine (and help everybody all the other mach SMP
>> efforts in a generic fashion), it's not the problem either - it works
>> - boot_secondary returns success and cpu_up enters the timeout wait
>> for the online bit.
>
> Well, the code has issues as it currently stands, and I'd like to see
> some of those issues resolved first, and then re-checking where we stand
> with this.
>
>> I debugged through what was happening, and boot_secondary was
>> succeeding (this means that the secondary IS now about to jump to
>> secondary_start_kernel).
>
> I assume you are aware that your boot_secondary() _always_ succeeds, even
> if this times out?
>
> ? ? ? ?timeout = jiffies + HZ;
> ? ? ? ?while (time_before(jiffies, timeout)) {
> ? ? ? ? ? ? ? ?if (readl(EVP_CPU_RESET_VECTOR) != boot_vector)
> ? ? ? ? ? ? ? ? ? ? ? ?break;
> ? ? ? ? ? ? ? ?udelay(10);
> ? ? ? ?}
> ...
> ? ? ? ?spin_unlock(&boot_lock);
>
> ? ? ? ?return 0;
>
> It would be good if you fixed that and re-tested - what could be
> happening is exactly the same thing you're accusing the generic code
> of doing:
>
> ? ? ? ?CPU0 ? ? ? ? ? ? ? ? ? ?CPU1
> ? ? ? ?writes RESET_VECTOR
> ? ? ? ?prods CPU
> ? ? ? ?starts wait
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?cpu starts slowly
> ? ? ? ?times out
> ? ? ? ?restores RESET_VECTOR
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?writes RESET_VECTOR
> ? ? ? ?returns success
> ? ? ? ?starts wait for CPU online
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?...
> ? ? ? ?times out
> ? ? ? ?returns -EIO
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?cpu comes online
>
> You'll never know if your reset vector has been corrupted in this way
> because you always return success.

Gah, that's terrible...how could I have missed this... I'll go put the
foot back in my mouth :).

>
>> But the wait to set the online bit timed out.
>> Not by much (judging from kernel messages), but timed out. Now, this
>> was on a IO/memory/compute overloaded system, this isn't supposed to
>> happen in real life, but it happened, and it's not helping our stress
>> tests ;-). Basically there is no safe guards to prevent something like
>> this from happening, hence the patch.
>
> We assume in __cpu_up() is that if boot_secondary() returns success, the
> CPU is well on its way to coming online - iow, platform_secondary_init()
> has been reached, and we have relatively little stuff to do before we see
> the CPU coming online.
>
> I can't see that your code allows that assumption to be made, as
> boot_secondary() always returns success.
>
> Maybe you need your wait-for-reset-vector to be longer, and on failure
> place the CPU back into reset to ensure that it doesn't corrupt the
> reset vector if it starts late?
>

Definitely. This would be exactly the right place to place any holding logic...

Thanks again,
A

[RFC] Make SMP secondary CPU up more resilient to failure.

[Russell King - ARM Linux]

On Sat, Dec 18, 2010 at 05:54:25AM -0600, Andrei Warkentin wrote:
> That's a good question. I think the best answer I can come up right
> now is to find another dual-core ARM platform and run our stress tests
> to get it to fail in the same fashion. The issue you pointed out -
> namely, the byzantine design of the hotplug code, does not affect the
> up path. Proof: boot_secondary always succeeds.

Send me your test case, and provided it can work in a limited filesystem
I can test it on the 4-core Versatile Express system, and an OMAP4
platform.

> I am not convinced with your writel argument either. Proof:
> boot_secondary always succeeded,

Your boot_secondary() _always_ succeeds.  It can never fail!  I think
that in itself makes some of your anaysis invalid (as I suspect you're
basing the fact that the secondary CPU is within kernel code to be upon
the reset vector being modified.)

> plus the writel expands to a write followed by a dsb and
> outer cache sync.

Covering what I said in the other email, it is the other way around.
It's a dsb+outer sync followed by the write.

> And the writel location looked polled at
> boot_secondary is in memory. Doesn't the dsb drain the write buffer?
> In all the failure cases the secondary was inside kernel code by the
> time __cpu_up started waiting on the online bit to appear. That means
> that whatever was taking it's sweet time - it was taking it from the
> moment secondary_start_kernel entered to the moment the online bit was
> set. I have printk logs showing that.

Show me the logs then!

> So the question we want to collectively answer is really - should the
> stuffing in secondary_start_kernel really be taking so long, assuming
> an unrealistic load? I don't have a good answer for that, yet. I'm
> going to get some timings for that code.

The answer to that is no.  There's very little which goes on in there.
Between the point where platform_secondary_startup() is called, and
the CPU is set online, we:

1. notify that the CPU is starting.  This calls the CPU notifier list,
   of which (until I added VFP) there are no consumers of this on ARM.
2. enable IRQs.  The secondary core should not be receiving any
   interrupts from peripherals at this time.
3. setup the per-CPU timer, which assuming twd_calibrate_rate() doesn't
   re-run, is just a case of re-registering the clock event driver.
4. calibrate the bogomips - this may take a short time, but certainly
   less than a second.
5. store the CPU specific information into the structure - a couple of
   writes at the most.

The biggest thing in there is (4), which should take less than 64
jiffy-update ticks to complete.

> Btw, I would guess that one of the reasons the hotplug logic is done
> the way it was is because secondary_startup is garbage after init,
> having been freed along with the rest of init memory. Is anybody
> addressing that atm?

No.  The reason its done the way it is is because hotplug CPU was
initially created on an ARM926 SMP platform, where it was not possible
to do anything with the secondary cores but to place them in WFI mode.
The same method applies to Realview and Versatile Express - you can
only place the cores into WFI mode, you can't stop them any other way.

The original hotplug CPU code which I received from ARM used to just
return straight back to the idle loop without re-initializing the CPU.
I fixed that to re-run as much of the initialization from as early on
as I possibly could (which was basically jumping back to
secondary_startup).  In order to take it back any further requires
jumping through large hoops to take the MMU down.

As such, the initial SMP assembly bring up code never got tested for
hotplug-safeness, and so the problem wasn't noticed until recently -
and that's now been fixed.

However, other people copied the Realview code without really thinking,
(maybe because of the buggy ASM code) but that's all been fixed and
everyone can now sort out their implementations.

[RFC] Make SMP secondary CPU up more resilient to failure.

[Russell King - ARM Linux]

On Sat, Dec 18, 2010 at 06:10:49AM -0600, Andrei Warkentin wrote:
> Definitely. This would be exactly the right place to place any holding
> logic...

FYI, I've managed to get some timing figures out of my Versatile
Express platform.  It takes about 100us for a CPU to come online via
hotplug, and a further 222ms to run the calibration before marking the
CPU online.

That leaves a margin of about 750ms before the timeout in the generic code
fires.

CPU requesting hotplug, times in ns:
SMP: Start: 0
SMP: Booting: 750
SMP: Cross call: 3500
SMP: Pen released: 41167
SMP: Unlock: 42417
SMP: Boot returned: 43250

CPU being brought online, referenced to "SMP: Start" above, times in ns.
SMP: Sec: restart: 3834
SMP: Sec: up: 5334
SMP: Sec: enter: 30667
SMP: Sec: pen write: 38917
SMP: Sec: pen done: 41417
SMP: Sec: exit: 42750
SMP: Sec: calibrate: 91834
SMP: Sec: online: 222054375

[RFC] Make SMP secondary CPU up more resilient to failure.

[Andrei Warkentin]

Russel,

Thank you! The culprit looks as it seems to be the writel without
__iowmb, as you pointed out. At the very least I've yet to hit the
problem again this way.

I still want to add code inside the platform SMP support as a safety
net. Maybe I am being too pedantic, but  In the near future (with
those 40 patches), secondaries are going to boot directly via
secondary_startup as well, so the first time platform-specific code
gets invoked is platform_secondary_init. I want to ensure that when
boot_secondary returns, the CPU is either guaranteed to be running or
for-sure dead. The problem is that platform_secondary_init is already
too late - if the CPU gets killed due to timeout anytime between the
entry to secondary_start_kernel and  platform_secondary_init, it could
have already increased the refcount on init_mm or disabled preemption.

I propose a platform_secondary_preinit, which would be invoked right
before the "Booted secondary processor" printk. This will give a
chance to synchronize with the platform boot_secondary and would be
the point, for example, where the reset vector is written with the
smpid of the booted processor, so that boot_secondary knows it came
up.

What do you think?
A

On Sat, Dec 18, 2010 at 2:04 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sat, Dec 18, 2010 at 06:10:49AM -0600, Andrei Warkentin wrote:
>> Definitely. This would be exactly the right place to place any holding
>> logic...
>
> FYI, I've managed to get some timing figures out of my Versatile
> Express platform. ?It takes about 100us for a CPU to come online via
> hotplug, and a further 222ms to run the calibration before marking the
> CPU online.
>
> That leaves a margin of about 750ms before the timeout in the generic code
> fires.
>
> CPU requesting hotplug, times in ns:
> SMP: Start: 0
> SMP: Booting: 750
> SMP: Cross call: 3500
> SMP: Pen released: 41167
> SMP: Unlock: 42417
> SMP: Boot returned: 43250
>
> CPU being brought online, referenced to "SMP: Start" above, times in ns.
> SMP: Sec: restart: 3834
> SMP: Sec: up: 5334
> SMP: Sec: enter: 30667
> SMP: Sec: pen write: 38917
> SMP: Sec: pen done: 41417
> SMP: Sec: exit: 42750
> SMP: Sec: calibrate: 91834
> SMP: Sec: online: 222054375
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-ARM-SMP-Do-early-platform-init-on-entering-secondary.patch
Type: text/x-patch
Size: 1507 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20101221/31913d2c/attachment-0001.bin>

[RFC] Make SMP secondary CPU up more resilient to failure.

[Russell King - ARM Linux]

On Tue, Dec 21, 2010 at 03:53:46PM -0600, Andrei Warkentin wrote:
> Russel,

Grr.

> Thank you! The culprit looks as it seems to be the writel without
> __iowmb, as you pointed out. At the very least I've yet to hit the
> problem again this way.

Good news.

> I still want to add code inside the platform SMP support as a safety
> net. Maybe I am being too pedantic, but  In the near future (with
> those 40 patches), secondaries are going to boot directly via
> secondary_startup as well, so the first time platform-specific code
> gets invoked is platform_secondary_init. I want to ensure that when
> boot_secondary returns, the CPU is either guaranteed to be running or
> for-sure dead. The problem is that platform_secondary_init is already
> too late - if the CPU gets killed due to timeout anytime between the
> entry to secondary_start_kernel and  platform_secondary_init, it could
> have already increased the refcount on init_mm or disabled preemption.

Here's a problem for you to ponder on over Christmas.

Let's say the secondary CPU is running slowly due to system load.  It
makes it through to secondary_start_kernel(), and calls through to
your preinit function.  It checks that it should be booting, and
passes that test.

At this point, the requesting CPU times out, but gets preempted to
other tasks (which could very well happen on a heavily loaded system
with preempt enabled).

The booting CPU signals that via writing the reset vector, and continues
on to increment the mm_count and switch its page tables.

The requesting CPU finally switches back to the thread requesting
that the CPU be brought up.  It decides as it timed out to kill the
booting CPU, and does so.

What this means that we now have exactly the same scenario you've
referred to above, and adding the pre-init function hasn't really
solved anything.

I _really_ don't want platforms to start playing these games, because
we'll end up with lots of different attempts to solve the problem,
each of them probably racy like the above.  The safest solution is to
use a longer timeout - maybe an excessively long timeout - to guarantee
that we never miss a starting CPU.

If we do end up needing something like this in the kernel, then it needs
to be done carefully and in generic code where it can be done properly
once.  (If any bugs are found in it, we've also only one version to fix,
not five or six different versions.)  However, I'd argue that it's better
to wait longer for the CPU to come up if there's a possibility that it
will rather than trying to sort out the mess from a partially booted
secondary CPU.

[RFC] Make SMP secondary CPU up more resilient to failure.

[Andrei Warkentin]

On Fri, Dec 24, 2010 at 11:38 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
>
> On Tue, Dec 21, 2010 at 03:53:46PM -0600, Andrei Warkentin wrote:
> > Russel,
>
> Grr.
>
> > Thank you! The culprit looks as it seems to be the writel without
> > __iowmb, as you pointed out. At the very least I've yet to hit the
> > problem again this way.
>
> Good news.
>
> > I still want to add code inside the platform SMP support as a safety
> > net. Maybe I am being too pedantic, but ?In the near future (with
> > those 40 patches), secondaries are going to boot directly via
> > secondary_startup as well, so the first time platform-specific code
> > gets invoked is platform_secondary_init. I want to ensure that when
> > boot_secondary returns, the CPU is either guaranteed to be running or
> > for-sure dead. The problem is that platform_secondary_init is already
> > too late - if the CPU gets killed due to timeout anytime between the
> > entry to secondary_start_kernel and ?platform_secondary_init, it could
> > have already increased the refcount on init_mm or disabled preemption.
>
> Here's a problem for you to ponder on over Christmas.
>
> Let's say the secondary CPU is running slowly due to system load. ?It
> makes it through to secondary_start_kernel(), and calls through to
> your preinit function. ?It checks that it should be booting, and
> passes that test.
>
> At this point, the requesting CPU times out, but gets preempted to
> other tasks (which could very well happen on a heavily loaded system
> with preempt enabled).
>
> The booting CPU signals that via writing the reset vector, and continues
> on to increment the mm_count and switch its page tables.

My goal was for the preinit to run explicitely before the mm_count is
incremented. The cpu
sits (spins) inside the preinit until it is either told to continue
with the init (thus the synchronized CPU knows it succeeded), or it
sits there spinning inside the preinit until it gets killed due to a
timeout. Since I think the "side effects" only start after the
mm_count is incremented, I thought right before would be a good place.

>
> The requesting CPU finally switches back to the thread requesting
> that the CPU be brought up. ?It decides as it timed out to kill the
> booting CPU, and does so.

I should have made this clearer in my email when I said 'synchronize',
but if the timeout ever occurs it means two things -
1) CPU is dead or someplace before secondary_start_kernel
2) CPU is about to enter/entering preinit
3) CPU is already spinning inside preinit waiting to be allowed to
continue. It hasn't incremented mm_count, switched pts, or done
anything else that affects global kernel state.

In either case, it can be torn down (by say, fiddling with the power/reset).

If the timeout doesn't occur, then the requesting cpu will allow the
secondary to quit spinning inside the preinit.

>
> What this means that we now have exactly the same scenario you've
> referred to above, and adding the pre-init function hasn't really
> solved anything.
>
> I _really_ don't want platforms to start playing these games, because
> we'll end up with lots of different attempts to solve the problem,
> each of them probably racy like the above. ?The safest solution is to
> use a longer timeout - maybe an excessively long timeout - to guarantee
> that we never miss a starting CPU.
>
> If we do end up needing something like this in the kernel, then it needs
> to be done carefully and in generic code where it can be done properly
> once. ?(If any bugs are found in it, we've also only one version to fix,
> not five or six different versions.)

I fully agree. Would you be interested in me bringing back the actual
synchronization code from platform-dependent code into the preinit
function and posting that as a patch for review?

?However, I'd argue that it's better
> to wait longer for the CPU to come up if there's a possibility that it
> will rather than trying to sort out the mess from a partially booted
> secondary CPU.

Fair enough, I suppose that does make any platform bugs in smp path
more immediately obvious :)

[RFC] Make SMP secondary CPU up more resilient to failure.

[Russell King - ARM Linux]

On Thu, Jan 13, 2011 at 04:19:40AM -0600, Andrei Warkentin wrote:
> I fully agree. Would you be interested in me bringing back the actual
> synchronization code from platform-dependent code into the preinit
> function and posting that as a patch for review?

I really don't like the idea of a preinit function - it's completely
unnecessary as I've been trying to point out.  It has been shown that
it takes a hundred or so microseconds to get into the kernel, and
then a couple of hundred milliseconds to run the delay loop.

As platforms are expected to wait in their boot_secondary() for the
first half - currently platforms wait one second - we're talking
about around a hundred microseconds vs a timeout of one second.
That's a factor of 10000 beyond what's required.

It has also been shown that the problem you were seeing was down to
synchronization/delayed write bugs which have since been solved - and
adding yet more synchronization is not the answer to buggy
synchronization.

So, as the timeouts are already well in excess and the root cause of
your problem has been resolved, I see no need to make this stuff more
complex - the more complexity there is, the more chance there is of
things going wrong.

[RFC] Make SMP secondary CPU up more resilient to failure.

[Andrei Warkentin]

On Thu, Jan 13, 2011 at 5:14 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
>
> On Thu, Jan 13, 2011 at 04:19:40AM -0600, Andrei Warkentin wrote:
> > I fully agree. Would you be interested in me bringing back the actual
> > synchronization code from platform-dependent code into the preinit
> > function and posting that as a patch for review?
>
> I really don't like the idea of a preinit function - it's completely
> unnecessary as I've been trying to point out. ?It has been shown that
> it takes a hundred or so microseconds to get into the kernel, and
> then a couple of hundred milliseconds to run the delay loop.
>
> As platforms are expected to wait in their boot_secondary() for the
> first half - currently platforms wait one second - we're talking
> about around a hundred microseconds vs a timeout of one second.
> That's a factor of 10000 beyond what's required.
>
> It has also been shown that the problem you were seeing was down to
> synchronization/delayed write bugs which have since been solved - and
> adding yet more synchronization is not the answer to buggy
> synchronization.
>
> So, as the timeouts are already well in excess and the root cause of
> your problem has been resolved, I see no need to make this stuff more
> complex - the more complexity there is, the more chance there is of
> things going wrong.

Fair enough :).