From patchwork Mon Feb 25 12:50:14 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Peter Lieven X-Patchwork-Id: 222916 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 48BF12C02C1 for ; Mon, 25 Feb 2013 23:50:40 +1100 (EST) Received: from localhost ([::1]:42511 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1U9xVu-0008DI-9u for incoming@patchwork.ozlabs.org; Mon, 25 Feb 2013 07:50:38 -0500 Received: from eggs.gnu.org ([208.118.235.92]:60256) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1U9xVg-00087U-UV for qemu-devel@nongnu.org; Mon, 25 Feb 2013 07:50:31 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1U9xVY-0000wN-Bj for qemu-devel@nongnu.org; Mon, 25 Feb 2013 07:50:24 -0500 Received: from ssl.dlhnet.de ([91.198.192.8]:53178 helo=ssl.dlh.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1U9xVY-0000w4-13 for qemu-devel@nongnu.org; Mon, 25 Feb 2013 07:50:16 -0500 Received: from localhost (localhost [127.0.0.1]) by ssl.dlh.net (Postfix) with ESMTP id 283E914DA90; Mon, 25 Feb 2013 13:50:15 +0100 (CET) X-Virus-Scanned: Debian amavisd-new at ssl.dlh.net Received: from ssl.dlh.net ([127.0.0.1]) by localhost (ssl.dlh.net [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id OEWvCci9kNZ3; Mon, 25 Feb 2013 13:50:14 +0100 (CET) Received: from lieven-mac.kamp-intra.net (unknown [82.141.1.199]) by ssl.dlh.net (Postfix) with ESMTPSA id BEE4914DA8F; Mon, 25 Feb 2013 13:50:14 +0100 (CET) Mime-Version: 1.0 (Mac OS X Mail 6.2 \(1499\)) From: Peter Lieven In-Reply-To: Date: Mon, 25 Feb 2013 13:50:14 +0100 Message-Id: <38D0DA1A-A001-4AD1-9F63-E4CEE4EC4610@dlhnet.de> References: <512B5096.7030901@dlhnet.de> <99BAF383-FBD3-4608-AE66-DF9D754F0C12@dlhnet.de> To: Peter Maydell X-Mailer: Apple Mail (2.1499) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6.x X-Received-From: 91.198.192.8 Cc: Orit Wasserman , "qemu-devel@nongnu.org" Subject: Re: [Qemu-devel] [PATCH] page_cache: dup memory on insert X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Am 25.02.2013 um 13:36 schrieb Peter Maydell : > On 25 February 2013 12:17, Peter Lieven wrote: >> Am 25.02.2013 um 13:13 schrieb Peter Maydell : >>> Doesn't this introduce a leak on cache resize in the case where >>> the element being moved from the old cache to the new does not >>> collide with any element we've already moved? [ie the code >>> path where we just cache_insert() the old item's data]. >> >> you are right. maybe we need different functions for inserts made >> during resize and inserts from outside. > > Looking a little more closely, I think you need that anyway, > because cache_insert() updates the it_age field, when I would > have expected that in the "just moving everything over to the > resized cache" case we would want to retain the existing age. > > Since cache_resize() already has code in it that does a > direct access and copy of CacheItem* fields [for the "copy > old_it over new_it" case] it might be cleaner to either > (a) have all the cases open-coded in cache_resize() rather > than calling cache_insert(), or (b) abstract out the > whole of the resize inner loop into something like > > cache_insert_item(PageCache *cache, CacheItem *olditem) > { > /* Insert olditem (an item from a different page cache) into this one. > * If there is a collision then we keep the data from whichever of > * olditem and the existing entry is newer. In either case, olditem's > * data pointer is either copied into the new cache or freed, so > * the caller must do nothing further with it. > */ > } > > Extra bonus leak: I think cache_resize() is leaking > cache->page_cache. sth like this? > > -- PMM diff --git a/page_cache.c b/page_cache.c index 376f1db..04205ee 100644 --- a/page_cache.c +++ b/page_cache.c @@ -196,21 +196,19 @@ int64_t cache_resize(PageCache *cache, int64_t new_num_pages) /* check for collision, if there is, keep MRU page */ new_it = cache_get_by_addr(new_cache, old_it->it_addr); if (new_it->it_data) { - /* keep the MRU page */ if (new_it->it_age >= old_it->it_age) { g_free(old_it->it_data); - } else { - g_free(new_it->it_data); - new_it->it_data = old_it->it_data; - new_it->it_age = old_it->it_age; - new_it->it_addr = old_it->it_addr; + continue; } - } else { - cache_insert(new_cache, old_it->it_addr, old_it->it_data); } + g_free(new_it->it_data); + new_it->it_data = old_it->it_data; + new_it->it_age = old_it->it_age; + new_it->it_addr = old_it->it_addr; } } + g_free(cache->page_cache); cache->page_cache = new_cache->page_cache; cache->max_num_items = new_cache->max_num_items; cache->num_items = new_cache->num_items;