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?:) Thanks, Xiaowei [ 2631.686041] Call Trace: [ 2631.689073] <IRQ> [<ffffffff811a8b5f>] ? xen_poll_irq+0x49/0x53 [ 2631.695338] [<ffffffff8100ff74>] xen_spin_lock_slow+0x13f/0x204 [ 2631.703151] [<ffffffff810100ef>] xen_spin_lock_flags+0xb6/0xe6 [ 2631.709405] [<ffffffff81050d68>] ? delayed_work_timer_fn+0x0/0x33 [ 2631.715663] [<ffffffff813283ba>] _spin_lock_irqsave+0x30/0x39 [ 2631.723479] [<ffffffff81050d42>] __queue_work+0x18/0x3e [ 2631.728171] [<ffffffff81050d97>] delayed_work_timer_fn+0x2f/0x33 [ 2631.735988] [<ffffffff81049f41>] run_timer_softirq+0x160/0x1f1 [ 2631.743804] [<ffffffff811a7488>] ? unmask_evtchn+0x34/0xd6 [ 2631.748493] [<ffffffff81045f6f>] __do_softirq+0xa2/0x13d [ 2631.754756] [<ffffffff81013e0c>] call_softirq+0x1c/0x30 [ 2631.761007] [<ffffffff81015950>] do_softirq+0x42/0x88 [ 2631.767262] [<ffffffff81045ecb>] irq_exit+0x3f/0x41 [ 2631.771955] [<ffffffff811a82af>] xen_evtchn_do_upcall+0x13e/0x15a [ 2631.779774] [<ffffffff81013e5e>] xen_do_hypervisor_callback+0x1e/0x30 [ 2631.787584] <EOI> [<ffffffff8100ff5d>] ? xen_spin_lock_slow+0x128/0x204 [ 2631.795407] [<ffffffff810100ef>] ? xen_spin_lock_flags+0xb6/0xe6 [ 2631.801696] [<ffffffff810862a4>] ? lru_add_drain_per_cpu+0x0/0xb [ 2631.809483] [<ffffffff813283ba>] ? _spin_lock_irqsave+0x30/0x39 [ 2631.815743] [<ffffffff81328520>] ? _spin_unlock_irqrestore+0x27/0x2a [ 2631.823557] [<ffffffff81053fb3>] ? finish_wait+0x3b/0x67 [ 2631.828251] [<ffffffff81050f7b>] ? worker_thread+0xb6/0x1f9 [ 2631.836067] [<ffffffff81053e8e>] ? autoremove_wake_function+0x0/0x38 [ 2631.842325] [<ffffffff81050ec5>] ? worker_thread+0x0/0x1f9 [ 2631.848576] [<ffffffff81053aeb>] ? kthread+0x8f/0x97 [ 2631.854829] [<ffffffff81013d0a>] ? child_rip+0xa/0x20 [ 2631.861083] [<ffffffff81012ee7>] ? int_ret_from_sys_call+0x7/0x1b [ 2631.867343] [<ffffffff8101369d>] ? retint_restore_args+0x5/0x6 [ 2631.875159] [<ffffffff81013d00>] ? child_rip+0x0/0x20 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jeremy Fitzhardinge
2009-Sep-08 23:35 UTC
[Xen-devel] Re: One issue of pvops dom0''s spinlock code
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? Hm, we can end up actually holding the lock with interrupts enabled, which isn''t going to be good. Does this help: 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] <IRQ> [<ffffffff811a8b5f>] ? xen_poll_irq+0x49/0x53 > [ 2631.695338] [<ffffffff8100ff74>] xen_spin_lock_slow+0x13f/0x204 > [ 2631.703151] [<ffffffff810100ef>] xen_spin_lock_flags+0xb6/0xe6 > [ 2631.709405] [<ffffffff81050d68>] ? delayed_work_timer_fn+0x0/0x33 > [ 2631.715663] [<ffffffff813283ba>] _spin_lock_irqsave+0x30/0x39 > [ 2631.723479] [<ffffffff81050d42>] __queue_work+0x18/0x3e > [ 2631.728171] [<ffffffff81050d97>] delayed_work_timer_fn+0x2f/0x33 > [ 2631.735988] [<ffffffff81049f41>] run_timer_softirq+0x160/0x1f1 > [ 2631.743804] [<ffffffff811a7488>] ? unmask_evtchn+0x34/0xd6 > [ 2631.748493] [<ffffffff81045f6f>] __do_softirq+0xa2/0x13d > [ 2631.754756] [<ffffffff81013e0c>] call_softirq+0x1c/0x30 > [ 2631.761007] [<ffffffff81015950>] do_softirq+0x42/0x88 > [ 2631.767262] [<ffffffff81045ecb>] irq_exit+0x3f/0x41 > [ 2631.771955] [<ffffffff811a82af>] xen_evtchn_do_upcall+0x13e/0x15a > [ 2631.779774] [<ffffffff81013e5e>] xen_do_hypervisor_callback+0x1e/0x30 > [ 2631.787584] <EOI> [<ffffffff8100ff5d>] ? > xen_spin_lock_slow+0x128/0x204 > [ 2631.795407] [<ffffffff810100ef>] ? xen_spin_lock_flags+0xb6/0xe6 > [ 2631.801696] [<ffffffff810862a4>] ? lru_add_drain_per_cpu+0x0/0xb > [ 2631.809483] [<ffffffff813283ba>] ? _spin_lock_irqsave+0x30/0x39 > [ 2631.815743] [<ffffffff81328520>] ? _spin_unlock_irqrestore+0x27/0x2a > [ 2631.823557] [<ffffffff81053fb3>] ? finish_wait+0x3b/0x67 > [ 2631.828251] [<ffffffff81050f7b>] ? worker_thread+0xb6/0x1f9 > [ 2631.836067] [<ffffffff81053e8e>] ? autoremove_wake_function+0x0/0x38 > [ 2631.842325] [<ffffffff81050ec5>] ? worker_thread+0x0/0x1f9 > [ 2631.848576] [<ffffffff81053aeb>] ? kthread+0x8f/0x97 > [ 2631.854829] [<ffffffff81013d0a>] ? child_rip+0xa/0x20 > [ 2631.861083] [<ffffffff81012ee7>] ? int_ret_from_sys_call+0x7/0x1b > [ 2631.867343] [<ffffffff8101369d>] ? retint_restore_args+0x5/0x6 > [ 2631.875159] [<ffffffff81013d00>] ? child_rip+0x0/0x20 >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Yang, Xiaowei
2009-Sep-09 16:26 UTC
[Xen-devel] Re: One issue of pvops dom0''s spinlock code
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] <IRQ> [<ffffffff811a8b5f>] ? xen_poll_irq+0x49/0x53 >> [ 2631.695338] [<ffffffff8100ff74>] xen_spin_lock_slow+0x13f/0x204 >> [ 2631.703151] [<ffffffff810100ef>] xen_spin_lock_flags+0xb6/0xe6 >> [ 2631.709405] [<ffffffff81050d68>] ? delayed_work_timer_fn+0x0/0x33 >> [ 2631.715663] [<ffffffff813283ba>] _spin_lock_irqsave+0x30/0x39 >> [ 2631.723479] [<ffffffff81050d42>] __queue_work+0x18/0x3e >> [ 2631.728171] [<ffffffff81050d97>] delayed_work_timer_fn+0x2f/0x33 >> [ 2631.735988] [<ffffffff81049f41>] run_timer_softirq+0x160/0x1f1 >> [ 2631.743804] [<ffffffff811a7488>] ? unmask_evtchn+0x34/0xd6 >> [ 2631.748493] [<ffffffff81045f6f>] __do_softirq+0xa2/0x13d >> [ 2631.754756] [<ffffffff81013e0c>] call_softirq+0x1c/0x30 >> [ 2631.761007] [<ffffffff81015950>] do_softirq+0x42/0x88 >> [ 2631.767262] [<ffffffff81045ecb>] irq_exit+0x3f/0x41 >> [ 2631.771955] [<ffffffff811a82af>] xen_evtchn_do_upcall+0x13e/0x15a >> [ 2631.779774] [<ffffffff81013e5e>] xen_do_hypervisor_callback+0x1e/0x30 >> [ 2631.787584] <EOI> [<ffffffff8100ff5d>] ? >> xen_spin_lock_slow+0x128/0x204 >> [ 2631.795407] [<ffffffff810100ef>] ? xen_spin_lock_flags+0xb6/0xe6 >> [ 2631.801696] [<ffffffff810862a4>] ? lru_add_drain_per_cpu+0x0/0xb >> [ 2631.809483] [<ffffffff813283ba>] ? _spin_lock_irqsave+0x30/0x39 >> [ 2631.815743] [<ffffffff81328520>] ? _spin_unlock_irqrestore+0x27/0x2a >> [ 2631.823557] [<ffffffff81053fb3>] ? finish_wait+0x3b/0x67 >> [ 2631.828251] [<ffffffff81050f7b>] ? worker_thread+0xb6/0x1f9 >> [ 2631.836067] [<ffffffff81053e8e>] ? autoremove_wake_function+0x0/0x38 >> [ 2631.842325] [<ffffffff81050ec5>] ? worker_thread+0x0/0x1f9 >> [ 2631.848576] [<ffffffff81053aeb>] ? kthread+0x8f/0x97 >> [ 2631.854829] [<ffffffff81013d0a>] ? child_rip+0xa/0x20 >> [ 2631.861083] [<ffffffff81012ee7>] ? int_ret_from_sys_call+0x7/0x1b >> [ 2631.867343] [<ffffffff8101369d>] ? retint_restore_args+0x5/0x6 >> [ 2631.875159] [<ffffffff81013d00>] ? child_rip+0x0/0x20 >> >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel