From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Tosatti Subject: Re: [PATCH 08/10] Move IO APIC to its own lock. Date: Wed, 15 Jul 2009 18:38:48 -0300 Message-ID: <20090715213848.GA10313@amt.cnet> References: <1247581845-7625-1-git-send-email-gleb@redhat.com> <1247581845-7625-9-git-send-email-gleb@redhat.com> <20090715175759.GA5689@amt.cnet> <20090715204817.GE8356@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org To: Gleb Natapov Return-path: Received: from mx2.redhat.com ([66.187.237.31]:59964 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751615AbZGOVi5 (ORCPT ); Wed, 15 Jul 2009 17:38:57 -0400 Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26]) by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id n6FLcwHU019690 for ; Wed, 15 Jul 2009 17:38:58 -0400 Content-Disposition: inline In-Reply-To: <20090715204817.GE8356@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Wed, Jul 15, 2009 at 11:48:17PM +0300, Gleb Natapov wrote: > > > + spin_unlock(&ioapic->lock); > > > + kvm_notify_acked_irq(ioapic->kvm, KVM_IRQCHIP_IOAPIC, i); > > > + spin_lock(&ioapic->lock); > > > > While traversing the IOAPIC pins you drop the lock and acquire it again, > > which is error prone: what if the redirection table is changed in > > between, for example? > > > Yes, the ack notifiers is a problematic part. The only thing that > current ack notifiers do is set_irq_level(0) so this is not the problem > in practice. I'll see if I can eliminate this dropping of the lock here. Currently, yes. But take into account that an ack notifier might do other things than set_irq_level(0). Say for example an ack notifier that takes the PIC or IOAPIC lock (for whatever reason). > > Also, irq_lock was held during ack and mask notifier callbacks, so they > > (the callbacks) did not have to worry about concurrency. > > > Is it possible to have more then one ack for the same interrupt line at > the same time? Why not? But maybe this is a somewhat stupid point, because you can require the notifiers to handle that. > > You questioned the purpose of irq_lock earlier, and thats what it is: > > synchronization between pic and ioapic blur at the irq notifiers. > > > Pic has its own locking and it fails miserably in regards ack notifiers. > I bet nobody tried device assignment with pic. It will not work. It fails to take irq_lock on automatic EOI because on guest entry interrupts are disabled (see d5ecfdd25c412df9c0ecf3ab8e066461fd4f69df). > irq_lock is indeed used to synchronization ioapic access, but the way > it is done requires calling kvm_set_irq() with lock held and this adds > unnecessary lock on a msi irq injection path. > > > Using RCU to synchronize ack/mask notifier list walk versus list > > addition is good, but i'm not entirely sure that smaller locks like you > > are proposing is a benefit long term (things become much more tricky). > Without removing irq_lock RCU is useless since the list walking is always > done with irq_lock held. I see smaller locks as simplification BTW. The > thing is tricky now, or so I felt while trying to understand what > irq_lock actually protects. With smaller locks with names that clearly > indicates which data structure it protects I feel the code is much > easy to understand. What is worrying is if you need to take both PIC and IOAPIC locks for some operation (then have to worry about lock ordering etc). > > Maybe it is the only way forward (and so you push this complexity to the > > ack/mask notifiers), but i think it can be avoided until its proven that > > there is contention on irq_lock (which is reduced by faster search). > This is not about contention. This is about existence of the lock in the > first place. With the patch set there is no lock at msi irq injection > path at all and this is better than having lock no matter if the lock is > congested or not. There is still a lock on ioapic path (which MSI does > not use) and removing of this lock should be done only after we see > that it is congested, because removing it involves adding a number of > atomic accesses, so it is not clear win without lock contention. > (removing it will also solve ack notification problem hmmm)