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

[Julien Grall <julien.grall@xxxxxxxxxx>]

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@xxxxxxxxxx>
>>> ---
>>>  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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel