__cpu_up vs. start_secondary race?

__cpu_up vs. start_secondary race?

[Nathan Lynch]

Hi,

I think there may be a plausible issue here.  If not, maybe I'll get
an education :)

cpu_callin_map is used during secondary CPU bootstrap to notify the
waiting CPU that the new CPU is coming up.  __cpu_up clears
cpu_callin_map[cpu] and then polls the same location, waiting for
start_secondary to set it to 1.  But I'm wondering how safe the
current implementation is -- start_secondary doesn't have an explicit
sync following cpu_callin_map[cpu] = 1, and __cpu_up has no
synchronization instructions in its polling loop, so how can we be
sure that the waiting cpu will see the update to that location in
time?

Compare with the prom_hold_cpus/__secondary_hold_acknowledge code,
which is doing a very similar task, but it has the mb and sync (in
head_64.S at least) that seem to be missing from the case above.

Since we're not buried in "Processor X is stuck" bug reports, I must
be missing something, or there's some incidental factor that makes it
okay in practice...

Relevant code from arch/powerpc/kernel/smp.c:

static volatile unsigned int cpu_callin_map[NR_CPUS];

....

int __cpuinit __cpu_up(unsigned int cpu)
{
        int c;

        secondary_ti = current_set[cpu];
        if (!cpu_enable(cpu))
                return 0;

        if (smp_ops == NULL ||
            (smp_ops->cpu_bootable && !smp_ops->cpu_bootable(cpu)))
                return -EINVAL;

        /* Make sure callin-map entry is 0 (can be leftover a CPU
         * hotplug
         */
        cpu_callin_map[cpu] = 0;

        /* The information for processor bringup must
         * be written out to main store before we release
         * the processor.
         */
        smp_mb();

        /* wake up cpus */
        DBG("smp: kicking cpu %d\n", cpu);
        smp_ops->kick_cpu(cpu);

        /*
         * wait to see if the cpu made a callin (is actually up).
         * use this value that I found through experimentation.
         * -- Cort
         */
        if (system_state < SYSTEM_RUNNING)
                for (c = 50000; c && !cpu_callin_map[cpu]; c--)
                        udelay(100);
#ifdef CONFIG_HOTPLUG_CPU
        else
                /*
                 * CPUs can take much longer to come up in the
                 * hotplug case.  Wait five seconds.
                 */
                for (c = 25; c && !cpu_callin_map[cpu]; c--) {
                        msleep(200);
                }
#endif

        if (!cpu_callin_map[cpu]) {
                printk("Processor %u is stuck.\n", cpu);
                return -ENOENT;
        }

        printk("Processor %u found.\n", cpu);

        if (smp_ops->give_timebase)
                smp_ops->give_timebase();

        /* Wait until cpu puts itself in the online map */
        while (!cpu_online(cpu))
                cpu_relax();

        return 0;
}
....

int __devinit start_secondary(void *unused)
{
        unsigned int cpu = smp_processor_id();
        struct device_node *l2_cache;
        int i, base;

        atomic_inc(&init_mm.mm_count);
        current->active_mm = &init_mm;

        smp_store_cpu_info(cpu);
        set_dec(tb_ticks_per_jiffy);
        preempt_disable();
        cpu_callin_map[cpu] = 1;

        smp_ops->setup_cpu(cpu);
        if (smp_ops->take_timebase)
                smp_ops->take_timebase();
....

Re: __cpu_up vs. start_secondary race?

[Benjamin Herrenschmidt]

On Mon, 2008-12-01 at 15:30 -0600, Nathan Lynch wrote:
> Hi,
> 
> I think there may be a plausible issue here.  If not, maybe I'll get
> an education :)
> 
> cpu_callin_map is used during secondary CPU bootstrap to notify the
> waiting CPU that the new CPU is coming up.  __cpu_up clears
> cpu_callin_map[cpu] and then polls the same location, waiting for
> start_secondary to set it to 1.  But I'm wondering how safe the
> current implementation is -- start_secondary doesn't have an explicit
> sync following cpu_callin_map[cpu] = 1, and __cpu_up has no
> synchronization instructions in its polling loop, so how can we be
> sure that the waiting cpu will see the update to that location in
> time?

I think it works because there's no big ordering problem (though we
should still probably stick a few barriers here for safety) so it's
really just a problem of how long it takes for the store to be visible,
and the duration of the waiting loop is such that in practice, it will
end up being visible wayyyyy before we timeout.

IE. It's not like stores get buffered for ever due to absence of
barriers. They ultimately get out to the bus.

Cheers,
Ben.

Re: __cpu_up vs. start_secondary race?

[Nathan Lynch]

Benjamin Herrenschmidt wrote:
> On Mon, 2008-12-01 at 15:30 -0600, Nathan Lynch wrote:
> > 
> > cpu_callin_map is used during secondary CPU bootstrap to notify the
> > waiting CPU that the new CPU is coming up.  __cpu_up clears
> > cpu_callin_map[cpu] and then polls the same location, waiting for
> > start_secondary to set it to 1.  But I'm wondering how safe the
> > current implementation is -- start_secondary doesn't have an explicit
> > sync following cpu_callin_map[cpu] = 1, and __cpu_up has no
> > synchronization instructions in its polling loop, so how can we be
> > sure that the waiting cpu will see the update to that location in
> > time?
> 
> I think it works because there's no big ordering problem (though we
> should still probably stick a few barriers here for safety) so it's
> really just a problem of how long it takes for the store to be visible,
> and the duration of the waiting loop is such that in practice, it will
> end up being visible wayyyyy before we timeout.

At least on "real" hardware, yes.  Various 64-bit systems I've tested
see the update after two iterations at most (during boot, didn't check
the hotplug case).

> IE. It's not like stores get buffered for ever due to absence of
> barriers. They ultimately get out to the bus.

Hrm, "ultimately" :)  Okay, thanks.

Apart from barriers (or lack thereof), the fact that __cpu_up gives up
after a more-or-less arbitrary period seems... well, arbitrary.  If we
get to "Processor X is stuck" then something is seriously wrong:
there's either a kernel bug or a platform issue, and the CPU just
kicked is in an unknown state.  Polling indefinitely seems safer, no?
Especially since some hypervisors allow overcommitting processors and
memory, which can introduce latencies in unexpected places.

Re: __cpu_up vs. start_secondary race?

[Trent Piepho]

On Tue, 2 Dec 2008, Nathan Lynch wrote:
> Apart from barriers (or lack thereof), the fact that __cpu_up gives up
> after a more-or-less arbitrary period seems... well, arbitrary.  If we
> get to "Processor X is stuck" then something is seriously wrong:
> there's either a kernel bug or a platform issue, and the CPU just
> kicked is in an unknown state.  Polling indefinitely seems safer, no?

I recently fixed a bug that did this.  There was a bug in how the secondary
CPU's memory was mapped (in some non-mailine code, not fixed).  It was nice
to get the warning and have the kernel not hang.  On embedded systems with
only network access and no persistent storage for system logs, a kernel
hang is a lot more a pain.

Re: __cpu_up vs. start_secondary race?

[Benjamin Herrenschmidt]

On Tue, 2008-12-02 at 20:16 -0600, Nathan Lynch wrote:
> Apart from barriers (or lack thereof), the fact that __cpu_up gives up
> after a more-or-less arbitrary period seems... well, arbitrary.  If we
> get to "Processor X is stuck" then something is seriously wrong:
> there's either a kernel bug or a platform issue, and the CPU just
> kicked is in an unknown state.  Polling indefinitely seems safer, no?
> Especially since some hypervisors allow overcommitting processors and
> memory, which can introduce latencies in unexpected places.

I'm pretty happy to keep the timeout :-) Proved useful in many cases
where we actually fail to bring it up or crash it at bringup. From my
experience, most of the time, the stuck CPU isn't getting in the way and
it gets us a chance to move forward.

Ben.

Re: __cpu_up vs. start_secondary race?

[Nathan Lynch]

Benjamin Herrenschmidt wrote:
> On Tue, 2008-12-02 at 20:16 -0600, Nathan Lynch wrote:
> > Apart from barriers (or lack thereof), the fact that __cpu_up gives up
> > after a more-or-less arbitrary period seems... well, arbitrary.  If we
> > get to "Processor X is stuck" then something is seriously wrong:
> > there's either a kernel bug or a platform issue, and the CPU just
> > kicked is in an unknown state.  Polling indefinitely seems safer, no?
> > Especially since some hypervisors allow overcommitting processors and
> > memory, which can introduce latencies in unexpected places.
> 
> I'm pretty happy to keep the timeout :-) Proved useful in many cases
> where we actually fail to bring it up or crash it at bringup. From my
> experience, most of the time, the stuck CPU isn't getting in the way and
> it gets us a chance to move forward.

Fair enough -- thanks.