Re: [PATCH v2 5/6] migration/postcopy: Handle RAMBlocks with a RamDiscardManager on the destination

[David Hildenbrand <david@redhat.com>]

>> +static int postcopy_request_page(MigrationIncomingState *mis, RAMBlock *rb,
>> +                                 ram_addr_t start, uint64_t haddr)
>> +{
>> +    /*
>> +     * Discarded pages (via RamDiscardManager) are never migrated. On unlikely
>> +     * access, place a zeropage, which will also set the relevant bits in the
>> +     * recv_bitmap accordingly, so we won't try placing a zeropage twice.
>> +     */
>> +    if (ramblock_page_is_discarded(rb, start)) {
>> +        bool received = ramblock_recv_bitmap_test_byte_offset(rb, start);
> 
> Will received be set for any case with the current code base?  As I thought
> virtio-mem forbids plug/unplug during the whole lifecycle of migration.

receive would only be set if you have two CPUs faulting on the same 
address at the same time and the first one already placed a zeropage on 
this code path (as the comment said, that will implicitly set it in the 
rceivedmask).

So, pretty unlikely to happen, but if the stars align ... :)

> 
>> +
>> +        return received ? 0 : postcopy_place_page_zero(mis, (void *)haddr, rb);
> 
> (now we can fill up pages in two threads.. but looks thread-safe)
> 
> Meanwhile if this is highly not wanted, maybe worth an error_report_once() so
> the admin could see something?


You mean, if postcopy_place_page_zero() fails?

[...]

>>   
>> +/*
>> + * Check if a page falls into a discarded range as managed by a
>> + * RamDiscardManager responsible for the mapped memory region of the RAMBlock.
>> + * Pages inside discarded ranges are never migrated and postcopy has to
>> + * place zeropages instead.
>> + *
>> + * Note: The result is only stable while migration (precopy/postcopy).
>> + */
>> +bool ramblock_page_is_discarded(RAMBlock *rb, ram_addr_t offset)
>> +{
>> +    if (rb->mr && memory_region_has_ram_discard_manager(rb->mr)) {
>> +        RamDiscardManager *rdm = memory_region_get_ram_discard_manager(rb->mr);
>> +        MemoryRegionSection section = {
>> +            .mr = rb->mr,
>> +            .offset_within_region = offset,
>> +            .size = int128_get64(TARGET_PAGE_SIZE),
> 
> rb->page_size?
> 
> Although I think it should also work with TARGET_PAGE_SIZE in this specific
> case, but maybe still better to use what we have.


If rb->page_size is discarded, TARGET_PAGE_SIZE is certainly discarded 
as well (as TARGET_PAGE_SIZE <= rb->page_size).

But yes, sounds cleaner.

Thanks Peter!

-- 
Thanks,

David / dhildenb