[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/12/2010 01:58 PM, Yoshiaki Tamura wrote:
>>> 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.
>
> The way I see it, the master bitmap is a buffer between the guest and
> all the other client bitmaps (migration and vga). So, a bit can be set
> in vga and cleared in master. Let's look at the following scenario:
>
> 1. guest touches address 0xa0000 (page 0xa0)
> 2. qemu updates master bitmap bit 0xa0
> 3. vga timer fires
> 4. vga syncs the dirty bitmap for addresses 0xa0000-0xc0000
> 4.1 vga_bitmap[0xa0-0xc0] |= master_bitmap[0xa0-0xc0]
> 4.2 migration_bitmap[0xa0-0xc0] |= master_bitmap[0xa0-0xc0]
> 4.3 master_bitmap[0xa0-0xc0] = 0
> 5. vga draws based on vga_bitmap
>
> the nice thing is that step 4.2 can be omitted if migration is not active.
>
> so, client bitmaps are always updated when the client requests them,
> master bitmap is only a buffer.

Thanks.  I think I'm finally getting your point.
At 4.2 and 4.3, is syncing to client necessary?
Simply doing OR at get_dirty like in this patch not enough?

>> 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.
>
> Yes, the problem is that set_dirty_flags is the common case and also
> uses random access, we'd want to make it touch only a single bit. client
> access is rare, and also sequential, and therefore efficient.

I see.
So set_dirty_flags would be like,

1. set master first regard less of dirty type.
2. if dirty type was CODE, set the client.

>>> 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?
>
> Sure, more patches is usually better.
>
>> I think this is about optimization. In this patch, I focused on not
>> breaking the existing function.
>
> Yes, that's the best approach. But we do need to see how all the
> incremental steps end up.

I totally agree.  Thanks for helping.

In summary, I'll prepare two patch sets.

1. v3 of this patch set.
- Change FLAGS value to (1,2,4,8), and add IDX (0,1,2,3)
- Use ffsl to convert FLAGS to IDX.
- Add help function which takes IDX.
- Change the behavior of set_dirty_flags as above.
- Change dirty bitmap access to a loop.
- Add brace after if ()
- Move some macros to qemu-common.h.

2. Allocate vga and migration dirty bitmap dynamically.

Please modify or add items if I were missing.