From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gregory Haskins Subject: Re: [KVM PATCH v5 3/4] KVM: Fix races in irqfd using new eventfd_kref_get interface Date: Sun, 28 Jun 2009 16:17:58 -0400 Message-ID: <4A47CFF6.6090100@novell.com> References: <20090625132441.26748.641.stgit@dev.haskins.net> <20090625132826.26748.15607.stgit@dev.haskins.net> <20090628114846.GA11764@redhat.com> <4A4767C2.3010503@novell.com> <20090628125612.GA11866@redhat.com> <20090628125730.GB11866@redhat.com> <20090628132035.GD11866@redhat.com> <4A479A23.1010804@novell.com> <20090628190710.GB14136@redhat.com> <4A47CA82.4040108@novell.com> <20090628200751.GA14993@redhat.com> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enigE14278C115A041B6D4FA507C" Cc: kvm@vger.kernel.org, avi@redhat.com To: "Michael S. Tsirkin" Return-path: Received: from victor.provo.novell.com ([137.65.250.26]:45283 "EHLO victor.provo.novell.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751901AbZF1USL (ORCPT ); Sun, 28 Jun 2009 16:18:11 -0400 In-Reply-To: <20090628200751.GA14993@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enigE14278C115A041B6D4FA507C Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Michael S. Tsirkin wrote: > On Sun, Jun 28, 2009 at 03:54:42PM -0400, Gregory Haskins wrote: > =20 >> Michael S. Tsirkin wrote: >> =20 >>> On Sun, Jun 28, 2009 at 12:28:19PM -0400, Gregory Haskins wrote: >>> =20 >>> =20 >>>> Michael S. Tsirkin wrote: >>>> =20 >>>> =20 >>>>> On Sun, Jun 28, 2009 at 03:57:30PM +0300, Michael S. Tsirkin wrote:= >>>>> =20 >>>>> =20 >>>>> =20 >>>>>> On Sun, Jun 28, 2009 at 03:56:12PM +0300, Michael S. Tsirkin wrote= : >>>>>> =20 >>>>>> =20 >>>>>> =20 >>>>>>> On Sun, Jun 28, 2009 at 08:53:22AM -0400, Gregory Haskins wrote: >>>>>>> =20 >>>>>>> =20 >>>>>>> =20 >>>>>>>> Michael S. Tsirkin wrote: >>>>>>>> =20 >>>>>>>> =20 >>>>>>>> =20 >>>>>>>>> On Thu, Jun 25, 2009 at 09:28:27AM -0400, Gregory Haskins wrote= : >>>>>>>>> =20 >>>>>>>>> =20 >>>>>>>>> =20 >>>>>>>>> =20 >>>>>>>>>> eventfd currently emits a POLLHUP wakeup on f_ops->release() t= o generate a >>>>>>>>>> "release" callback. This lets eventfd clients know if the eve= ntfd is about >>>>>>>>>> to go away and is very useful particularly for in-kernel clien= ts. However, >>>>>>>>>> until recently it is not possible to use this feature of event= fd in a >>>>>>>>>> race-free way. This patch utilizes a new eventfd interface to= rectify >>>>>>>>>> the problem. >>>>>>>>>> >>>>>>>>>> Note that one final race is known to exist: the slow-work thre= ad may race >>>>>>>>>> with module removal. We are currently working with slow-work = upstream >>>>>>>>>> to fix this issue as well. Since the code prior to this patch= also >>>>>>>>>> races with module_put(), we are not making anything worse, but= rather >>>>>>>>>> shifting the cause of the race. Once the slow-work code is pa= tched we >>>>>>>>>> will be fixing the last remaining issue. >>>>>>>>>> =20 >>>>>>>>>> =20 >>>>>>>>>> =20 >>>>>>>>>> =20 >>>>>>>>> By the way, why are we using slow-work here? Wouldn't a regular= >>>>>>>>> workqueue do just as well, with less code, and avoid the race? >>>>>>>>> >>>>>>>>> =20 >>>>>>>>> =20 >>>>>>>>> =20 >>>>>>>>> =20 >>>>>>>> I believe it will cause a problem if you do a "flush_work()" fro= m inside >>>>>>>> a work-item. I could be wrong, of course, but it looks like a r= ecipe to >>>>>>>> deadlock. >>>>>>>> >>>>>>>> -Greg >>>>>>>> >>>>>>>> =20 >>>>>>>> =20 >>>>>>>> =20 >>>>>>> Sure, but the idea is to only flush on kvm close, never from work= item. >>>>>>> =20 >>>>>>> =20 >>>>>>> =20 >>>>>> To clarify, you don't flush slow works from a work-item, >>>>>> so you shouldn't need to flush workqueue either. >>>>>> =20 >>>>>> =20 >>>>>> =20 >>>>> I guess my question is - why is slow work different? It's still >>>>> a thread pool underneath ... >>>>> >>>>> =20 >>>>> =20 >>>>> =20 >>>> Its not interdependent. Flush-work blocks the thread..if the thread= >>>> happens to be the work-queue thread you may deadlock preventing it f= rom >>>> processing further jobs like the inject. In reality it shouldnt be >>>> possible, but its just a bad idea to assume its ok. >>>> Slow work, on the >>>> other hand, will just make a new thread. >>>> >>>> -Greg >>>> >>>> =20 >>>> =20 >>> But if you create your own workqueue, and all you do there is destroy= >>> irqfds, things are ok I think. Right? >>> =20 >>> =20 >> Yep, creating your own queue works too. I picked slow-work as an >> alternate to generating a dedicated resource, but I agree either metho= d >> would work fine. Do you have a preference?=20 >> >> Regards, >> -Greg >> =20 > > It's not something I lose sleep about, but I think workqueue might be > less code: for example, you can just flush it instead of using your own= > counter. And possibly things can be further simplified by making the > workqueue single-threaded and always doing deassign from there. > > =20 Yeah, its not a huge deal. The logic is probably slightly simpler with a dedicated single-thread queue for shutdown, at the expense of having the work-queue hang around mostly idle. I'll code it up both ways so we can compare. Thanks Michael, -Greg --------------enigE14278C115A041B6D4FA507C 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 iEUEARECAAYFAkpHz/oACgkQlOSOBdgZUxkyugCXT5GIGgpWm8Mx5fa2vqMpBQ8v CQCggkeWVmLWtAvF57M9aCe6okbfiEQ= =ZW4c -----END PGP SIGNATURE----- --------------enigE14278C115A041B6D4FA507C--