Messages in this thread Patch in this message | | | Date | Wed, 19 Mar 2014 13:54:56 +0100 | From | Igor Mammedov <> | Subject | Re: [PATCH 0/3] x86: fix hang when AP bringup is too slow |
| |
On Wed, 19 Mar 2014 07:51:05 -0400 Prarit Bhargava <prarit@redhat.com> wrote:
> > > On 03/18/2014 02:49 PM, Igor Mammedov wrote: > > On Tue, 18 Mar 2014 08:21:19 -0400 > > Prarit Bhargava <prarit@redhat.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. 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; }
> > > > >> > >> 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. > > > >
| |