Date:	Tue, 25 Mar 2014 13:44:22 +0100
From:	Igor Mammedov <imammedo@...hat.com>
To:	Prarit Bhargava <prarit@...hat.com>
Cc:	linux-kernel@...r.kernel.org, tglx@...utronix.de, mingo@...hat.com,
	hpa@...or.com, bp@...e.de, paul.gortmaker@...driver.com,
	JBeulich@...e.com, drjones@...hat.com, toshi.kani@...com,
	x86@...nel.org, riel@...hat.com, gong.chen@...ux.intel.com
Subject: Re: [PATCH 0/3] x86: fix hang when AP bringup is too slow

On Tue, 25 Mar 2014 07:36:07 -0400
Prarit Bhargava <prarit@...hat.com> wrote:

> 
> 
> On 03/19/2014 08:54 AM, Igor Mammedov wrote:
> > On Wed, 19 Mar 2014 07:51:05 -0400
> > Prarit Bhargava <prarit@...hat.com> wrote:
> > 
> >>
> >>
> >> On 03/18/2014 02:49 PM, Igor Mammedov wrote:
> >>> On Tue, 18 Mar 2014 08:21:19 -0400
> >>> Prarit Bhargava <prarit@...hat.com> wrote:
> >>>
> >>>>
> >>>>
> >>>> On 03/13/2014 10:25 AM, Igor Mammedov wrote:
> >>>>> Hang is observed on virtual machines during CPU hotplug,
> >>>>> especially in big guests with many CPUs. (It happens more
> >>>>> often if host is over-committed).
> >>>>>
> >>>>
> >>>> Hey Igor, I like this better than the previous version.  Thanks for taking into
> >>>> account the possible races in this code.
> >>>>
> >>>> A quick question on system behaviour.  As you know I've been more concerned
> >>>> lately with error handling, etc., through the cpu hotplug code as we've seen
> >>>> several customer reports of silent failures or cascading failures in the cpu
> >>>> hotplug code when users have been attempting to perform physical hotplug.
> >>>>
> >>>> After your patches have been applied, in theory the following can happen:
> >>>>
> >>>> The master CPU is completing the AP cpu's bring up.  The AP cpu is doing (sorry
> >>>> for the cut-and-paste),
> >>>>
> >>>> void cpu_init(void)
> >>>> {
> >>>>         int cpu = smp_processor_id();
> >>>>         struct task_struct *curr = current;
> >>>>         struct tss_struct *t = &per_cpu(init_tss, cpu);
> >>>>         struct thread_struct *thread = &curr->thread;
> >>>>
> >>>>         /*
> >>>>          * wait till the master CPU completes it's STARTUP sequence,
> >>>>          * and decides to wait till this AP boots
> >>>>          */
> >>>>         while (!cpumask_test_cpu(cpu, cpu_callout_mask)) {
> >>>>                 cpu_relax();
> >>>>                 if (per_cpu(x86_cpu_to_apicid, cpu) == BAD_APICID)
> >>>>                         halt();
> >>>>         }
> >>>>
> >>>> and is spinning on cpu_relax().  Suppose something goes wrong and the softlockup
> >>>> watchdog fires on the AP cpu:
> >>>>
> >>>> 1.  Can it? :) ie) will the softlockup fire at this point of the AP init?  Okay,
> >>>> I'm being really lazy and not looking at the code ;)
> >>> It shouldn't, CPU is in pristine state and just came from boot trampoline at
> >>> this point without interrupts configured yet.
> >>
> >> Okay, not a big problem.
> >>
> >>>
> >>>>
> >>>> 2.  Is there anything we can do in this code to notify the user of a problem?
> >>>> Even a pr_crit() here I think would help to indicate what went wrong; it might
> >>>> be useful for future debugging in this area to have some sort of output.  I
> >>>> think a WARN() or BUG() is necessary here as there are several calls to cpu_init().
> >>> Do you mean something like this:
> >>>
> >>> +		if (per_cpu(x86_cpu_to_apicid, cpu) == BAD_APICID) {
> >>> +                       WARN(1);
> >>> +			halt();
> >>> +               }
> >>
> >> Yeah, maybe WARN_ON(1, "some comment") though.
> > printk at so early stage might be cause issues, since it is quite complex.
> > Its' disabling/enabling irqs, calls *_delay_*() functions and takes locks.
> > The last is especially dangerous because if AP is shot down by another
> > INIT/SIPI, system will hang on next printk if locks were acquired by AP
> > at that time.
> 
> early_printk()?
early_printk could mess with console output, when another CPU does output
on the associated console.

Just to be safe I'd avoid to do anything here, provided that master CPU
will print error message.


> 
> > That case is possible if master CPU has got errors during wakeup_ap() and
> > failed cpu_up() then it was unplugged + plugged via ACPI and attempted
> > to be onlined again. 
> > 
> > It's much safer not to do anything complex at AP start-up so early.
> > 
> > BTW:
> > when AP reaches halt() line, failure is not silent. the master CPU might
> > print error message if debug level logging is active:
> > see arch/x86/kernel/smpboot.c:native_cpu_up()
> > ...
> >         if (err) {
> >                 pr_debug("do_boot_cpu failed %d\n", err);
> >                 return -EIO;
> >         }
> > ...
> > 
> > perhaps we should change pr_debug to pr_crit here to make it more visible.
> > something like:
> > 
> > @@ -858,7 +858,7 @@ int native_cpu_up(unsigned int cpu, struct task_struct *tidle)
> >  
> >         err = do_boot_cpu(apicid, cpu, tidle);
> >         if (err) {
> > -               pr_debug("do_boot_cpu failed %d\n", err);
> > +               pr_crit("do_boot_cpu failed(%d) to wakeup CPU#%u\n", err, cpu);
> >                 return -EIO;
> >         }
> > 
> 
> Yes, this is a good idea.
Ok, I'll respin series with above patch added.

> 
> > 
> >>
> >>>
> >>>>
> >>>> 3.  Change this comment:
> >>>>
> >>>>          * wait till the master CPU completes it's STARTUP sequence,
> >>>>          * and decides to wait till this AP boots
> >>>>
> >>>> to
> >>>>
> >>>> 	/* wait for the master CPU to complete this cpu's STARTUP. */ ?
> >>> well, that is not quite the same as above, comment should underline that
> >>> AP waits for ACK from master CPU before continuing with this AP initialization.
> >>>
> >>> How about:
> >>> /* wait for ACK from master CPU before continuing with AP initialization */
> >>
> >> Awesome :)
> >>
> >> P.
> >>
> >>>
> >>>>
> >>>> Apologies for the late review,
> >>>>
> >>>> P.
> >>>
> >>>
> > 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists