From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gleb Natapov Subject: Re: [PATCH 6/8 v2] Move IO APIC to its own lock. Date: Wed, 12 Aug 2009 13:05:03 +0300 Message-ID: <20090812100503.GD4764@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> <4A8291F3.2030106@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org To: Avi Kivity Return-path: Received: from mx2.redhat.com ([66.187.237.31]:45238 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755158AbZHLKFE (ORCPT ); Wed, 12 Aug 2009 06:05:04 -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 n7CA55XB006789 for ; Wed, 12 Aug 2009 06:05:05 -0400 Content-Disposition: inline In-Reply-To: <4A8291F3.2030106@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Wed, Aug 12, 2009 at 12:57:07PM +0300, Avi Kivity wrote: > 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. > So change it to mutex then? >>>>> 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? > Interrupt wasn't processed by a guest. Why should it be accounted as such? > > > >>>> 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 -- Gleb.