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

[Julien Grall <julien.grall@linaro.org>]

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. I guess
this deadline is for the time to initialize the CPU for Xen? If so,
maybe you can add a comment for it.

Also, any reason for 1s?

> +
> +    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

possibility

> +     * 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.

Regards,

-- 
Julien Grall