From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:39363) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T60qM-0000tP-RP for qemu-devel@nongnu.org; Mon, 27 Aug 2012 11:03:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T60qF-0006OG-Ho for qemu-devel@nongnu.org; Mon, 27 Aug 2012 11:03:10 -0400 Received: from goliath.siemens.de ([192.35.17.28]:17894) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T60qF-0006Ng-8A for qemu-devel@nongnu.org; Mon, 27 Aug 2012 11:03:03 -0400 Message-ID: <503B8C1E.6090800@siemens.com> Date: Mon, 27 Aug 2012 17:02:54 +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> <874nnoh3pg.fsf@codemonkey.ws> In-Reply-To: <874nnoh3pg.fsf@codemonkey.ws> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 8bit 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: Anthony Liguori Cc: Paolo Bonzini , Avi Kivity , Liu Ping Fan , Liu Ping Fan , qemu-devel@nongnu.org On 2012-08-27 15:19, Anthony Liguori wrote: > Liu Ping Fan writes: > >> From: Liu Ping Fan >> >> Scene: >> obja lies in objA, when objA's ref->0, it will be freed, >> but at that time obja can still be in use. >> >> The real example is: >> typedef struct PCIIDEState { >> PCIDevice dev; >> IDEBus bus[2]; --> create in place >> ..... >> } >> >> When without big lock protection for mmio-dispatch, we will hold >> obj's refcnt. So memory_region_init_io() will replace the third para >> "void *opaque" with "Object *obj". >> With this patch, we can protect PCIIDEState from disappearing during >> mmio-dispatch hold the IDEBus->ref. >> >> And the ref circle has been broken when calling qdev_delete_subtree(). >> >> Signed-off-by: Liu Ping Fan > > I think this is solving the wrong problem. There are many, many > dependencies a device may have on other devices. Memory allocation > isn't the only one. > > The problem is that we want to make sure that a device doesn't "go away" > while an MMIO dispatch is happening. This is easy to solve without > touching referencing counting. > > The device will hold a lock while the MMIO is being dispatched. The > delete path simply needs to acquire that same lock. This will ensure > that a delete operation cannot finish while MMIO is still in flight. That's a bit too simple. Quite a few MMIO/PIO fast-paths will work without any device-specific locking, e.g. just to read a simple register value. So we will need reference counting (for devices using private locks), but on the "front-line" object: the memory region. That region will block its owner from disappearing by waiting on dispatch when someone tries to unregister it. Also note that "holding a lock" is easily said but will be more tricky in practice. Quite a significant share of our code will continue to run under BQL, even for devices with their own locks. Init/cleanup functions will likely fall into this category, simply because the surrounding logic is hard to convert into fine-grained locking and is also not performance critical. At the same time, we can't take BQL -> device-lock as we have to support device-lock -> BQL ordering for (slow-path) calls into BQL-protected areas while holding a per-device lock (e.g. device mapping changes). Jan -- Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux