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

[David Hildenbrand]

On 29.07.21 21:20, Peter Xu wrote:
> On Thu, Jul 29, 2021 at 06:15:58PM +0200, David Hildenbrand wrote:
> > On 29.07.21 17:52, Peter Xu wrote:
> > > On Thu, Jul 29, 2021 at 02:14:41PM +0200, David Hildenbrand wrote:
> > > > On 24.07.21 00:10, Peter Xu wrote:
> > > > > On Fri, Jul 23, 2021 at 09:01:42PM +0200, David Hildenbrand wrote:
> > > > > > It can happen in corner cases and is valid: with the current virtio-mem
> > > > > > spec, guests are allowed to read unplugged memory. This will, for example,
> > > > > > happen on older Linux guests when reading /proc/kcore or (with even older
> > > > > > guests) when dumping guest memory via kdump. These corner cases were the
> > > > > > main reason why the spec allows for it -- until we have guests properly
> > > > > > adjusted such that it won't happen even in corner cases.
> > > > > >
> > > > > > A future feature bit will disallow it for the guest: required for supporting
> > > > > > shmem/hugetlb cleanly. With that in place, I agree that we would want to
> > > > > > warn in this case!
> > > > >
> > > > > OK that makes sense; with the page_size change, feel free to add:
> > > >
> > > > I just realized that relying on the page_size would be wrong.
> > > >
> > > > We migrate TARGET_PAGE_SIZE chunks and the offset might not be page_size
> > > > aligned. So if we were to replace TARGET_PAGE_SIZE by rb->page_size, we
> > > > might accidentally cover a "too big" range.
> > >
> > > I'm wondering whether we should make the offset page size aligned instead.  For
> > > example, note that postcopy_place_page_zero() should only take page_size
> > > aligned host addr or UFFDIO_COPY could fail (hugetlb doesn't support
> > > UFFDIO_ZEROPAGE yet).
> >
> > That is true indeed. I'd assume in that case that we would get called with
> > the proper offset already, right? Because uffd would only report properly
> > aligned pages IIRC.
>
> Nop; it should return the faulted address. So postcopy_request_page() may need
> some alignment work, as it was handled in migrate_send_rp_req_pages().

Right, figured that out myself just now:

static int postcopy_request_page(MigrationIncomingState *mis, RAMBlock *rb,
                                 ram_addr_t start, uint64_t haddr)
{
    void *aligned = (void *)(uintptr_t)(haddr & -qemu_ram_pagesize(rb));

    /*
     * 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.
     *
     * Checking a single bit is sufficient to handle pagesize > TPS as either
     * all relevant bits are set or not.
     */
    assert(QEMU_IS_ALIGNED(start, qemu_ram_pagesize(rb)));
    if (ramblock_page_is_discarded(rb, start)) {
        bool received = ramblock_recv_bitmap_test_byte_offset(rb, start);

        return received ? 0 : postcopy_place_page_zero(mis, aligned, rb);
    }

    return migrate_send_rp_req_pages(mis, rb, start, haddr);
}


--
Thanks,

David / dhildenb