Re: [PATCH] xen: arm: wait 1000ms for a CPU to come up, instead of forever

[Ian Campbell <Ian.Campbell@citrix.com>]

On Wed, 2015-01-14 at 14:51 +0000, Julien Grall wrote:
> On 14/01/15 14:39, Ian Campbell wrote:
> > On Wed, 2015-01-14 at 14:27 +0000, Julien Grall wrote:
> >> Hi Ian,
> >>
> >> On 14/01/15 14:01, Ian Campbell wrote:
> >>> Otherwise continue without it, which is preferable to the current
> >>> infinite hang.
> >>
> >> Nice!
> >>
> >>> Slightly tweak the grammar of a comment in the same function.
> >>>
> >>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> >>> ---
> >>>  xen/arch/arm/smpboot.c |   26 ++++++++++++++++++++++++--
> >>>  1 file changed, 24 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> >>> index 14054ae..12538d4 100644
> >>> --- a/xen/arch/arm/smpboot.c
> >>> +++ b/xen/arch/arm/smpboot.c
> >>> @@ -357,6 +357,7 @@ int __init cpu_up_send_sgi(int cpu)
> >>>  int __cpu_up(unsigned int cpu)
> >>>  {
> >>>      int rc;
> >>> +    s_time_t deadline;
> >>>  
> >>>      printk("Bringing up CPU%d\n", cpu);
> >>>  
> >>> @@ -369,7 +370,7 @@ int __cpu_up(unsigned int cpu)
> >>>      /* Tell the remote CPU which stack to boot on. */
> >>>      init_data.stack = idle_vcpu[cpu]->arch.stack;
> >>>  
> >>> -    /* Tell the remote CPU what is it's logical CPU ID */
> >>> +    /* Tell the remote CPU what its logical CPU ID is. */
> >>>      init_data.cpuid = cpu;
> >>>  
> >>>      /* Open the gate for this CPU */
> >>> @@ -386,12 +387,33 @@ int __cpu_up(unsigned int cpu)
> >>>          return rc;
> >>>      }
> >>>  
> >>> -    while ( !cpu_online(cpu) )
> >>> +    deadline = NOW() + MILLISECS(1000);
> >>
> >> Most of cpu_up callbacks are waiting for the CPU to come back.
> > 
> > Where do you mean exactly?
> 
> See for instance exynos5_cpu_power_up.

Appears to be waiting the h/w to acknowledge that the CPU power is on,
which is no guarantee that it is going to actually boot, or even make it
to Xen code.

> 
> >>
> >>> +
> >>> +    while ( !cpu_online(cpu) && NOW() < deadline )
> >>>      {
> >>>          cpu_relax();
> >>>          process_pending_softirqs();
> >>>      }
> >>>  
> >>> +    /*
> >>> +     * Nuke start of day info before checking one last time if the CPU
> >>> +     * actually came online.
> >>> +     *
> >>> +     * Doesn't completely avoid the posibility of it trying to
> >>> +     * progress with another CPUs stack etc, but better than nothing,
> >>> +     * hopefully.
> >>> +     */
> >>> +    init_data.stack = NULL;
> >>> +    init_data.cpuid = ~0;
> >>> +    smp_up_cpu = MPIDR_INVALID;
> >>> +    clean_dcache(smp_up_cpu);
> >>
> >> I don't understand why you need to do this. Is it for pure clean up? If
> >> so, please explain it in the commit message.
> > 
> > Is the comment right above it not sufficient explanation? I can insert
> > at the end of the first paragraph "If it is not online it may still be
> > trying and may show up later" is that would help.
> 
> It's more clear for me with "If it is not online ...".
> 
> For the second paragraph, I would say "It doesn't completely avoid ...".

OK, I'll make both changes.

> Also I was wondering if there is any possibility to turn off the cpu if
> it doesn't come online?

PSCI has a cpu_off, and there will be arch specific mechanisms. Whether
they will work under the circumstances is hard to say. In any case that
is out of scope for this patch.

Ian.