From: "Jan Beulich" <JBeulich@novell.com> To: "Jeremy Fitzhardinge" <jeremy@goop.org> Cc: "Jeremy Fitzhardinge" <jeremy.fitzhardinge@citrix.com>, "Eric Dumazet" <dada1@cosmosbay.com>, <xiyou.wangcong@gmail.com>, "Peter Zijlstra" <peterz@infradead.org>, "Nick Piggin" <npiggin@kernel.dk>, "Srivatsa Vaddagiri" <vatsa@linux.vnet.ibm.com>, "Linux Virtualization" <virtualization@lists.linux-foundation.org>, "Xen-devel" <xen-devel@lists.xensource.com>, "Mathieu Desnoyers" <mathieu.desnoyers@polymtl.ca>, "Avi Kivity" <avi@redhat.com>, "Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>, "H. Peter Anvin" <hpa@zytor.com> Subject: Re: [Xen-devel] Re: [PATCH 09/14] xen/pvticketlock: Xen implementation for PV ticket locks Date: Wed, 17 Nov 2010 10:34:39 +0000 [thread overview] Message-ID: <4CE3BDCF0200007800022BF1@vpn.id2.novell.com> (raw) In-Reply-To: <4CE3A714.9010509@goop.org> >>> On 17.11.10 at 10:57, Jeremy Fitzhardinge <jeremy@goop.org> wrote: > On 11/17/2010 12:52 AM, Jeremy Fitzhardinge wrote: >> On 11/17/2010 12:11 AM, Jan Beulich wrote: >>>>>> On 16.11.10 at 22:08, Jeremy Fitzhardinge <jeremy@goop.org> wrote: >>>> +static void xen_lock_spinning(struct arch_spinlock *lock, unsigned want) >>>> { >>>> - struct xen_spinlock *xl = (struct xen_spinlock *)lock; >>>> - struct xen_spinlock *prev; >>>> int irq = __get_cpu_var(lock_kicker_irq); >>>> - int ret; >>>> + struct xen_lock_waiting *w = &__get_cpu_var(lock_waiting); >>>> + int cpu = smp_processor_id(); >>>> u64 start; >>>> >>>> /* If kicker interrupts not initialized yet, just spin */ >>>> if (irq == -1) >>>> - return 0; >>>> + return; >>>> >>>> start = spin_time_start(); >>>> >>>> - /* announce we're spinning */ >>>> - prev = spinning_lock(xl); >>>> + w->want = want; >>>> + w->lock = lock; >>>> + >>>> + /* This uses set_bit, which atomic and therefore a barrier */ >>>> + cpumask_set_cpu(cpu, &waiting_cpus); >>> Since you don't allow nesting, don't you need to disable >>> interrupts before you touch per-CPU state? >> Yes, I think you're right - interrupts need to be disabled for the bulk >> of this function. > > Actually, on second thoughts, maybe it doesn't matter so much. The main > issue is making sure that the interrupt will make the VCPU drop out of > xen_poll_irq() - if it happens before xen_poll_irq(), it should leave > the event pending, which will cause the poll to return immediately. I > hope. Certainly disabling interrupts for some of the function will make > it easier to analyze with respect to interrupt nesting. That's not my main concern. Instead, what if you get interrupted anywhere here, the interrupt handler tries to acquire another spinlock and also has to go into the slow path? It'll overwrite part or all of the outer context's state. > Another issue may be making sure the writes and reads of "w->want" and > "w->lock" are ordered properly to make sure that xen_unlock_kick() never > sees an inconsistent view of the (lock,want) tuple. The risk being that > xen_unlock_kick() sees a random, spurious (lock,want) pairing and sends > the kick event to the wrong VCPU, leaving the deserving one hung. Yes, proper operation sequence (and barriers) is certainly required here. If you allowed nesting, this may even become simpler (as you'd have a single write making visible the new "head" pointer, after having written all relevant fields of the new "head" structure). Jan
WARNING: multiple messages have this Message-ID (diff)
From: "Jan Beulich" <JBeulich@novell.com> To: Jeremy Fitzhardinge <jeremy@goop.org> Cc: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>, Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>, Nick Piggin <npiggin@kernel.dk>, Peter Zijlstra <peterz@infradead.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Srivatsa Vaddagiri <vatsa@linux.vnet.ibm.com>, Eric Dumazet <dada1@cosmosbay.com>, Xen-devel <xen-devel@lists.xensource.com>, Avi Kivity <avi@redhat.com>, "H. Peter Anvin" <hpa@zytor.com>, xiyou.wangcong@gmail.com, Linux Virtualization <virtualization@lists.linux-foundation.org> Subject: Re: Re: [PATCH 09/14] xen/pvticketlock: Xen implementation for PV ticket locks Date: Wed, 17 Nov 2010 10:34:39 +0000 [thread overview] Message-ID: <4CE3BDCF0200007800022BF1@vpn.id2.novell.com> (raw) In-Reply-To: <4CE3A714.9010509@goop.org> >>> On 17.11.10 at 10:57, Jeremy Fitzhardinge <jeremy@goop.org> wrote: > On 11/17/2010 12:52 AM, Jeremy Fitzhardinge wrote: >> On 11/17/2010 12:11 AM, Jan Beulich wrote: >>>>>> On 16.11.10 at 22:08, Jeremy Fitzhardinge <jeremy@goop.org> wrote: >>>> +static void xen_lock_spinning(struct arch_spinlock *lock, unsigned want) >>>> { >>>> - struct xen_spinlock *xl = (struct xen_spinlock *)lock; >>>> - struct xen_spinlock *prev; >>>> int irq = __get_cpu_var(lock_kicker_irq); >>>> - int ret; >>>> + struct xen_lock_waiting *w = &__get_cpu_var(lock_waiting); >>>> + int cpu = smp_processor_id(); >>>> u64 start; >>>> >>>> /* If kicker interrupts not initialized yet, just spin */ >>>> if (irq == -1) >>>> - return 0; >>>> + return; >>>> >>>> start = spin_time_start(); >>>> >>>> - /* announce we're spinning */ >>>> - prev = spinning_lock(xl); >>>> + w->want = want; >>>> + w->lock = lock; >>>> + >>>> + /* This uses set_bit, which atomic and therefore a barrier */ >>>> + cpumask_set_cpu(cpu, &waiting_cpus); >>> Since you don't allow nesting, don't you need to disable >>> interrupts before you touch per-CPU state? >> Yes, I think you're right - interrupts need to be disabled for the bulk >> of this function. > > Actually, on second thoughts, maybe it doesn't matter so much. The main > issue is making sure that the interrupt will make the VCPU drop out of > xen_poll_irq() - if it happens before xen_poll_irq(), it should leave > the event pending, which will cause the poll to return immediately. I > hope. Certainly disabling interrupts for some of the function will make > it easier to analyze with respect to interrupt nesting. That's not my main concern. Instead, what if you get interrupted anywhere here, the interrupt handler tries to acquire another spinlock and also has to go into the slow path? It'll overwrite part or all of the outer context's state. > Another issue may be making sure the writes and reads of "w->want" and > "w->lock" are ordered properly to make sure that xen_unlock_kick() never > sees an inconsistent view of the (lock,want) tuple. The risk being that > xen_unlock_kick() sees a random, spurious (lock,want) pairing and sends > the kick event to the wrong VCPU, leaving the deserving one hung. Yes, proper operation sequence (and barriers) is certainly required here. If you allowed nesting, this may even become simpler (as you'd have a single write making visible the new "head" pointer, after having written all relevant fields of the new "head" structure). Jan
next prev parent reply other threads:[~2010-11-17 10:34 UTC|newest] Thread overview: 147+ messages / expand[flat|nested] mbox.gz Atom feed top 2010-11-16 21:08 [PATCH 00/14] PV ticket locks without expanding spinlock Jeremy Fitzhardinge 2010-11-16 21:08 ` Jeremy Fitzhardinge 2010-11-16 21:08 ` [PATCH 01/14] x86/ticketlock: clean up types and accessors Jeremy Fitzhardinge 2011-01-11 17:21 ` [Xen-devel] " Konrad Rzeszutek Wilk 2011-01-11 17:21 ` Konrad Rzeszutek Wilk 2010-11-16 21:08 ` Jeremy Fitzhardinge 2010-11-16 21:08 ` [PATCH 02/14] x86/ticketlock: convert spin loop to C Jeremy Fitzhardinge 2010-11-16 21:08 ` Jeremy Fitzhardinge 2010-11-16 21:08 ` Jeremy Fitzhardinge 2010-11-16 21:08 ` [PATCH 03/14] x86/ticketlock: Use C for __ticket_spin_unlock Jeremy Fitzhardinge 2010-11-16 21:08 ` Jeremy Fitzhardinge 2010-11-16 21:08 ` Jeremy Fitzhardinge 2010-11-16 21:08 ` [PATCH 04/14] x86/ticketlock: make large and small ticket versions of spin_lock the same Jeremy Fitzhardinge 2010-11-16 21:08 ` Jeremy Fitzhardinge 2010-11-16 21:08 ` [PATCH 05/14] x86/ticketlock: make __ticket_spin_lock common Jeremy Fitzhardinge 2010-11-16 21:08 ` Jeremy Fitzhardinge 2010-11-16 21:08 ` [PATCH 06/14] x86/ticketlock: make __ticket_spin_trylock common Jeremy Fitzhardinge 2010-11-16 21:08 ` Jeremy Fitzhardinge 2010-11-16 21:08 ` [PATCH 07/14] x86/spinlocks: replace pv spinlocks with pv ticketlocks Jeremy Fitzhardinge 2010-11-16 21:08 ` Jeremy Fitzhardinge 2010-11-16 21:08 ` [PATCH 08/14] x86/ticketlock: collapse a layer of functions Jeremy Fitzhardinge 2010-11-16 21:08 ` Jeremy Fitzhardinge 2010-11-16 21:08 ` [PATCH 09/14] xen/pvticketlock: Xen implementation for PV ticket locks Jeremy Fitzhardinge 2010-11-17 8:11 ` Jan Beulich 2010-11-17 8:11 ` Jan Beulich 2010-11-17 8:11 ` Jan Beulich 2010-11-17 8:52 ` Jeremy Fitzhardinge 2010-11-17 8:52 ` Jeremy Fitzhardinge 2010-11-17 9:57 ` [Xen-devel] " Jeremy Fitzhardinge 2010-11-17 10:34 ` Jan Beulich 2010-11-17 10:34 ` Jan Beulich [this message] 2010-11-17 10:34 ` Jan Beulich 2010-11-17 17:41 ` [Xen-devel] " Jeremy Fitzhardinge 2010-11-17 17:41 ` Jeremy Fitzhardinge 2010-11-17 17:41 ` Jeremy Fitzhardinge 2010-11-17 9:57 ` [Xen-devel] " Jeremy Fitzhardinge 2010-11-16 21:08 ` Jeremy Fitzhardinge 2010-11-16 21:08 ` [PATCH 10/14] x86/pvticketlock: use callee-save for lock_spinning Jeremy Fitzhardinge 2010-11-16 21:08 ` Jeremy Fitzhardinge 2010-11-16 21:08 ` [PATCH 11/14] x86/ticketlock: don't inline _spin_unlock when using paravirt spinlocks Jeremy Fitzhardinge 2010-11-16 21:08 ` Jeremy Fitzhardinge 2010-11-16 21:08 ` [PATCH 12/14] x86/ticketlocks: when paravirtualizing ticket locks, increment by 2 Jeremy Fitzhardinge 2010-11-16 21:08 ` Jeremy Fitzhardinge 2010-11-16 21:08 ` [PATCH 13/14] x86/ticketlock: add slowpath logic Jeremy Fitzhardinge 2010-11-16 21:08 ` Jeremy Fitzhardinge 2010-11-17 8:31 ` Jan Beulich 2010-11-17 8:31 ` Jan Beulich 2010-11-17 8:52 ` Jeremy Fitzhardinge 2010-11-17 8:52 ` Jeremy Fitzhardinge 2010-11-17 8:56 ` Jeremy Fitzhardinge 2010-11-17 8:56 ` Jeremy Fitzhardinge 2010-11-17 9:08 ` Jeremy Fitzhardinge 2010-11-17 9:08 ` Jeremy Fitzhardinge 2010-11-17 9:08 ` Jeremy Fitzhardinge 2010-11-17 9:34 ` Jan Beulich 2010-11-17 9:34 ` Jan Beulich 2010-11-17 9:34 ` Jan Beulich 2010-11-17 8:56 ` Jeremy Fitzhardinge 2010-11-17 8:58 ` Avi Kivity 2010-11-17 8:58 ` Avi Kivity 2010-11-17 9:05 ` Jeremy Fitzhardinge 2010-11-17 9:05 ` Jeremy Fitzhardinge 2010-11-17 9:10 ` Avi Kivity 2010-11-17 9:10 ` Avi Kivity 2010-11-17 9:10 ` Avi Kivity 2010-11-17 9:05 ` Jeremy Fitzhardinge 2010-11-17 8:52 ` Jeremy Fitzhardinge 2010-11-17 8:31 ` Jan Beulich 2010-11-17 12:21 ` Peter Zijlstra 2010-11-17 12:21 ` Peter Zijlstra 2010-11-17 15:25 ` [Xen-devel] " Jeremy Fitzhardinge 2010-11-17 15:25 ` Jeremy Fitzhardinge 2010-11-17 15:25 ` [Xen-devel] " Jeremy Fitzhardinge 2011-01-17 15:22 ` Srivatsa Vaddagiri 2011-01-19 16:23 ` Srivatsa Vaddagiri 2011-01-19 16:23 ` Srivatsa Vaddagiri 2011-01-24 21:56 ` Jeremy Fitzhardinge 2011-01-24 21:56 ` Jeremy Fitzhardinge 2011-02-18 17:03 ` Srivatsa Vaddagiri 2011-02-18 17:03 ` Srivatsa Vaddagiri 2011-02-18 17:03 ` Srivatsa Vaddagiri 2011-01-19 16:23 ` Srivatsa Vaddagiri 2011-01-19 18:31 ` Jeremy Fitzhardinge 2011-01-19 18:31 ` Jeremy Fitzhardinge 2011-01-19 18:31 ` Jeremy Fitzhardinge 2011-01-19 18:39 ` Srivatsa Vaddagiri 2011-01-19 18:39 ` Srivatsa Vaddagiri 2011-01-19 18:55 ` Jeremy Fitzhardinge 2011-01-20 4:28 ` Srivatsa Vaddagiri 2011-01-20 4:28 ` Srivatsa Vaddagiri 2011-01-20 9:52 ` Jan Beulich 2011-01-20 9:52 ` Jan Beulich 2011-01-20 9:52 ` Jan Beulich 2011-01-19 18:55 ` Jeremy Fitzhardinge 2011-01-17 15:22 ` Srivatsa Vaddagiri 2010-11-16 21:08 ` [PATCH 14/14] x86/ticketlocks: tidy up __ticket_unlock_kick() Jeremy Fitzhardinge 2010-11-16 21:08 ` Jeremy Fitzhardinge 2010-11-17 8:56 ` [PATCH 00/14] PV ticket locks without expanding spinlock Avi Kivity 2010-11-17 8:56 ` Avi Kivity 2011-01-19 16:44 ` Srivatsa Vaddagiri 2011-01-19 16:44 ` Srivatsa Vaddagiri 2011-01-19 17:07 ` [PATCH 1/3] debugfs: Add support to print u32 array Srivatsa Vaddagiri 2011-01-19 17:07 ` Srivatsa Vaddagiri 2011-01-19 17:07 ` Srivatsa Vaddagiri 2011-01-19 17:12 ` [PATCH 2/3] kvm hypervisor : Add hypercalls to support pv-ticketlock Srivatsa Vaddagiri 2011-01-19 17:12 ` Srivatsa Vaddagiri 2011-01-19 17:21 ` Peter Zijlstra 2011-01-19 17:21 ` Peter Zijlstra 2011-01-19 17:21 ` Peter Zijlstra 2011-01-19 18:29 ` Srivatsa Vaddagiri 2011-01-19 18:29 ` Srivatsa Vaddagiri 2011-01-19 18:29 ` Srivatsa Vaddagiri 2011-01-19 18:53 ` Jeremy Fitzhardinge 2011-01-19 18:53 ` Jeremy Fitzhardinge 2011-01-20 11:42 ` Srivatsa Vaddagiri 2011-01-20 11:42 ` Srivatsa Vaddagiri 2011-01-20 17:49 ` Jeremy Fitzhardinge 2011-01-20 17:49 ` Jeremy Fitzhardinge 2011-01-20 17:49 ` Jeremy Fitzhardinge 2011-01-20 11:59 ` Srivatsa Vaddagiri 2011-01-20 13:41 ` Peter Zijlstra 2011-01-20 13:41 ` Peter Zijlstra 2011-01-20 13:41 ` Peter Zijlstra 2011-01-20 14:34 ` Srivatsa Vaddagiri 2011-01-20 14:34 ` Srivatsa Vaddagiri 2011-01-20 17:56 ` Jeremy Fitzhardinge 2011-01-20 17:56 ` Jeremy Fitzhardinge 2011-01-21 14:02 ` Srivatsa Vaddagiri 2011-01-21 14:48 ` Rik van Riel 2011-01-21 14:48 ` Rik van Riel 2011-01-22 6:14 ` Srivatsa Vaddagiri 2011-01-22 6:14 ` Srivatsa Vaddagiri 2011-01-22 14:53 ` Rik van Riel 2011-01-22 14:53 ` Rik van Riel 2011-01-24 17:49 ` Jeremy Fitzhardinge 2011-01-24 17:49 ` Jeremy Fitzhardinge 2011-01-22 6:14 ` Srivatsa Vaddagiri 2011-01-21 14:48 ` Rik van Riel 2011-01-21 14:02 ` Srivatsa Vaddagiri 2011-01-20 11:59 ` Srivatsa Vaddagiri 2011-01-19 17:23 ` Srivatsa Vaddagiri 2011-01-19 17:23 ` Srivatsa Vaddagiri 2011-01-19 17:50 ` Peter Zijlstra 2011-01-19 17:50 ` Peter Zijlstra 2011-01-19 17:50 ` Peter Zijlstra 2011-01-19 17:17 ` [PATCH 3/3] kvm guest : Add support for pv-ticketlocks Srivatsa Vaddagiri 2011-01-19 17:17 ` Srivatsa Vaddagiri
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=4CE3BDCF0200007800022BF1@vpn.id2.novell.com \ --to=jbeulich@novell.com \ --cc=avi@redhat.com \ --cc=dada1@cosmosbay.com \ --cc=hpa@zytor.com \ --cc=jeremy.fitzhardinge@citrix.com \ --cc=jeremy@goop.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mathieu.desnoyers@polymtl.ca \ --cc=npiggin@kernel.dk \ --cc=peterz@infradead.org \ --cc=vatsa@linux.vnet.ibm.com \ --cc=virtualization@lists.linux-foundation.org \ --cc=xen-devel@lists.xensource.com \ --cc=xiyou.wangcong@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.