Messages in this thread | | | Date | Tue, 18 Mar 2014 19:49:51 +0100 | From | Igor Mammedov <> | Subject | Re: [PATCH 0/3] x86: fix hang when AP bringup is too slow |
| |
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.
> > 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(); + }
> > 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 */
> > Apologies for the late review, > > P.
-- Regards, Igor
| |