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

From: Peter Xu <peterx@redhat.com>
To: David Hildenbrand <david@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>,
	Juan Quintela <quintela@redhat.com>,
	Pankaj Gupta <pankaj.gupta@cloud.ionos.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	teawater <teawaterz@linux.alibaba.com>,
	qemu-devel@nongnu.org,
	"Dr. David Alan Gilbert" <dgilbert@redhat.com>,
	Alex Williamson <alex.williamson@redhat.com>,
	Marek Kedzierski <mkedzier@redhat.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Andrey Gruzdev <andrey.gruzdev@virtuozzo.com>,
	Wei Yang <richard.weiyang@linux.alibaba.com>
Subject: Re: [PATCH v2 5/6] migration/postcopy: Handle RAMBlocks with a RamDiscardManager on the destination
Date: Thu, 29 Jul 2021 15:20:47 -0400	[thread overview]
Message-ID: <YQL/jxRizBc0OFZS@t490s> (raw)
In-Reply-To: <5ede7b00-1048-c124-e239-eeff21d9adb0@redhat.com>

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().

> 
> > 
> > Btw, does virtio-mem supports hugetlbfs now?  When with it, the smallest unit
> > to plug/unplug would the huge page size (e.g., for 1g huge page, sounds not
> > helpful to unplug 2M memory), am I right?
> 
> It supports it to 99.9 % I'd say (especially with the dump/tpm/migration
> fixes upstream). The units are handled properly: the unit is at least as big
> as the page size.
> 
> So with 1 GiB pages, you have a unit of 1 GiB.

I see, thanks for confirming.  Then fixing up the offset seems to be the right
thing to do.

Thanks,

-- 
Peter Xu