Re: [PATCH] migration: Move bitmap_mutex out of migration_bitmap_clear_dirty()

[David Hildenbrand <david@redhat.com>]

On 13.07.21 10:40, Wang, Wei W wrote:
> On Thursday, July 1, 2021 4:08 AM, Peter Xu wrote:
>> Taking the mutex every time for each dirty bit to clear is too slow, especially we'll
>> take/release even if the dirty bit is cleared.  So far it's only used to sync with
>> special cases with qemu_guest_free_page_hint() against migration thread,
>> nothing really that serious yet.  Let's move the lock to be upper.
>>
>> There're two callers of migration_bitmap_clear_dirty().
>>
>> For migration, move it into ram_save_iterate().  With the help of MAX_WAIT
>> logic, we'll only run ram_save_iterate() for no more than 50ms-ish time, so taking
>> the lock once there at the entry.  It also means any call sites to
>> qemu_guest_free_page_hint() can be delayed; but it should be very rare, only
>> during migration, and I don't see a problem with it.
>>
>> For COLO, move it up to colo_flush_ram_cache().  I think COLO forgot to take
>> that lock even when calling ramblock_sync_dirty_bitmap(), where another
>> example is migration_bitmap_sync() who took it right.  So let the mutex cover
>> both the
>> ramblock_sync_dirty_bitmap() and migration_bitmap_clear_dirty() calls.
>>
>> It's even possible to drop the lock so we use atomic operations upon rb->bmap
>> and the variable migration_dirty_pages.  I didn't do it just to still be safe, also
>> not predictable whether the frequent atomic ops could bring overhead too e.g.
>> on huge vms when it happens very often.  When that really comes, we can
>> keep a local counter and periodically call atomic ops.  Keep it simple for now.
>>
>> Cc: Wei Wang <wei.w.wang@intel.com>
>> Cc: David Hildenbrand <david@redhat.com>
>> Cc: Hailiang Zhang <zhang.zhanghailiang@huawei.com>
>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> Cc: Juan Quintela <quintela@redhat.com>
>> Cc: Leonardo Bras Soares Passos <lsoaresp@redhat.com>
>> Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> FWIW
> Reviewed-by: Wei Wang <wei.w.wang@intel.com>
> 
> If no one could help do a regression test of free page hint, please document something like this in the patch:
> Locking at the coarser granularity is possible to minimize the improvement brought by free page hints, but seems not causing critical issues.
> We will let users of free page hints to report back any requirement and come up with a better solution later.

Can you send an official patch for the free page hinting clean_bmap 
handling I reported?

I can then give both tests in combination a quick test (before/after 
this patch here).

Cheers

-- 
Thanks,

David / dhildenb