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:29:49 -0400 Message-ID: <4A5B36CD.6010507@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> <4A5B33BF.2090409@gmail.com> <20090713132506.GL28046@redhat.com> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enigBA2BF90E267778B9576B885A" 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.182]:55930 "EHLO wa-out-1112.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755388AbZGMNhu (ORCPT ); Mon, 13 Jul 2009 09:37:50 -0400 Received: by wa-out-1112.google.com with SMTP id j5so418564wah.21 for ; Mon, 13 Jul 2009 06:37:50 -0700 (PDT) In-Reply-To: <20090713132506.GL28046@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enigBA2BF90E267778B9576B885A Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Gleb Natapov wrote: > On Mon, Jul 13, 2009 at 09:16:47AM -0400, Gregory Haskins wrote: > =20 >> Gleb Natapov wrote: >> =20 >>> On Mon, Jul 13, 2009 at 09:01:33AM -0400, Gregory Haskins wrote: >>> =20 >>> =20 >>>> Gleb Natapov wrote: >>>> =20 >>>> =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= _id, 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 >>>>> =20 >>>> Hi Gleb, >>>> I haven't had a chance to fully digest and review these patches, b= ut >>>> one thing I did notice is that you seem to be converting from a list= to >>>> an open-coded structure. I am just curious why you made this design= >>>> decision instead of using the RCU variant of list? >>>> >>>> =20 >>>> =20 >>> It is not scary "open-coded structure" it's just an array :) As I res= ponded >>> to Michael the idea is to move msis out of irq_routing, make the arra= y >>> much smaller and either use gsi as an index in the array or use hash = table >>> instead looping over all entries. For now I can justify array as more= >>> cache friendly data structure as we scan it linearly. >>> =20 >>> =20 >> Ok, but that might be a good thing to mention in the patch header ;) >> >> =20 > What exactly? Besides this is just an RFC. By the time it will be > applied (if at all) I may do the change already :) > =20 Heh, thats fine. FWIW, I would suggest this: "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 tabl= e instead looping over all entries. For now I can justify array as more cache friendly data structure as we scan it linearly" Otherwise, reviewers might be curious why you are not using list_X_rcu() = ;) Kind Regards, -Greg --------------enigBA2BF90E267778B9576B885A 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 iEYEARECAAYFAkpbNs0ACgkQP5K2CMvXmqHL9QCfYDfI00q6Krbqdc4qTffFFxSI IJQAn3B8nahz0Nxq629SUDG3Kjuqjs4l =gTqT -----END PGP SIGNATURE----- --------------enigBA2BF90E267778B9576B885A--