Re: [PATCH 2/6] qemu-kvm: Modify and introduce wrapper functions to access phys_ram_dirty.

[Anthony Liguori <anthony@codemonkey.ws>]

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