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:18:10 +0300 Message-ID: <4A8288D2.9000607@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> 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]:56498 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932292AbZHLJSM (ORCPT ); Wed, 12 Aug 2009 05:18:12 -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 n7C9IDLL023092 for ; Wed, 12 Aug 2009 05:18:13 -0400 In-Reply-To: <20090812090458.GZ4764@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 08/12/2009 12:04 PM, Gleb Natapov wrote: > On Wed, Aug 12, 2009 at 11:27:13AM +0300, Avi Kivity wrote: > >> On 08/11/2009 03:31 PM, Gleb Natapov wrote: >> >> >> What is the motivation for this change? >> >> > The motivation was explained in 0/0. I want to get rid of lock on > general irq injection path so the lock have to be pushed into ioapic > since multiple cpus can access it concurrently. PIC has such lock > already. > 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. >> 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). >> 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. >>> void kvm_pic_clear_isr_ack(struct kvm *kvm) >>> @@ -238,7 +240,9 @@ void kvm_pic_reset(struct kvm_kpic_state *s) >>> if (vcpu0&& kvm_apic_accept_pic_intr(vcpu0)) >>> if (s->irr& (1<< irq) || s->isr& (1<< irq)) { >>> n = irq + irqbase; >>> + spin_unlock(&s->pics_state->lock); >>> kvm_notify_acked_irq(kvm, SELECT_PIC(n), n); >>> + spin_lock(&s->pics_state->lock); >>> >>> >> Ditto here, needs to be moved until after done changing state. >> >> > I am not sure this code is even needed. IOAPIC don't call notifiers on > reset. > It should. What if there's a reset with an assigned device? We need to release the device interrupt (after doing FLR?). > >>> -static void __kvm_ioapic_update_eoi(struct kvm_ioapic *ioapic, int pin, >>> - int trigger_mode) >>> +static void __kvm_ioapic_update_eoi(struct kvm_ioapic *ioapic, int vector, >>> + int trigger_mode) >>> { >>> - union kvm_ioapic_redirect_entry *ent; >>> + int i; >>> + >>> + for (i = 0; i< IOAPIC_NUM_PINS; i++) { >>> + union kvm_ioapic_redirect_entry *ent =&ioapic->redirtbl[i]; >>> + >>> + if (ent->fields.vector != vector) >>> + continue; >>> >>> - ent =&ioapic->redirtbl[pin]; >>> + spin_unlock(&ioapic->lock); >>> + kvm_notify_acked_irq(ioapic->kvm, KVM_IRQCHIP_IOAPIC, i); >>> + spin_lock(&ioapic->lock); >>> >>> >>> >> I *think* we need to clear remote_irr before dropping the lock. I >> *know* there's a missing comment here. >> > 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. >>> - kvm_notify_acked_irq(ioapic->kvm, KVM_IRQCHIP_IOAPIC, pin); >>> + if (trigger_mode != IOAPIC_LEVEL_TRIG) >>> + continue; >>> >>> - if (trigger_mode == IOAPIC_LEVEL_TRIG) { >>> ASSERT(ent->fields.trig_mode == IOAPIC_LEVEL_TRIG); >>> ent->fields.remote_irr = 0; >>> - if (!ent->fields.mask&& (ioapic->irr& (1<< pin))) >>> - ioapic_service(ioapic, pin); >>> + if (!ent->fields.mask&& (ioapic->irr& (1<< i))) >>> + ioapic_service(ioapic, i); >>> } >>> } >>> >>> >> To make the patch easier to read, suggest keeping the loop in the other >> function. >> >> > I don't follow. All __kvm_ioapic_update_eoi() contains is the loop, so > the loop is already in its own function. Do you mean move the context of > the loop into the other function and leave only for(;;) fun(); in > __kvm_ioapic_update_eoi()? > No, I mean keep the for loop in kvm_ioapic_update_eoi(). -- error compiling committee.c: too many arguments to function