From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 6/8 v2] Move IO APIC to its own lock. Date: Wed, 12 Aug 2009 12:57:07 +0300 Message-ID: <4A8291F3.2030106@redhat.com> References: <1249993895-11119-1-git-send-email-gleb@redhat.com> <1249993895-11119-7-git-send-email-gleb@redhat.com> <4A827CE1.7040304@redhat.com> <20090812090458.GZ4764@redhat.com> <4A8288D2.9000607@redhat.com> <20090812094708.GB4764@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org To: Gleb Natapov Return-path: Received: from mx2.redhat.com ([66.187.237.31]:59132 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932341AbZHLJ5I (ORCPT ); Wed, 12 Aug 2009 05:57:08 -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 n7C9v9ED004256 for ; Wed, 12 Aug 2009 05:57:09 -0400 In-Reply-To: <20090812094708.GB4764@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 08/12/2009 12:47 PM, Gleb Natapov wrote: >> Ah, the real motivation is msi. Pushing locks down doesn't help if we >> keep locking them. But for msi we avoid the lock entirely. >> >> > Yes. MSI is one. Multiple IOAPICs may inject interrupt in parallel too > (if we will choose to implement multiple IOAPICs sometime). > Right. Given msi, I don't think we'll have multiple ioapics though. >>>> Why a spinlock and not a mutex? >>>> >>>> >>>> >>> Protected sections are small and we do not sleep there. >>> >>> >> So what? A mutex is better since it allows preemption (and still has >> spinlock performance if it isn't preempted). >> >> > This lock will be taken during irq injection from irqfd, so may be leave > it as spinlock and take it _irqsave()? Do we want to allow irq injection > from interrupt context? Otherwise if you say that performance is the > same I don't care one way or the other. > Let's leave _irqsave() until later since that has other implications. >>>> Need to explain why this is safe. I'm not sure it is, because we touch >>>> state afterwards in pic_intack(). We need to do all vcpu-synchronous >>>> operations before dropping the lock. >>>> >>>> >>> Forst pic_intack() calls pic_clear_isr() only in auto eoi mode and this mode >>> is already broken for assigned devices. Second for level triggered >>> interrupts pic_intack() does nothing after calling pic_clear_isr() and >>> third I can move pic_clear_isr() call to the end of pic_intack(). >>> >>> >> I meant, in a comment. >> > I you agree with above I'll add it as a comment. > Sure. >> It should. What if there's a reset with an assigned device? We need to >> release the device interrupt (after doing FLR?). >> > Doing this will just re-enable host interrupt while irq condition is not > cleared in the device. The host will hang. So I think we really shouldn't. > Ok. What about timer acks? >>> I don't see why we clear remote_irr before dropping the lock. If, while >>> lock was dropped, interrupt was delivered to this entry it will be >>> injected when ack notifier returns. >>> >>> >> But we'll clear remote_irr afterward the redelivery, and we should to >> that only after the new interrupt is acked. >> > It depend on whether you consider calling ack notifiers a part of > interrupt acknowledgement process. I don't really care, but the ack process has to be atomic. Since we need to drop the lock, it means the notifier is not part of the process. > If you do then remote_irr should not > be cleared before ack notifiers since ack process is not completed yet. > With current users functionally it shouldn't matter when we clear > remote_irr. I prefer doing it like we do it now since this how it was > before my patches and since code is simpler this way. > No, I think it introduces a race if an interrupt is raised while the ack notifier is running. >> No, I mean keep the for loop in kvm_ioapic_update_eoi(). >> >> > Can't do that. __kvm_ioapic_update_eoi() is called from other place with > lock held already. > Ok. -- error compiling committee.c: too many arguments to function