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

Anthony Liguori
2010-03-16T13:35:05Z

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