From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gregory Haskins Subject: Re: [PATCH 1/4] Move irq routing data structure to rcu locking Date: Mon, 13 Jul 2009 09:16:47 -0400 Message-ID: <4A5B33BF.2090409@gmail.com> References: <1247400233-24243-1-git-send-email-gleb@redhat.com> <1247400233-24243-2-git-send-email-gleb@redhat.com> <4A5B302D.2040802@novell.com> <20090713131534.GJ28046@redhat.com> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enigE8CC693A6A528C78F77DBCFE" Cc: Gregory Haskins , avi@redhat.com, kvm@vger.kernel.org To: Gleb Natapov Return-path: Received: from wa-out-1112.google.com ([209.85.146.179]:61177 "EHLO wa-out-1112.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755851AbZGMNQ4 (ORCPT ); Mon, 13 Jul 2009 09:16:56 -0400 Received: by wa-out-1112.google.com with SMTP id j5so416798wah.21 for ; Mon, 13 Jul 2009 06:16:55 -0700 (PDT) In-Reply-To: <20090713131534.GJ28046@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enigE8CC693A6A528C78F77DBCFE Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Gleb Natapov wrote: > On Mon, Jul 13, 2009 at 09:01:33AM -0400, Gregory Haskins wrote: > =20 >> Gleb Natapov wrote: >> =20 >>> Signed-off-by: Gleb Natapov >>> --- >>> include/linux/kvm_host.h | 2 +- >>> virt/kvm/irq_comm.c | 55 +++++++++++++++++++++---------------= ---------- >>> virt/kvm/kvm_main.c | 1 - >>> 3 files changed, 26 insertions(+), 32 deletions(-) >>> >>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h >>> index f54a0d3..6756b3e 100644 >>> --- a/include/linux/kvm_host.h >>> +++ b/include/linux/kvm_host.h >>> @@ -161,7 +161,7 @@ struct kvm { >>> =20 >>> struct mutex irq_lock; >>> #ifdef CONFIG_HAVE_KVM_IRQCHIP >>> - struct list_head irq_routing; /* of kvm_kernel_irq_routing_entry */= >>> + struct kvm_kernel_irq_routing_entry *irq_routing; >>> struct hlist_head mask_notifier_list; >>> #endif >>> =20 >>> diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c >>> index 7af18b8..b2fa3f6 100644 >>> --- a/virt/kvm/irq_comm.c >>> +++ b/virt/kvm/irq_comm.c >>> @@ -148,7 +148,8 @@ int kvm_set_irq(struct kvm *kvm, int irq_source_i= d, int irq, int level) >>> * IOAPIC. So set the bit in both. The guest will ignore >>> * writes to the unused one. >>> */ >>> - list_for_each_entry(e, &kvm->irq_routing, link) >>> + rcu_read_lock(); >>> + for (e =3D rcu_dereference(kvm->irq_routing); e && e->set; e++) { >>> =20 >>> =20 >> Hi Gleb, >> I haven't had a chance to fully digest and review these patches, but= >> one thing I did notice is that you seem to be converting from a list t= o >> an open-coded structure. I am just curious why you made this design >> decision instead of using the RCU variant of list? >> >> =20 > It is not scary "open-coded structure" it's just an array :) As I respo= nded > to Michael the idea is to move msis out of irq_routing, make the array > much smaller and either use gsi as an index in the array or use hash ta= ble > instead looping over all entries. For now I can justify array as more > cache friendly data structure as we scan it linearly. > =20 Ok, but that might be a good thing to mention in the patch header ;) Kind Regards, -Greg > =20 >> Regards, >> -Greg >> >> =20 >>> if (e->gsi =3D=3D irq) { >>> int r =3D e->set(e, kvm, sig_level); >>> if (r < 0) >>> @@ -156,6 +157,8 @@ int kvm_set_irq(struct kvm *kvm, int irq_source_i= d, int irq, int level) >>> =20 >>> ret =3D r + ((ret < 0) ? 0 : ret); >>> } >>> + } >>> + rcu_read_unlock(); >>> return ret; >>> } >>> =20 >>> @@ -168,12 +171,15 @@ void kvm_notify_acked_irq(struct kvm *kvm, unsi= gned irqchip, unsigned pin) >>> =20 >>> trace_kvm_ack_irq(irqchip, pin); >>> =20 >>> - list_for_each_entry(e, &kvm->irq_routing, link) >>> + rcu_read_lock(); >>> + for (e =3D rcu_dereference(kvm->irq_routing); e && e->set; e++) { >>> if (e->irqchip.irqchip =3D=3D irqchip && >>> e->irqchip.pin =3D=3D pin) { >>> gsi =3D e->gsi; >>> break; >>> } >>> + } >>> + rcu_read_unlock(); >>> =20 >>> hlist_for_each_entry(kian, n, &kvm->arch.irq_ack_notifier_list, lin= k) >>> if (kian->gsi =3D=3D gsi) >>> @@ -264,19 +270,11 @@ void kvm_fire_mask_notifiers(struct kvm *kvm, i= nt irq, bool mask) >>> kimn->func(kimn, mask); >>> } >>> =20 >>> -static void __kvm_free_irq_routing(struct list_head *irq_routing) >>> -{ >>> - struct kvm_kernel_irq_routing_entry *e, *n; >>> - >>> - list_for_each_entry_safe(e, n, irq_routing, link) >>> - kfree(e); >>> -} >>> - >>> void kvm_free_irq_routing(struct kvm *kvm) >>> { >>> - mutex_lock(&kvm->irq_lock); >>> - __kvm_free_irq_routing(&kvm->irq_routing); >>> - mutex_unlock(&kvm->irq_lock); >>> + /* Called only during vm destruction. Nobody can use the pointer >>> + at this stage */ >>> + kfree(kvm->irq_routing); >>> } >>> =20 >>> static int setup_routing_entry(struct kvm_kernel_irq_routing_entry *= e, >>> @@ -326,43 +324,40 @@ int kvm_set_irq_routing(struct kvm *kvm, >>> unsigned nr, >>> unsigned flags) >>> { >>> - struct list_head irq_list =3D LIST_HEAD_INIT(irq_list); >>> - struct list_head tmp =3D LIST_HEAD_INIT(tmp); >>> - struct kvm_kernel_irq_routing_entry *e =3D NULL; >>> + struct kvm_kernel_irq_routing_entry *new, *old; >>> unsigned i; >>> int r; >>> =20 >>> + /* last element is left zeroed and indicates the end of the array *= / >>> + new =3D kzalloc(sizeof(*new) * (nr + 1), GFP_KERNEL); >>> + >>> + if (!new) >>> + return -ENOMEM; >>> + >>> for (i =3D 0; i < nr; ++i) { >>> r =3D -EINVAL; >>> if (ue->gsi >=3D KVM_MAX_IRQ_ROUTES) >>> goto out; >>> if (ue->flags) >>> goto out; >>> - r =3D -ENOMEM; >>> - e =3D kzalloc(sizeof(*e), GFP_KERNEL); >>> - if (!e) >>> - goto out; >>> - r =3D setup_routing_entry(e, ue); >>> + r =3D setup_routing_entry(new + i, ue); >>> if (r) >>> goto out; >>> ++ue; >>> - list_add(&e->link, &irq_list); >>> - e =3D NULL; >>> } >>> =20 >>> mutex_lock(&kvm->irq_lock); >>> - list_splice(&kvm->irq_routing, &tmp); >>> - INIT_LIST_HEAD(&kvm->irq_routing); >>> - list_splice(&irq_list, &kvm->irq_routing); >>> - INIT_LIST_HEAD(&irq_list); >>> - list_splice(&tmp, &irq_list); >>> + old =3D kvm->irq_routing; >>> + rcu_assign_pointer(kvm->irq_routing, new); >>> mutex_unlock(&kvm->irq_lock); >>> =20 >>> + synchronize_rcu(); >>> + >>> r =3D 0; >>> + new =3D old; >>> =20 >>> out: >>> - kfree(e); >>> - __kvm_free_irq_routing(&irq_list); >>> + kfree(new); >>> return r; >>> } >>> =20 >>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c >>> index cf20dc1..24013b4 100644 >>> --- a/virt/kvm/kvm_main.c >>> +++ b/virt/kvm/kvm_main.c >>> @@ -945,7 +945,6 @@ static struct kvm *kvm_create_vm(void) >>> if (IS_ERR(kvm)) >>> goto out; >>> #ifdef CONFIG_HAVE_KVM_IRQCHIP >>> - INIT_LIST_HEAD(&kvm->irq_routing); >>> INIT_HLIST_HEAD(&kvm->mask_notifier_list); >>> #endif >>> =20 >>> =20 >>> =20 >> =20 > > > > -- > Gleb. > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > =20 --------------enigE8CC693A6A528C78F77DBCFE Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG/MacGPG2 v2.0.11 (Darwin) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iEYEARECAAYFAkpbM78ACgkQP5K2CMvXmqELnQCeMfMI2nRXMxqVV9ztRv4Xlo5Y Um4AnRwTXWDbhHP7ZIgzvdrpUIhRhryM =uz55 -----END PGP SIGNATURE----- --------------enigE8CC693A6A528C78F77DBCFE--