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

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

[Ian Campbell]

Otherwise continue without it, which is preferable to the current
infinite hang.

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);
+
+    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);
+
+    if ( !cpu_online(cpu) )
+    {
+        printk("CPU%d never came online\n", cpu);
+        return -EIO;
+    }
+
     return 0;
 }
 
-- 
1.7.10.4

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

[Julien Grall]

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

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

[Ian Campbell]

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?

>  I guess
> this deadline is for the time to initialize the CPU for Xen? If so,
> maybe you can add a comment for it.

I would have thought it was evident from the patch that it is how long
we will wait for cpu_online(cpu) to become true. Does that really need
explaining in prose?

> Also, any reason for 1s?

Anything taking longer than that is pretty broken IMHO. It also happens
to be the Linux time out.

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

Ian.

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

[Julien Grall]

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.

>>  I guess
>> this deadline is for the time to initialize the CPU for Xen? If so,
>> maybe you can add a comment for it.
> 
> I would have thought it was evident from the patch that it is how long
> we will wait for cpu_online(cpu) to become true. Does that really need
> explaining in prose?

I was just checking because I found 1s a bit too much.

>> Also, any reason for 1s?
> 
> Anything taking longer than that is pretty broken IMHO. It also happens
> to be the Linux time out.

Ok.

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

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

Regards,

-- 
Julien Grall

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

[Ian Campbell]

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.

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

[Julien Grall]

On 14/01/15 14:56, Ian Campbell wrote:
>>> 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.

Right. This is what I was trying to say.

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

True. I will give a look later when I will have time.

With the change in the comment:

Reviewed-by: Julien Grall <julien.grall@linaro.org>

Regards,

-- 
Julien Grall