From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:50804) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T8Thw-0000JV-Hn for qemu-devel@nongnu.org; Mon, 03 Sep 2012 06:16:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T8Thv-0008Mr-7A for qemu-devel@nongnu.org; Mon, 03 Sep 2012 06:16:40 -0400 Received: from mx1.redhat.com ([209.132.183.28]:19426) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T8Thu-0008Ml-UT for qemu-devel@nongnu.org; Mon, 03 Sep 2012 06:16:39 -0400 Message-ID: <5044837C.2050801@redhat.com> Date: Mon, 03 Sep 2012 13:16:28 +0300 From: Avi Kivity MIME-Version: 1.0 References: <1345801763-24227-1-git-send-email-qemulist@gmail.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> <503F1172.70103@siemens.com> <5041CB73.4050800@redhat.com> <50446FD9.3040805@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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: liu ping fan Cc: Jan Kiszka , Liu Ping Fan , "qemu-devel@nongnu.org" , Anthony Liguori , Paolo Bonzini On 09/03/2012 01:06 PM, liu ping fan wrote: > On Mon, Sep 3, 2012 at 4:52 PM, Avi Kivity wrote: >> On 09/03/2012 10:44 AM, liu ping fan wrote: >>>>> >>>> >>>> If we make the refcount/lock internal to the region, we must remove the >>>> opaque, since the region won't protect it. >>>> >>>> Replacing the opaque with container_of(mr) doesn't help, since we can't >>>> refcount mr, only mr->impl. >>>> >>> I think you mean if using MemoryRegionImpl, then at this level, we had >>> better not touch the refcnt of container_of(mr) and should face the >>> mr->impl->refcnt. Right? >> >> I did not understand the second part, sorry. >> > My understanding of "Replacing the opaque with container_of(mr) > doesn't help, since we can't refcount mr, only > mr->impl." is that although Object_ref(container_of(mr)) can help us > to protect it from disappearing. But apparently it is not right place > to do it it in memory core. Do I catch you meaning? We cannot call container_of(mr) in the memory core, because the parameters to container_of() are not known. But that is not the main issue. Something we can do is make MemoryRegionOps::object() take a mr parameter instead of opaque. MemoryRegionOps::object() then uses container_of() to derive the object to ref. (works with MemoryRegionOps::ref()/MemoryRegionOps::unref() too; Jan, this decouples Object from MemoryRegion at the cost of extra boilerplate in client code, but it may be worthwhile as a temporary measure while we gain more experience with this) > >>>> We could externalize the refcounting and push it into device code. This >>>> means: >>>> >>>> memory_region_init_io(&s->mem, dev) >>>> >>>> ... >>>> >>>> object_ref(dev) >>>> memory_region_add_subregion(..., &dev->mr) >>>> >>>> ... >>>> >>>> memory_region_del_subregion(..., &dev->mr) // implied flush >>>> object_unref(dev) >>>> >>> I think "object_ref(dev)" just another style to push >>> MemoryRegionOps::object() to device level. And I think we can bypass >>> it. The caller (unplug, pci-reconfig ) of >>> memory_region_del_subregion() ensure the @dev is valid. >>> If the implied flush is implemented in synchronize, _del_subregion() >>> will guarantee no reader for @dev->mr and @dev exist any longer. >> >> The above code has a deadlock. memory_region_del_subregion() may be >> called under the device lock (since it may be the result of mmio to the >> device), and if the flush uses synchronized_rcu(), it will wait forever >> for the read-side critical section to complete. >> > But if _del_subregion() just wait for mr-X quiescent period, while > calling in mr-Y's read side critical section, then we can avoid > deadlock. I saw in pci-mapping, we delete mr-X in mr-Y read side. Look at cirrus-vga.c, there are many calls to the memory API there. While I doubt that we have one region disabling itself (or a subregion of itself), that's all code that would be run under the same device lock, and so would deadlock. -- error compiling committee.c: too many arguments to function