From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: [PATCH 1/3] KVM: Fold assigned interrupt work into IRQ handler Date: Mon, 01 Nov 2010 17:34:19 +0100 Message-ID: <4CCEEC0B.2050903@web.de> References: <9a0f9b99c8eef0c0b14c28339e2fefd6b0f987b6.1288620511.git.jan.kiszka@web.de> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enig07A87E41196E57292C980E58" Cc: kvm , Alex Williamson , "Michael S. Tsirkin" To: Avi Kivity , Marcelo Tosatti Return-path: Received: from fmmailgate02.web.de ([217.72.192.227]:39044 "EHLO fmmailgate02.web.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750815Ab0KAQeW (ORCPT ); Mon, 1 Nov 2010 12:34:22 -0400 In-Reply-To: <9a0f9b99c8eef0c0b14c28339e2fefd6b0f987b6.1288620511.git.jan.kiszka@web.de> Sender: kvm-owner@vger.kernel.org List-ID: This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig07A87E41196E57292C980E58 Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Am 01.11.2010 15:08, Jan Kiszka wrote: > From: Jan Kiszka >=20 > The complete work handler runs with assigned_dev_lock acquired and > interrupts disabled, so there is nothing to gain pushing this work out > of the actually IRQ handler. Fold them together. Err, forget it. kvm_set_irq pulls in the famous pic lock, and that one is not prepared to be called from IRQ context (lockdep just complained). I will try to clean this up via a threaded IRQ handler, maybe using the slow-path only for INTx-type IRQs and pushing MSIs directly from the hard handler. Jan >=20 > Signed-off-by: Jan Kiszka > --- > include/linux/kvm_host.h | 1 - > virt/kvm/assigned-dev.c | 71 +++++++++++++++-----------------------= -------- > 2 files changed, 23 insertions(+), 49 deletions(-) >=20 > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index 462b982..df5497f 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -465,7 +465,6 @@ struct kvm_guest_msix_entry { > =20 > struct kvm_assigned_dev_kernel { > struct kvm_irq_ack_notifier ack_notifier; > - struct work_struct interrupt_work; > struct list_head list; > int assigned_dev_id; > int host_segnr; > diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c > index 7c98928..5c1b56a 100644 > --- a/virt/kvm/assigned-dev.c > +++ b/virt/kvm/assigned-dev.c > @@ -55,18 +55,24 @@ static int find_index_from_host_irq(struct kvm_assi= gned_dev_kernel > return index; > } > =20 > -static void kvm_assigned_dev_interrupt_work_handler(struct work_struct= *work) > +static irqreturn_t kvm_assigned_dev_intr(int irq, void *dev_id) > { > - struct kvm_assigned_dev_kernel *assigned_dev; > - int i; > - > - assigned_dev =3D container_of(work, struct kvm_assigned_dev_kernel, > - interrupt_work); > + struct kvm_assigned_dev_kernel *assigned_dev =3D > + (struct kvm_assigned_dev_kernel *) dev_id; > + unsigned long flags; > =20 > - spin_lock_irq(&assigned_dev->assigned_dev_lock); > + spin_lock_irqsave(&assigned_dev->assigned_dev_lock, flags); > if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_MSIX) { > struct kvm_guest_msix_entry *guest_entries =3D > assigned_dev->guest_msix_entries; > + int index =3D find_index_from_host_irq(assigned_dev, irq); > + int i; > + > + if (index < 0) > + goto out; > + > + guest_entries[index].flags |=3D KVM_ASSIGNED_MSIX_PENDING; > + > for (i =3D 0; i < assigned_dev->entries_nr; i++) { > if (!(guest_entries[i].flags & > KVM_ASSIGNED_MSIX_PENDING)) > @@ -76,33 +82,15 @@ static void kvm_assigned_dev_interrupt_work_handler= (struct work_struct *work) > assigned_dev->irq_source_id, > guest_entries[i].vector, 1); > } > - } else > + } else { > kvm_set_irq(assigned_dev->kvm, assigned_dev->irq_source_id, > assigned_dev->guest_irq, 1); > =20 > - spin_unlock_irq(&assigned_dev->assigned_dev_lock); > -} > - > -static irqreturn_t kvm_assigned_dev_intr(int irq, void *dev_id) > -{ > - unsigned long flags; > - struct kvm_assigned_dev_kernel *assigned_dev =3D > - (struct kvm_assigned_dev_kernel *) dev_id; > - > - spin_lock_irqsave(&assigned_dev->assigned_dev_lock, flags); > - if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_MSIX) { > - int index =3D find_index_from_host_irq(assigned_dev, irq); > - if (index < 0) > - goto out; > - assigned_dev->guest_msix_entries[index].flags |=3D > - KVM_ASSIGNED_MSIX_PENDING; > - } > - > - schedule_work(&assigned_dev->interrupt_work); > - > - if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_GUEST_INTX) { > - disable_irq_nosync(irq); > - assigned_dev->host_irq_disabled =3D true; > + if (assigned_dev->irq_requested_type & > + KVM_DEV_IRQ_GUEST_INTX) { > + disable_irq_nosync(irq); > + assigned_dev->host_irq_disabled =3D true; > + } > } > =20 > out: > @@ -147,33 +135,23 @@ static void deassign_guest_irq(struct kvm *kvm, > assigned_dev->irq_requested_type &=3D ~(KVM_DEV_IRQ_GUEST_MASK); > } > =20 > -/* The function implicit hold kvm->lock mutex due to cancel_work_sync(= ) */ > static void deassign_host_irq(struct kvm *kvm, > struct kvm_assigned_dev_kernel *assigned_dev) > { > /* > - * In kvm_free_device_irq, cancel_work_sync return true if: > - * 1. work is scheduled, and then cancelled. > - * 2. work callback is executed. > - * > - * The first one ensured that the irq is disabled and no more events > - * would happen. But for the second one, the irq may be enabled (e.g.= > - * for MSI). So we disable irq here to prevent further events. > + * We disable irq here to prevent further events. > * > * Notice this maybe result in nested disable if the interrupt type i= s > * INTx, but it's OK for we are going to free it. > * > * If this function is a part of VM destroy, please ensure that till > * now, the kvm state is still legal for probably we also have to wai= t > - * interrupt_work done. > + * on a currently running IRQ handler. > */ > if (assigned_dev->irq_requested_type & KVM_DEV_IRQ_HOST_MSIX) { > int i; > for (i =3D 0; i < assigned_dev->entries_nr; i++) > - disable_irq_nosync(assigned_dev-> > - host_msix_entries[i].vector); > - > - cancel_work_sync(&assigned_dev->interrupt_work); > + disable_irq(assigned_dev->host_msix_entries[i].vector); > =20 > for (i =3D 0; i < assigned_dev->entries_nr; i++) > free_irq(assigned_dev->host_msix_entries[i].vector, > @@ -185,8 +163,7 @@ static void deassign_host_irq(struct kvm *kvm, > pci_disable_msix(assigned_dev->dev); > } else { > /* Deal with MSI and INTx */ > - disable_irq_nosync(assigned_dev->host_irq); > - cancel_work_sync(&assigned_dev->interrupt_work); > + disable_irq(assigned_dev->host_irq); > =20 > free_irq(assigned_dev->host_irq, (void *)assigned_dev); > =20 > @@ -558,8 +535,6 @@ static int kvm_vm_ioctl_assign_device(struct kvm *k= vm, > match->irq_source_id =3D -1; > match->kvm =3D kvm; > match->ack_notifier.irq_acked =3D kvm_assigned_dev_ack_irq; > - INIT_WORK(&match->interrupt_work, > - kvm_assigned_dev_interrupt_work_handler); > =20 > list_add(&match->list, &kvm->arch.assigned_dev_head); > =20 --------------enig07A87E41196E57292C980E58 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.15 (GNU/Linux) Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org/ iEYEARECAAYFAkzO7AsACgkQitSsb3rl5xSOxACg2NUtpZzIM+HtI3b6TjvW73dN UPQAn1a4dFR90ck1Xx31N3gGNcAVmxfi =XTJO -----END PGP SIGNATURE----- --------------enig07A87E41196E57292C980E58--