From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([140.186.70.92]:46285)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <owasserm@redhat.com>) id 1RiQtX-0005Xv-PA
	for qemu-devel@nongnu.org; Wed, 04 Jan 2012 08:28:49 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <owasserm@redhat.com>) id 1RiQtS-0002YC-4e
	for qemu-devel@nongnu.org; Wed, 04 Jan 2012 08:28:43 -0500
Received: from mx1.redhat.com ([209.132.183.28]:52169)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <owasserm@redhat.com>) id 1RiQtR-0002WX-Op
	for qemu-devel@nongnu.org; Wed, 04 Jan 2012 08:28:38 -0500
Message-ID: <4F0453D9.1070200@redhat.com>
Date: Wed, 04 Jan 2012 15:27:53 +0200
From: Orit Wasserman <owasserm@redhat.com>
MIME-Version: 1.0
References: <1325604879-15862-1-git-send-email-owasserm@redhat.com>
	<1325604879-15862-2-git-send-email-owasserm@redhat.com>
	<CAJSP0QV+j1-Kj274P+XDd7CKRnYJQc-vdgxKNBozNnqFmppT1Q@mail.gmail.com>
In-Reply-To: <CAJSP0QV+j1-Kj274P+XDd7CKRnYJQc-vdgxKNBozNnqFmppT1Q@mail.gmail.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH v5 1/9] Add cache handling functions
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Stefan Hajnoczi <stefanha@gmail.com>
Cc: blauwirbel@gmail.com, qemu-devel@nongnu.org, quintela@redhat.com

On 01/04/2012 01:46 PM, Stefan Hajnoczi wrote:
> On Tue, Jan 3, 2012 at 3:34 PM, Orit Wasserman <owasserm@redhat.com> wrote:
>> +static unsigned long cache_get_cache_pos(ram_addr_t address)
>> +{
>> +    unsigned long pos;
>> +
>> +    assert(cache_num_buckets);
>> +    pos = (address/TARGET_PAGE_SIZE) & (cache_num_buckets - 1);
> 
> Where do we guarantee that cache_num_buckets is a power of 2?

We should do it  to when setting cache size , I will add the check.
> 
>> +static void cache_insert(unsigned long addr, uint8_t *pdata)
>> +{
>> +    unsigned long pos;
>> +    int slot = -1;
>> +    CacheBucket *bucket;
>> +
>> +    pos = cache_get_cache_pos(addr);
>> +    assert(page_cache);
>> +    bucket = &page_cache[pos];
>> +    slot = cache_get_oldest(bucket); /* evict LRU */
>> +
>> +    /* actual update of entry */
>> +    CacheItem *it = cache_item_get(pos, slot);
>> +    if (!it->it_data) {
>> +        cache_num_items++;
>> +    }
>> +    g_free(it->it_data);
>> +
>> +    it->it_data = g_malloc0(TARGET_PAGE_SIZE);
>> +    memcpy(it->it_data, pdata, TARGET_PAGE_SIZE);
> 
> If we're evicting an entry:
> 1. Free existing data.
> 2. Allocate new data.
> 3. Zero new data.
> 4. Memcpy pdata into new data.
> 
> That does a bunch of extra work.  How about:
> 1. Memcpy pdata over old data.

I noticed it too , i will fix it in next version.
> 
> So:
> if (!it->it_data) {
>     cache_num_items++;
>     it->it_data = g_malloc(TARGET_PAGE_SIZE);
> }
> memcpy(it->it_data, pdata, TARGET_PAGE_SIZE);