From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40957) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XCrNX-0004mZ-3n for qemu-devel@nongnu.org; Thu, 31 Jul 2014 10:30:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XCrNQ-0006WR-Px for qemu-devel@nongnu.org; Thu, 31 Jul 2014 10:30:47 -0400 Received: from mx1.redhat.com ([209.132.183.28]:24252) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XCrNQ-0006WE-DQ for qemu-devel@nongnu.org; Thu, 31 Jul 2014 10:30:40 -0400 Message-ID: <53DA530C.3020802@redhat.com> Date: Thu, 31 Jul 2014 16:30:36 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1406716032-21795-1-git-send-email-pbonzini@redhat.com> <1406716032-21795-7-git-send-email-pbonzini@redhat.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 6/9] ioport: split deletion and destruction List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Crosthwaite Cc: "qemu-devel@nongnu.org Developers" Il 31/07/2014 14:34, Peter Crosthwaite ha scritto: > So I found the original implementation made sense to me, in that _del > is the converse of _add and _destroy _init WRT to the MR ops. > > Currently > _init = malloc array > _add = mr_init + mr_add_subregion > _del = mr_del_subregion + mr_destroy > _destory = free array > > This delays mr_destory to _destroy breaking the symmetry. Note that there is a fundamental difference between init and destroy: when you do init, the MemoryRegion should not be providing references to the MemoryRegion's owner. So you can do it pretty much whenever you want. However, after del_subregion the device models can have references to the MemoryRegion's owner (via address_space_map/unmap) and destroy has to be delayed to a point when these references have disappeared. This is why I'm keen on making memory_region_destroy happen automatically at finalize time, as I do later in this series: last year I had tried doing it manually with instance_finalize, and it was an unreviewable mess. With this patch, you still would need manual portio_list_destroy calls in instance_finalize, but PortioLists are used sparingly so it's much simpler to manage them. This difference is why in this patch I focused only on portio_list_del/destroy. However, I can try and move init to portio_list_add; then the symmetry is restored. > With this change is is still possible to: > > _init() > _add() > _del() > _add() > _del() > _destrory() > > And not leak a ref, with the reinited memory region on second add? > > Then again I may be misunderstanding, as apparently neither of these > API have any users so I'm having trouble getting a handle on intended > usage: Yes, that's because these patches are mostly used by ISA devices or VGAs, and currently neither is hot-unpluggable. Paolo