[Qemu-devel] Re: [PATCH v2 3/6] Modifies wrapper functions for byte-based phys_ram_dirty bitmap to bit-based phys_ram_dirty bitmap.

[Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>]

Avi Kivity wrote:
> On 04/06/2010 03:51 AM, Yoshiaki Tamura wrote:
>> Signed-off-by: Yoshiaki Tamura<tamura.yoshiaki@lab.ntt.co.jp>
>> Signed-off-by: OHMURA Kei<ohmura.kei@lab.ntt.co.jp>
>> ---
>>
>> static inline int cpu_physical_memory_get_dirty_flags(ram_addr_t addr)
>> {
>> - return phys_ram_dirty[addr>> TARGET_PAGE_BITS];
>> + 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_dirty[MASTER_DIRTY_FLAG][index]& mask)
>> + return 0xff;
>> + if (phys_ram_dirty[VGA_DIRTY_FLAG][index]& mask)
>> + ret |= VGA_DIRTY_FLAG;
>> + if (phys_ram_dirty[CODE_DIRTY_FLAG][index]& mask)
>> + ret |= CODE_DIRTY_FLAG;
>> + if (phys_ram_dirty[MIGRATION_DIRTY_FLAG][index]& mask)
>> + ret |= MIGRATION_DIRTY_FLAG;
>> +
>> + return ret;
>> }
>
> Again, nicer as a loop.
> I think if you define both *_DIRTY_FLAG and *_DIRTY_IDX the transition
> patches can be nicer.

Got your suggestion from the other message.
I agree.

> Coding style: use braces after if (), even for single statements.

Thanks for pointing out.

>> 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;
>> + 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;
>> + return (phys_ram_dirty[MASTER_DIRTY_FLAG][index]& mask) ||
>> + (phys_ram_dirty[dirty_flags][index]& mask);
>> }
>
> A helper that also accepts the DIRTY_IDX index can increase reuse.

I would ffsl to map DIRTY_FLAG to DIRTY_IDX.

>> 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_dirty[MASTER_DIRTY_FLAG][index] |= mask;
>
>>
>> -static inline int cpu_physical_memory_set_dirty_flags(ram_addr_t addr,
>> - int dirty_flags)
>> +static inline void cpu_physical_memory_set_dirty_flags(ram_addr_t addr,
>> + int dirty_flags)
>> {
>> - return phys_ram_dirty[addr>> TARGET_PAGE_BITS] |= dirty_flags;
>> + 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;
>> + if (dirty_flags& VGA_DIRTY_FLAG)
>> + phys_ram_dirty[VGA_DIRTY_FLAG][index] |= mask;
>> + if (dirty_flags& CODE_DIRTY_FLAG)
>> + phys_ram_dirty[CODE_DIRTY_FLAG][index] |= mask;
>> + if (dirty_flags& MIGRATION_DIRTY_FLAG)
>> + phys_ram_dirty[MIGRATION_DIRTY_FLAG][index] |= mask;
>> }
>
> Is it necessary to update migration and vga bitmaps?
>
> We can simply update the master bitmap, and update the migration and vga
> bitmaps only when they need it. That can be done in a different patch.

Let me explain the role of the master bitmap here.

In the previous discussion, the master bitmap represented at least one dirty 
type is actually dirty.  While implementing this approach, I though there is one 
downside that upon resetting the bitmap, it needs to check all dirty types 
whether they are already cleared to unset the master bitmap, and this might hurt 
the performance in general.

In this patch, master bitmap represents all types are dirty, similar to existing 
0xff.  With this approach, resetting the master bitmap can be done without 
checking the other types.  set_dirty_flags is actually taking the burden in this 
case though.  Anyway, IIUC somebody would be unhappy depending on the role of 
the master bitmap.

> Note that we should only allocate the migration and vga bitmaps when
> migration or vga is active.

Right.  May I do it in a different patch?
I think this is about optimization.  In this patch, I focused on not breaking 
the existing function.