* One issue of pvops dom0's spinlock code @ 2009-09-07 7:49 Yang, Xiaowei 2009-09-08 23:35 ` Jeremy Fitzhardinge 0 siblings, 1 reply; 3+ messages in thread From: Yang, Xiaowei @ 2009-09-07 7:49 UTC (permalink / raw) To: Jeremy Fitzhardinge, xen-devel@lists.xensource.com 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: One issue of pvops dom0's spinlock code 2009-09-07 7:49 One issue of pvops dom0's spinlock code Yang, Xiaowei @ 2009-09-08 23:35 ` Jeremy Fitzhardinge 2009-09-09 16:26 ` Yang, Xiaowei 0 siblings, 1 reply; 3+ messages in thread From: Jeremy Fitzhardinge @ 2009-09-08 23:35 UTC (permalink / raw) To: Yang, Xiaowei; +Cc: xen-devel@lists.xensource.com 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 > ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: One issue of pvops dom0's spinlock code 2009-09-08 23:35 ` Jeremy Fitzhardinge @ 2009-09-09 16:26 ` Yang, Xiaowei 0 siblings, 0 replies; 3+ messages in thread From: Yang, Xiaowei @ 2009-09-09 16:26 UTC (permalink / raw) To: Jeremy Fitzhardinge; +Cc: xen-devel@lists.xensource.com 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 >> > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2009-09-09 16:26 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-09-07 7:49 One issue of pvops dom0's spinlock code Yang, Xiaowei 2009-09-08 23:35 ` Jeremy Fitzhardinge 2009-09-09 16:26 ` Yang, Xiaowei
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.