From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:33634) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T7jWC-0004fa-2v for qemu-devel@nongnu.org; Sat, 01 Sep 2012 04:57:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T7jWA-0004N2-Iu for qemu-devel@nongnu.org; Sat, 01 Sep 2012 04:57:28 -0400 Received: from mout.web.de ([212.227.17.12]:61374) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T7jWA-0004Mo-9Q for qemu-devel@nongnu.org; Sat, 01 Sep 2012 04:57:26 -0400 Message-ID: <5041CDEC.4070105@web.de> Date: Sat, 01 Sep 2012 10:57:16 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <1345801763-24227-1-git-send-email-qemulist@gmail.com> <1345801763-24227-11-git-send-email-qemulist@gmail.com> <503792F1.4090109@redhat.com> <503B1B4B.6050108@redhat.com> <503B260E.70607@web.de> <503BA9BC.5010207@redhat.com> <503BAAF0.2020103@siemens.com> <503BB7E7.4050709@redhat.com> <503BB9C5.3030605@siemens.com> <503BBA77.4090006@redhat.com> <503BBED4.9050705@siemens.com> <503BC1EE.4060608@redhat.com> <503BCCA1.10403@siemens.com> <503C9275.5040105@siemens.com> <503E4FF4.7060506@redhat.com> <503E51BB.9070308@siemens.com> <503E5422.5050805@redhat.com> <503E5637.4040701@siemens.com> <5041C7FE.5090306@redhat.com> In-Reply-To: <5041C7FE.5090306@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enig51F5D14114973689C81E2B5B" Subject: Re: [Qemu-devel] [PATCH 10/10] qdev: fix create in place obj's life cycle problem List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Avi Kivity Cc: Paolo Bonzini , Liu Ping Fan , liu ping fan , Anthony Liguori , "qemu-devel@nongnu.org" This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig51F5D14114973689C81E2B5B Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable On 2012-09-01 10:31, Avi Kivity wrote: > On 08/29/2012 10:49 AM, Jan Kiszka wrote: >>> >>> Let's experiment with refcounting MemoryRegion. We can move the enti= re >>> contents of MemoryRegion to MemoryRegionImpl, add a reference count (= to >>> MemoryRegionImpl), and change MemoryRegion to contain a pointer to th= e >>> refcounted MemoryRegionImpl: >>> >>> struct MemoryRegion { >>> struct MemoryRegionImpl *impl; >>> }; >>> >>> struct MemoryRegionImpl { >>> atomic int refs; >>> ... >>> }; >>> >>> The memory core can then store MemoryRegion copies (with elevated >>> refcounts) instead of pointers. Devices can destroy MemoryRegions at= >>> any time, the implementation will not go away. However, what of the >>> opaque stored in MemoryRegionImpl? It becomes a dangling pointer. >>> >>> One way out is to add a lock to MemoryRegionImpl. Dispatch takes the= >>> lock, examines the ->enabled member, and bails out if it is cleared. = >>> The (MemoryRegion, not MemoryRegionImpl) destructor also takes the lo= ck, >>> clears ->enabled, releases the lock, and drops the reference. >> >> That means holding the MemoryRegionImpl lock across the handler call? >=20 > Blech. As you said on the other side of this thread, we must not take = a > coarse grained lock within a fine grained one, and > MemoryRegionImpl::lock is as fine as they get. Not sure what you compare here. MemoryRegionImpl::lock would be per memory region, so with finer scope than the BQL but with similar scope like a per-device lock. >=20 >>> >>> The advantage to this scheme is that all changes are localized to the= >>> memory core, no need for a full sweep. It is a little complicated, b= ut >>> we may be able to simplify it (or find another one). >> >> May work. We just need to detect if memory region tries to delete itse= lf >> from its handler to prevent the deadlock. >=20 > Those types of hacks are fragile. IMO it just demonstrates what I said= > earlier (then tried to disprove with this): if we call an opaque's > method, we must refcount or otherwise lock that opaque. No way around = it. But that still doesn't solve the problem that we need to lock down the *state* of the opaque during dispatch /wrt to memory region changes. Just ensuring its existence is not enough unless we declare memory region transactions to be asynchronous - and adapt all users. >=20 >> >>> >>>> >>>>> >>>>>> Besides >>>>>> MMIO and PIO dispatching, it will haunt us for file or event handl= ers, >>>>>> any kind of callbacks etc. >>>>>> >>>>>> Context A Context B >>>>>> --------- --------- >>>>>> object =3D lookup() >>>>>> deregister(object) >>>>>> modify(object) -> invalid state >>>>>> ... use(object) >>>>>> modify(object) -> valid state >>>>>> register(object) >>>>>> >>>>>> And with "object" I'm not talking about QOM but any data structure= =2E >>>>>> >>>>> >>>>> >>>>> Context B >>>>> --------- >>>>> rcu_read_lock() >>>>> object =3D lookup() >>>>> if (object) { >>>>> ref(object) >>>>> } >>>>> rcu_read_unlock() >>>>> >>>>> use(object) >>>>> >>>>> unref(object) >>>>> >>>>> (substitute locking scheme to conform to taste and/or dietary >>>>> restrictions) =20 >>>>> >>>> >>>> + wait for refcount(object) =3D=3D 0 in deregister(object). That's w= hat I'm >>>> proposing. >>> >>> Consider timer_del() called from a timer callback. It's often not do= able. >> >> This is inherently synchronous already (when working against the same >> alarm timer backend). We can detect this in timer_del and skip waiting= , >> as in the above scenario. >=20 > It can always be broken. The timer callback takes the device lock to > update the device. The device mmio handler, holding the device lock, > takes the timer lock to timer_mod. Deadlock. Well, how is this solved in Linux? By waiting on the callback in hrtimer_cancel. Not by wait_on_magic_opaque (well, there is even no opaque in the hrtimer API). Jan --------------enig51F5D14114973689C81E2B5B Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.16 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://www.enigmail.net/ iEYEARECAAYFAlBBzfAACgkQitSsb3rl5xTQdQCdGAH/HwnvInKgneTzjJ1IjKDu LNcAnjPq/lojFmBeQnx+ihro/L6K1I3K =FyQW -----END PGP SIGNATURE----- --------------enig51F5D14114973689C81E2B5B--