From mboxrd@z Thu Jan  1 00:00:00 1970
From: Anthony Liguori <anthony@codemonkey.ws>
Subject: Re: [PATCH 2/6] qemu-kvm: Modify and introduce wrapper functions
 to access phys_ram_dirty.
Date: Tue, 16 Mar 2010 08:35:00 -0500
Message-ID: <4B9F8904.2040704@codemonkey.ws>
References: <1268736839-27371-1-git-send-email-tamura.yoshiaki@lab.ntt.co.jp> <1268736839-27371-3-git-send-email-tamura.yoshiaki@lab.ntt.co.jp> <4B9F7D78.5090201@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Cc: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>,
	kvm@vger.kernel.org, qemu-devel@nongnu.org,
	ohmura.kei@lab.ntt.co.jp
To: Avi Kivity <avi@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mail-pw0-f46.google.com ([209.85.160.46]:53217 "EHLO
	mail-pw0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1754674Ab0CPNfF (ORCPT <rfc822;kvm@vger.kernel.org>);
	Tue, 16 Mar 2010 09:35:05 -0400
Received: by pwi1 with SMTP id 1so2595032pwi.19
        for <kvm@vger.kernel.org>; Tue, 16 Mar 2010 06:35:04 -0700 (PDT)
In-Reply-To: <4B9F7D78.5090201@redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On 03/16/2010 07:45 AM, Avi Kivity wrote:
> On 03/16/2010 12:53 PM, Yoshiaki Tamura wrote:
>> Modifies wrapper functions for byte-based phys_ram_dirty bitmap to
>> bit-based phys_ram_dirty bitmap, and adds more wrapper functions to 
>> prevent
>> direct access to the phys_ram_dirty bitmap.
>
>> +
>> +static inline int cpu_physical_memory_get_dirty_flags(ram_addr_t addr)
>> +{
>> +    unsigned long mask;
>> +    int index = (addr>>  TARGET_PAGE_BITS) / HOST_LONG_BITS;
>> +    int offset = (addr>>  TARGET_PAGE_BITS)&  (HOST_LONG_BITS - 1);
>> +    int ret = 0;
>> +
>> +    mask = 1UL<<  offset;
>> +    if (phys_ram_vga_dirty[index]&  mask)
>> +        ret |= VGA_DIRTY_FLAG;
>> +    if (phys_ram_code_dirty[index]&  mask)
>> +        ret |=  CODE_DIRTY_FLAG;
>> +    if (phys_ram_migration_dirty[index]&  mask)
>> +        ret |= MIGRATION_DIRTY_FLAG;
>> +
>> +    return ret;
>>   }
>>
>>   static inline int cpu_physical_memory_get_dirty(ram_addr_t addr,
>>                                                   int dirty_flags)
>>   {
>> -    return phys_ram_dirty[addr>>  TARGET_PAGE_BITS]&  dirty_flags;
>> +    return cpu_physical_memory_get_dirty_flags(addr)&  dirty_flags;
>>   }
>
> This turns one cacheline access into three.  If the dirty bitmaps were 
> in an array, you could do
>
>   return dirty_bitmaps[dirty_index][addr >> (TARGET_PAGE_BITS + 
> BITS_IN_LONG)] & mask;
>
> with one cacheline access.

As far as I can tell, we only ever call with a single flag so your 
suggestion makes sense.

I'd suggest introducing these functions before splitting the bitmap up.  
It makes review a bit easier.

>>
>>   static inline void cpu_physical_memory_set_dirty(ram_addr_t addr)
>>   {
>> -    phys_ram_dirty[addr>>  TARGET_PAGE_BITS] = 0xff;
>> +    unsigned long mask;
>> +    int index = (addr>>  TARGET_PAGE_BITS) / HOST_LONG_BITS;
>> +    int offset = (addr>>  TARGET_PAGE_BITS)&  (HOST_LONG_BITS - 1);
>> +
>> +    mask = 1UL<<  offset;
>> +    phys_ram_vga_dirty[index] |= mask;
>> +    phys_ram_code_dirty[index] |= mask;
>> +    phys_ram_migration_dirty[index] |= mask;
>> +}
>
> This is also three cacheline accesses.  I think we should have a 
> master bitmap which is updated by set_dirty(), and which is or'ed into 
> the other bitmaps when they are accessed.  At least the vga and 
> migration bitmaps are only read periodically, not randomly, so this 
> would be very fast.  In a way, this is similar to how the qemu bitmap 
> is updated from the kvm bitmap today.
>
> I am not sure about the code bitmap though.

I think your suggestion makes sense and would also work for the code bitmap.

Regards,

Anthony Liguori