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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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?

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


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