Re: [PATCH 1/1] migration: skip poisoned memory pages on "ram saving" phase

[Peter Xu]

On Mon, Sep 11, 2023 at 03:48:38PM -0400, Peter Xu wrote:
> On Sat, Sep 09, 2023 at 03:57:44PM +0100, Joao Martins wrote:
> > > Should I continue to treat them as zero pages written with
> > > save_zero_page_to_file ? 
> > 
> > MCE had already been forward to the guest, so guest is supposed to not be 
> > using
> > the page (nor rely on its contents). Hence destination ought to just see a 
> > zero
> > page. So what you said seems like the best course of action.
> > 
> > > Or should I consider the case of an ongoing compression
> > > use and create a new code compressing an empty page with 
> > > save_compress_page() ?
> > > 
> > The compress code looks to be a tentative compression (not guaranteed 
> > IIUC), so
> > I am not sure it needs any more logic that just adding at the top of
> > ram_save_target_page_legacy() as Peter suggested?
> > 
> > > And what about an RDMA memory region impacted by a memory error ?
> > > This is an important aspect.
> > > Does anyone know how this situation is dealt with ? And how it should be 
> > > handled
> > > in Qemu ?
> > > 
> > 
> > If you refer to guest RDMA MRs that is just guest RAM, not sure we are even
> > aware of those from qemu. But if you refer to the RDMA transport that sits 
> > below
> > the Qemu file (or rather acts as an implementation of QemuFile), so 
> > handling in
> > ram_save_target_page_legacy() already seems to cover it.
> 
> I'm also not familiar enough with RDMA, but it looks tricky indeed. AFAIU
> it's leveraging RDMA_CONTROL_COMPRESS for zero pages for now (with
> RDMACompress.value==0), so it doesn't seem to be using generic migration
> protocols.
> 
> If we want to fix all places well, one way to consider is to introduce
> migration_buffer_is_zero(), which can be a wrapper for buffer_is_zero() by
> default, but also returns true for poisoned pages before reading the
> buffer.  Then we use it in all three places:
> 
>   - For compression, in do_compress_ram_page()
>   - For RDMA, in qemu_rdma_write_one()

Ah, this may not be enough.. sorry.

It seems this is only one path that RDMA will use to save a target page,
for (!rdma->pin_all || !block->is_ram_block) && !block->remote_keys[chunk].

RDMA seems to also possible to merge buffers if virtually continuous
(qemu_rdma_buffer_mergable()), so IIUC it may not trigger an immediate
access to the guest page until later if it finds continuous pages and skip
even more logic.  I suspect that's also problematic for poisoned pages so
we should not allow any merged buffer to contain a poisoned page.

Not sure how complicated will it be to fix rdma specifically, copy again
two rdma developers.  One option is we state the issue in rdma and fix
non-rdma first.  Looks like rdma needs its own fix anyway.

>   - For generic migration, in save_zero_page_to_file() (your current patch)
> 
> I suppose then all cases will be fixed.  We need to make sure we'll always
> use migration_buffer_is_zero() as the 1st thing to call when QEMU wants to
> migrate a target page.  Maybe it'll worth a comment above that function.
> 
> Thanks,
> 
> -- 
> Peter Xu

-- 
Peter Xu