[RFC] Make SMP secondary CPU up more resilient to failure.
Russell King - ARM Linux
linux at arm.linux.org.uk
Fri Dec 17 19:08:28 EST 2010
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 at 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 at 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 at 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.
More information about the linux-arm-kernel
mailing list