Re: [PATCH v2 0/6] migration/ram: Optimize for virtio-mem via RamDiscardManager

[David Hildenbrand <david@redhat.com>]

On 23.07.21 18:12, Peter Xu wrote:
> On Thu, Jul 22, 2021 at 01:43:41PM +0200, David Hildenbrand wrote:
>>>> a) In precopy code, always clearing all dirty bits from the bitmap that
>>>>      correspond to discarded range, whenever we update the dirty bitmap. This
>>>>      results in logically unplugged memory to never get migrated.
>>>
>>> Have you seen cases where discarded areas are being marked as dirty?
>>> That suggests something somewhere is writing to them and shouldn't be.
>>
>> I have due to sub-optimal clear_bmap handling to be sorted out by
>>
>> https://lkml.kernel.org/r/20210722083055.23352-1-wei.w.wang@intel.com
>>
>> Whereby the issue is rather that initially dirty bits don't get cleared in
>> lower layers and keep popping up as dirty.
>>
>> The issue with postcopy recovery code setting discarded ranges dirty in
>> the dirty bitmap, I did not try reproducing. But from looking at the
>> code, it's pretty clear that it would happen.
>>
>> Apart from that, nothing should dirty that memory. Of course,
>> malicious guests could trigger it for now, in which case we wouldn't catch it
>> and migrate such pages with postcopy, because the final bitmap sync in
>> ram_postcopy_send_discard_bitmap() is performed without calling notifiers
>> right now.
> 
> I have the same concern with Dave: does it mean that we don't need to touch at
> least ramblock_sync_dirty_bitmap in patch 3?

Yes, see the comment in patch #3:

"
Note: If discarded ranges span complete clear_bmap chunks, we'll never
clear the corresponding bits from clear_bmap and consequently never call
memory_region_clear_dirty_bitmap on the affected regions. While this is
perfectly fine, we're still synchronizing the bitmap of discarded ranges,
for example, in
ramblock_sync_dirty_bitmap()->cpu_physical_memory_sync_dirty_bitmap()
but also during memory_global_dirty_log_sync().

In the future, it might make sense to never even synchronize the dirty 
log of these ranges, for example in KVM code, skipping discarded ranges
completely.
"

The KVM path might be even more interesting (with !dirty ring IIRC).

So that might certainly be worth looking into if we find it to be a real 
performance problem.

> 
> Doing that for bitmap init and postcopy recovery looks right.
> 
> One other trivial comment is instead of touching up ram_dirty_bitmap_reload(),
> IMHO it's simpler to set all 1's to disgarded memories on dst receivedmap;
> imagine multiple postcopy recovery happened, then with that we walk the disgard
> memory list only once for each migration.  Not a big deal, though.

Right, but I decided to reuse 
ramblock_dirty_bitmap_exclude_discarded_pages() such that I can avoid 
yet another helper.

-- 
Thanks,

David / dhildenb