From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54682) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YLxfP-0002WF-1X for qemu-devel@nongnu.org; Thu, 12 Feb 2015 12:35:07 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YLxfK-00024n-2H for qemu-devel@nongnu.org; Thu, 12 Feb 2015 12:35:06 -0500 Received: from mail-we0-x229.google.com ([2a00:1450:400c:c03::229]:58506) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YLxfJ-00023r-Sn for qemu-devel@nongnu.org; Thu, 12 Feb 2015 12:35:02 -0500 Received: by mail-we0-f169.google.com with SMTP id k48so11651325wev.0 for ; Thu, 12 Feb 2015 09:35:00 -0800 (PST) Sender: Paolo Bonzini Message-ID: <54DCE440.6000609@redhat.com> Date: Thu, 12 Feb 2015 18:34:56 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1423758091-26462-1-git-send-email-mjrosato@linux.vnet.ibm.com> In-Reply-To: <1423758091-26462-1-git-send-email-mjrosato@linux.vnet.ibm.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH] memory: Fix double unref of flatview List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Matthew Rosato , Christian Borntraeger , Fam Zheng , qemu-devel On 12/02/2015 17:21, Matthew Rosato wrote: > Since 374f2981d1 "memory: protect current_map by RCU", > address_space_update_topology unrefs the old_flatview twice, > once by call_rcu and once by direct call. This patch removes > the direct call in favor of the call_rcu. Fixes at least one > assertion failure seen in s390, where a ref count for a memory > region attempts to go negative during hot-unplug of guest memory. This is buggy: MemoryRegion *standby_ram = g_new(MemoryRegion, 1); memory_region_init_ram(standby_ram, NULL, id, this_subregion_size, &error_abort); ... object_unparent(OBJECT(mr)); g_free(mr); Memory might not be released after object_unparent (and will not be released if the RCU callbacks haven't run yet). Your patch worked around it because it never freed the FlatView, and thus the RCU callbacks never did anything on the memory regions. Instead, you have to do this: MemoryRegion *standby_ram = g_new(MemoryRegion, 1); Object *obj = OBJECT(standby_ram); memory_region_init_ram(standby_ram, NULL, id, this_subregion_size, &error_abort); OBJECT(mr)->free = g_free; ... object_unparent(OBJECT(mr)); and the freeing will happen once the reference count goes to zero. Paolo > > Signed-off-by: Matthew Rosato > --- > memory.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/memory.c b/memory.c > index 130152c..d08abe5 100644 > --- a/memory.c > +++ b/memory.c > @@ -755,7 +755,6 @@ static void address_space_update_topology(AddressSpace *as) > > /* Writes are protected by the BQL. */ > atomic_rcu_set(&as->current_map, new_view); > - call_rcu(old_view, flatview_unref, rcu); > > /* Note that all the old MemoryRegions are still alive up to this > * point. This relieves most MemoryListeners from the need to > @@ -763,7 +762,7 @@ static void address_space_update_topology(AddressSpace *as) > * outside the iothread mutex, in which case precise reference > * counting is necessary. > */ > - flatview_unref(old_view); > + call_rcu(old_view, flatview_unref, rcu); > > address_space_update_ioeventfds(as); > } >