From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gregory Haskins Subject: Re: [PATCH 4/4] Convert irq notifiers lists to RCU locking. Date: Mon, 13 Jul 2009 10:02:13 -0400 Message-ID: <4A5B3E65.8060309@gmail.com> References: <1247400233-24243-1-git-send-email-gleb@redhat.com> <1247400233-24243-5-git-send-email-gleb@redhat.com> <20090713130256.GC10402@redhat.com> <20090713131128.GI28046@redhat.com> <4A5B35FD.9090208@gmail.com> <20090713133234.GN28046@redhat.com> <4A5B3931.9080508@gmail.com> <20090713135257.GO28046@redhat.com> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enig2F173BA44A03013216434C27" Cc: "Michael S. Tsirkin" , avi@redhat.com, "kvm@vger.kernel.org" , paulmck@linux.vnet.ibm.com To: Gleb Natapov Return-path: Received: from mail-yx0-f184.google.com ([209.85.210.184]:33154 "EHLO mail-yx0-f184.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755764AbZGMOI3 (ORCPT ); Mon, 13 Jul 2009 10:08:29 -0400 Received: by yxe14 with SMTP id 14so3120059yxe.33 for ; Mon, 13 Jul 2009 07:08:29 -0700 (PDT) In-Reply-To: <20090713135257.GO28046@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig2F173BA44A03013216434C27 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Gleb Natapov wrote: > On Mon, Jul 13, 2009 at 09:40:01AM -0400, Gregory Haskins wrote: > =20 >> Gleb Natapov wrote: >> =20 >>> On Mon, Jul 13, 2009 at 09:26:21AM -0400, Gregory Haskins wrote: >>> =20 >>> =20 >>>> Gleb Natapov wrote: >>>> =20 >>>> =20 >>>>> On Mon, Jul 13, 2009 at 04:02:56PM +0300, Michael S. Tsirkin wrote:= >>>>> =20 >>>>> =20 >>>>> =20 >>>>>> On Sun, Jul 12, 2009 at 03:03:53PM +0300, Gleb Natapov wrote: >>>>>> =20 >>>>>> =20 >>>>>> =20 >>>>>>> Use RCU locking for mask/ack notifiers lists. >>>>>>> >>>>>>> Signed-off-by: Gleb Natapov >>>>>>> --- >>>>>>> virt/kvm/irq_comm.c | 20 +++++++++++--------- >>>>>>> 1 files changed, 11 insertions(+), 9 deletions(-) >>>>>>> >>>>>>> diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c >>>>>>> index 5dde1ef..ba3a115 100644 >>>>>>> --- a/virt/kvm/irq_comm.c >>>>>>> +++ b/virt/kvm/irq_comm.c >>>>>>> @@ -179,18 +179,18 @@ void kvm_notify_acked_irq(struct kvm *kvm, = unsigned irqchip, unsigned pin) >>>>>>> break; >>>>>>> } >>>>>>> } >>>>>>> - rcu_read_unlock(); >>>>>>> =20 >>>>>>> - hlist_for_each_entry(kian, n, &kvm->irq_ack_notifier_list, link= ) >>>>>>> + hlist_for_each_entry_rcu(kian, n, &kvm->irq_ack_notifier_list, = link) >>>>>>> if (kian->gsi =3D=3D gsi) >>>>>>> kian->irq_acked(kian); >>>>>>> + rcu_read_unlock(); >>>>>>> } >>>>>>> =20 >>>>>>> void kvm_register_irq_ack_notifier(struct kvm *kvm, >>>>>>> struct kvm_irq_ack_notifier *kian) >>>>>>> { >>>>>>> mutex_lock(&kvm->irq_lock); >>>>>>> - hlist_add_head(&kian->link, &kvm->irq_ack_notifier_list); >>>>>>> + hlist_add_head_rcu(&kian->link, &kvm->irq_ack_notifier_list); >>>>>>> mutex_unlock(&kvm->irq_lock); >>>>>>> } >>>>>>> =20 >>>>>>> @@ -198,8 +198,9 @@ void kvm_unregister_irq_ack_notifier(struct k= vm *kvm, >>>>>>> struct kvm_irq_ack_notifier *kian) >>>>>>> { >>>>>>> mutex_lock(&kvm->irq_lock); >>>>>>> - hlist_del_init(&kian->link); >>>>>>> + hlist_del_init_rcu(&kian->link); >>>>>>> mutex_unlock(&kvm->irq_lock); >>>>>>> + synchronize_rcu(); >>>>>>> } >>>>>>> =20 >>>>>>> =20 >>>>>>> =20 >>>>>> This is done under kvm->lock still, which means the lock might be = held >>>>>> potentially for a very long time. Can synchronize_rcu be moved out= of >>>>>> this lock? >>>>>> >>>>>> =20 >>>>>> =20 >>>>>> =20 >>>>> Only if kvm_free_assigned_device() will be moved out of this lock. >>>>> Device de-assignment is not very frequent event though. How long do= you >>>>> think it may be held? KVM RCU read sections are very brief. >>>>> =20 >>>>> =20 >>>>> =20 >>>> Note that the delay imposed by the barrier is not only related to th= e >>>> length of the critical section. The barrier blocks until the next g= race >>>> period, and depending on the type of RCU you are using and your conf= ig >>>> options, this could be multiple milliseconds. >>>> >>>> I am not saying that this is definitely a problem for your design. = I >>>> am just pointing out that the length of the KVM-RCU read section is = only >>>> =20 >>>> =20 >>> Yeah I understand that other RCU read section may introduce delays to= o. >>> The question is how big the delay may be. >>> =20 >> I think you are misunderstanding me. The read-side CS is not a >> significant factor here so I am not worried about concurrent read-side= >> CS causing a longer delay. What I am saying is that the grace period = of >> your RCU subsystem is the dominant factor in the equation here, and th= is >> may be several milliseconds. >> >> =20 > How is the "grace period" is determined? Isn't it just means "no cpus i= s > in RCU read section anymore"? > =20 Nope ;) RCU is pretty complex, so I won't even try to explain it here as there are numerous articles floating around out there that do a much better job= =2E But here is a summary: RCU buys you two things: 1) concurrent readers *and* writers, and 2) a much lower overhead reader path because it generally doesn't use atomic. Its point (2) that is relevant here. If taking an atomic were ok, you could approximate the RCU model using reference counting. Reference counting buys you "precise" resource acquistion/release at the expense of the overhead of the atomic operation (and any associated cache-line bouncing). RCU uses a "imprecise" model where we don't really know the *exact* moment the resource is released. Instead, there are specific boundaries in time when we can guarantee that it had to have been released prior to the expiry of the event. This event is what is called the "grace period". So that is what synchronize_rcu() is doing. Its a barrier to the next imprecise moment in time when we can be assured (if you used the rest of the RCU API properly) that there can not be any outstanding references to your object left in flight. Each grace period can be milliseconds, depending on what version of the kernel you have and how it is configured= =2E HTH Kind Regards, -Greg > =20 >>> I don't think multiple >>> milliseconds delay in device de-assignment is a big issue though. >>> =20 >>> =20 >> I would tend to agree with you. It's not fast path. >> >> I only brought this up because I saw your design being justified >> incorrectly: you said "KVM RCU read sections are very brief", but tha= t >> is not really relevant to Michael's point. I just want to make sure >> that the true impact is understood. >> >> Kind Regards, >> -Greg >> >> >> =20 > > > > -- > Gleb. > =20 --------------enig2F173BA44A03013216434C27 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 iEYEARECAAYFAkpbPmUACgkQP5K2CMvXmqEMKgCcCSlNMtundHpmLullQ21tcYHq 8d0AmgMdh4uYorJbVgwewXug2+ZaFss/ =ZGsA -----END PGP SIGNATURE----- --------------enig2F173BA44A03013216434C27--