From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v3 1/2] IOMMU/spinlock: Fix a bug found in AMD IOMMU initialization Date: Fri, 11 Mar 2016 17:12:01 +0100 Message-ID: <1457712721.3102.610.camel@citrix.com> References: <1457529455-38314-1-git-send-email-quan.xu@intel.com> <1457529455-38314-2-git-send-email-quan.xu@intel.com> <945CA011AD5F084CBEA3E851C0AB28894B861913@SHSMSX101.ccr.corp.intel.com> <1457692537.3102.563.camel@citrix.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0918645880645842684==" Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Meng Xu Cc: "Xu, Quan" , Jan Beulich , Suravee Suthikulpanit , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org --===============0918645880645842684== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-9LiyNjTuntiwp0imyQx6" --=-9LiyNjTuntiwp0imyQx6 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, 2016-03-11 at 09:41 -0500, Meng Xu wrote: > Thank you very much for your explanation and education! They are > really helpful! :-D >=20 > Let me summarize: ;-) > >=20 > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0| spin_lock | > spin_lock_irq | spin_lock_irqsave > >=20 > > Can run in irq context?=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0| No=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0|=C2=A0=C2= =A0Yes > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0| Yes > >=20 > > Can run in irq disabled context?=C2=A0=C2=A0=C2=A0| > > No=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0|=C2=A0=C2=A0No=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0| Yes > The table came out mangled, at least in my mail reader. For what I can see, I think I see the point you're trying to make, and it's hard to say whether the table itself is right or wrong... Probably, a table like this, is just not the best way with which to try to represent things. For instance, you seem to be saying that spin_lock() can't be used if interrupts are disabled, which is not true. For instance, it is perfectly fine to do the following: =C2=A0 =C2=A0 CPU: =C2=A0 =C2=A0 . =C2=A0 =C2=A0 spin_lock_irq(l1); =C2=A0 =C2=A0 . =C2=A0 =C2=A0 . [1] spin_lock(l2); =C2=A0 =C2=A0 . =C2=A0 =C2=A0 . [2] spin_unlock(l2); =C2=A0 =C2=A0 . =C2=A0 =C2=A0 . =C2=A0 =C2=A0 spin_unlock_irq(l1); =C2=A0 =C2=A0 . Even if l2 is also taken in an interrupt handler. In fact, what we want is make sure that such an interrupt handler that may take l2, does not interrupt CPU in the [1]--[2] time frame... But that can't happen because interrupts are already disabled, so just using spin_lock() is ok, and should be done, as that's cheaper than spin_lock_irqsave(). Fact is, what really matters is whether or not a lock is taken both inside and outside of IRQ context. As a matter of fact, it is mostly the case that, if a lock is ever taken inside an interrupt handler, then spin_lock_irqsave() is what you want... but this does not justify coming up with a table like the one above and claiming it to be general. > Why deadlock may occur if we mix the spin_lock and > spin_lock_irq(save)? > If we mix the spin_lock and spin_lock_irq(save), and a group of CPUs > rendezvousing in an IPI handler, we will have deadlock. Because the > CPU A that takes spin_lock will wait for the rendezvousing condition > to be satisfied, while the CPU B that takes th spin_lock_irq(save) > will not enter into the rendezvousing condition (since it disable the > interrupt). Then, > CPU A waits for CPU B to handle the IPI to get out of the > rendezvousing condition (kind of synchrous point), which won't until > it gets the spin_lock. > CPU B waits for CPU A to release the spin_lock, which won't until it > get out of the rendezvousing condition; >=20 > Are my understanding and summary correct? >=20 Yes, this looks a decent explanation of the rendezvous-related spinlock problem... Although I prefer the wording that we do have already in check_lock() and in start_secondary(). :-D Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-9LiyNjTuntiwp0imyQx6 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iEYEABECAAYFAlbi7lEACgkQk4XaBE3IOsQguACeJ2M7s8Ec9fPrEEnvnppgmoLN aa0AoK4k4Wz/5i/x3XwdEORoMjInmr9Z =chmv -----END PGP SIGNATURE----- --=-9LiyNjTuntiwp0imyQx6-- --===============0918645880645842684== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwOi8vbGlzdHMueGVuLm9y Zy94ZW4tZGV2ZWwK --===============0918645880645842684==--