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

[David Hildenbrand]

On 23.07.21 20:52, Peter Xu wrote:
> On Fri, Jul 23, 2021 at 08:36:32PM +0200, David Hildenbrand wrote:
> > > > +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).
>
> Ah I see; or just ignore the error of postcopy_place_page_zero() here because
> per my understanding this whole path is not expected after all.

See below, if placing would go wrong in this corner case, I think we 
would still want to know it instead of letting a guest thread not make 
progress because nobody would wake it up.

Does that make sense?

> > 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?
>
> I meant ramblock_page_is_discarded() shouldn't really trigger for a sane guest,
> right?  Because it means the guest is accessing some unplugged memory.

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!

--
Thanks,

David / dhildenb