From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46486) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YJMo4-0003q2-Ld for qemu-devel@nongnu.org; Thu, 05 Feb 2015 08:49:21 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YJMo1-0001Oz-F1 for qemu-devel@nongnu.org; Thu, 05 Feb 2015 08:49:20 -0500 Received: from mx1.redhat.com ([209.132.183.28]:47339) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YJMo1-0001Oo-6u for qemu-devel@nongnu.org; Thu, 05 Feb 2015 08:49:17 -0500 Message-ID: <54D374AE.2090500@redhat.com> Date: Thu, 05 Feb 2015 08:48:30 -0500 From: Max Reitz MIME-Version: 1.0 References: <1423140931-11823-1-git-send-email-berto@igalia.com> In-Reply-To: <1423140931-11823-1-git-send-email-berto@igalia.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] block: Give always priority to unused entries in the qcow2 L2 cache List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alberto Garcia , qemu-devel@nongnu.org Cc: Kevin Wolf , Stefan Hajnoczi On 2015-02-05 at 07:55, Alberto Garcia wrote: > The current algorithm to replace entries from the L2 cache gives > priority to newer hits by dividing the hit count of all existing > entries by two everytime there is a cache miss. > > However, if there are several cache misses the hit count of the > existing entries can easily go down to 0. This will result in those > entries being replaced even when there are others that have never been > used. > > This problem is more noticeable with larger disk images and cache > sizes, since the chances of having several misses before the cache is > full are higher. > > If we make sure that the hit count can never go down to 0 again, > unused entries will always have priority. > > Signed-off-by: Alberto Garcia > --- > block/qcow2-cache.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c > index 904f6b1..b115549 100644 > --- a/block/qcow2-cache.c > +++ b/block/qcow2-cache.c > @@ -253,7 +253,9 @@ static int qcow2_cache_find_entry_to_replace(Qcow2Cache *c) > > /* Give newer hits priority */ > /* TODO Check how to optimize the replacement strategy */ > - c->entries[i].cache_hits /= 2; > + if (c->entries[i].cache_hits > 1) { > + c->entries[i].cache_hits /= 2; > + } > } > > if (min_index == -1) { Hm, I can't see where the code is actually giving priority to unused entries. qcow2_cache_find_entry_to_replace() is the only place which selects the entry to be used, and it does not check entries[i].offset which it should in order to determine whether there are unused entries. Furthermore, I think we should prioritize clean entries over dirty ones; but I guess that's what the TODO comment is hinting at... Max