From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Yang, Xiaowei" Subject: Re: One issue of pvops dom0's spinlock code Date: Thu, 10 Sep 2009 00:26:07 +0800 Message-ID: <4AA7D71F.5070507@intel.com> References: <4AA4BAFE.50906@intel.com> <4AA6EA28.4050609@goop.org> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <4AA6EA28.4050609@goop.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Jeremy Fitzhardinge Cc: "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org Jeremy Fitzhardinge wrote: > On 09/07/09 00:49, Yang, Xiaowei wrote: >> As we tested pvops dom0, sometimes we met some vCPUs hung due to dead >> lock. After checking the dom0's stack (see below) and the code, we >> found it's caused by this commit: 1e696f638 (xen: allow interrupts to >> be enabled while doing a blocking spin). If we don't enable irq inside >> spinlock slow path the issue is gone. >> >> Jeremy, >> Can you have a check of this?:) > > It seemed like a good idea at the time... > > There's an inherent fragility if there's a nested lock (ie, the > interrupt handler takes the same spinlock that the outer code is waiting > on), it relies on the inner lock leaving the wakeup event pending to > stop the outer lock from block-spinning indefinitely. > > But that doesn't appear to be what's happening in your case; the inner > lock is spinning indefinitely... > > One possibility is that this is a bug in the generic kernel; the > standard ticket-lock implementation doesn't enable interrupts while > spinning, so perhaps we're provoking a bug in doing so. But I can't see > how enabling interrupts while spinning before taking the lock is > behaviourally any different from taking an uncontended lock with > interrupts enabled. > > BTW, assume this is after applying your barrier->wmb patch? Yes. > > Hm, we can end up actually holding the lock with interrupts enabled, > which isn't going to be good. Does this help: The patch looks OK to me. I think it can avoid the dead lock I met by preventing holding the lock with interrupt enabled. I will have it a try when I get the test machine. Thanks, xiaowei > > diff --git a/arch/x86/xen/spinlock.c b/arch/x86/xen/spinlock.c > index 5601506..2f91e56 100644 > --- a/arch/x86/xen/spinlock.c > +++ b/arch/x86/xen/spinlock.c > @@ -187,7 +187,6 @@ static noinline int xen_spin_lock_slow(struct raw_spinlock *lock, bool irq_enabl > struct xen_spinlock *prev; > int irq = __get_cpu_var(lock_kicker_irq); > int ret; > - unsigned long flags; > u64 start; > > /* If kicker interrupts not initialized yet, just spin */ > @@ -199,16 +198,12 @@ static noinline int xen_spin_lock_slow(struct raw_spinlock *lock, bool irq_enabl > /* announce we're spinning */ > prev = spinning_lock(xl); > > - flags = __raw_local_save_flags(); > - if (irq_enable) { > - ADD_STATS(taken_slow_irqenable, 1); > - raw_local_irq_enable(); > - } > - > ADD_STATS(taken_slow, 1); > ADD_STATS(taken_slow_nested, prev != NULL); > > do { > + unsigned long flags; > + > /* clear pending */ > xen_clear_irq_pending(irq); > > @@ -228,6 +223,12 @@ static noinline int xen_spin_lock_slow(struct raw_spinlock *lock, bool irq_enabl > goto out; > } > > + flags = __raw_local_save_flags(); > + if (irq_enable) { > + ADD_STATS(taken_slow_irqenable, 1); > + raw_local_irq_enable(); > + } > + > /* > * Block until irq becomes pending. If we're > * interrupted at this point (after the trylock but > @@ -238,13 +239,15 @@ static noinline int xen_spin_lock_slow(struct raw_spinlock *lock, bool irq_enabl > * pending. > */ > xen_poll_irq(irq); > + > + raw_local_irq_restore(flags); > + > ADD_STATS(taken_slow_spurious, !xen_test_irq_pending(irq)); > } while (!xen_test_irq_pending(irq)); /* check for spurious wakeups */ > > kstat_incr_irqs_this_cpu(irq, irq_to_desc(irq)); > > out: > - raw_local_irq_restore(flags); > unspinning_lock(xl, prev); > spin_time_accum_blocked(start); > > > > > J > >> Thanks, >> Xiaowei >> >> [ 2631.686041] Call Trace: >> [ 2631.689073] [] ? xen_poll_irq+0x49/0x53 >> [ 2631.695338] [] xen_spin_lock_slow+0x13f/0x204 >> [ 2631.703151] [] xen_spin_lock_flags+0xb6/0xe6 >> [ 2631.709405] [] ? delayed_work_timer_fn+0x0/0x33 >> [ 2631.715663] [] _spin_lock_irqsave+0x30/0x39 >> [ 2631.723479] [] __queue_work+0x18/0x3e >> [ 2631.728171] [] delayed_work_timer_fn+0x2f/0x33 >> [ 2631.735988] [] run_timer_softirq+0x160/0x1f1 >> [ 2631.743804] [] ? unmask_evtchn+0x34/0xd6 >> [ 2631.748493] [] __do_softirq+0xa2/0x13d >> [ 2631.754756] [] call_softirq+0x1c/0x30 >> [ 2631.761007] [] do_softirq+0x42/0x88 >> [ 2631.767262] [] irq_exit+0x3f/0x41 >> [ 2631.771955] [] xen_evtchn_do_upcall+0x13e/0x15a >> [ 2631.779774] [] xen_do_hypervisor_callback+0x1e/0x30 >> [ 2631.787584] [] ? >> xen_spin_lock_slow+0x128/0x204 >> [ 2631.795407] [] ? xen_spin_lock_flags+0xb6/0xe6 >> [ 2631.801696] [] ? lru_add_drain_per_cpu+0x0/0xb >> [ 2631.809483] [] ? _spin_lock_irqsave+0x30/0x39 >> [ 2631.815743] [] ? _spin_unlock_irqrestore+0x27/0x2a >> [ 2631.823557] [] ? finish_wait+0x3b/0x67 >> [ 2631.828251] [] ? worker_thread+0xb6/0x1f9 >> [ 2631.836067] [] ? autoremove_wake_function+0x0/0x38 >> [ 2631.842325] [] ? worker_thread+0x0/0x1f9 >> [ 2631.848576] [] ? kthread+0x8f/0x97 >> [ 2631.854829] [] ? child_rip+0xa/0x20 >> [ 2631.861083] [] ? int_ret_from_sys_call+0x7/0x1b >> [ 2631.867343] [] ? retint_restore_args+0x5/0x6 >> [ 2631.875159] [] ? child_rip+0x0/0x20 >> >