Re: [PATCH] migration: ram block cpr blockers

[Peter Xu]

On Wed, Jan 29, 2025 at 01:20:13PM -0500, Steven Sistare wrote:
> On 1/17/2025 6:57 PM, Peter Xu wrote:
> > On Fri, Jan 17, 2025 at 02:10:14PM -0500, Steven Sistare wrote:
> > > On 1/17/2025 1:16 PM, Peter Xu wrote:
> > > > On Fri, Jan 17, 2025 at 09:46:11AM -0800, Steve Sistare wrote:
> > > > > +/*
> > > > > + * Return true if ram contents would be lost during CPR.
> > > > > + * Return false for ram_device because it is remapped in new QEMU.  
> > > > > Do not
> > > > > + * exclude rom, even though it is readonly, because the rom file 
> > > > > could change
> > > > > + * in new QEMU.  Return false for non-migratable blocks.  They are 
> > > > > either
> > > > > + * re-created in new QEMU, or are handled specially, or are covered 
> > > > > by a
> > > > > + * device-level CPR blocker.  Return false for an fd, because it is 
> > > > > visible and
> > > > > + * can be remapped in new QEMU.
> > > > > + */
> > > > > +static bool ram_is_volatile(RAMBlock *rb)
> > > > > +{
> > > > > +    MemoryRegion *mr = rb->mr;
> > > > > +
> > > > > +    return mr &&
> > > > > +        memory_region_is_ram(mr) &&
> > > > > +        !memory_region_is_ram_device(mr) &&
> > > > > +        (!qemu_ram_is_shared(rb) || !qemu_ram_is_named_file(rb)) &&
> > > > > +        qemu_ram_is_migratable(rb) &&
> > > > > +        rb->fd < 0;
> > > > > +}
> > > > 
> > > > Blocking guest_memfd looks ok, but comparing to add one more block
> > > > notifier, can we check all ramblocks once in migrate_prepare(), and fail
> > > > that command directly if it fails the check?
> > > 
> > > In an upcoming patch, I will be adding an option analogous to 
> > > only-migratable which
> > > prevents QEMU from starting if anything would block cpr-transfer.  That 
> > > option
> > > will be checked when blockers are added, like for only-migratable. 
> > > migrate_prepare
> > > is too late.
> > > 
> > > > OTOH, is there any simpler way to simplify the check conditions?  It'll 
> > > > be
> > > > at least nice to break these checks into smaller if conditions for
> > > > readability..
> > > 
> > > I thought the function header comments made it clear, but I could move 
> > > each
> > > comment next to each condition:
> > > 
> > >      ...
> > >      /*
> > >       * Return false for an fd, because it is visible and can be remapped 
> > > in
> > >       * new QEMU.
> > >       */
> > >      if (rb->fd >= 0) {
> > >          return false;
> > >      }
> > >      ...
> > > 
> > > > I wonder if we could stick with looping over all ramblocks, then make 
> > > > sure
> > > > each of them is on the cpr saved fd list.  It may need to make
> > > > cpr_save_fd() always register with the name of ramblock to do such 
> > > > lookup,
> > > > or maybe we could also cache the ramblock pointer in CprFd, then the 
> > > > lookup
> > > > will be a pointer match check.
> > > 
> > > Some ramblocks are not on the list, such as named files.  Plus looping in
> > > migrate_prepare is too late as noted above.
> > > 
> > > IMO what I have already implemented using blockers is clean and elegant.
> > 
> > OK if we need to fail it early at boot, then yes blockers are probably
> > better.
> > 
> > We'll need one more cmdline parameter. I've no objection, but I don't know
> > how to judge when it's ok to add, when it's better not.. I'll leave others
> > to comment on this.
> > 
> > But still, could we check it when ramblocks are created?  So in that way
> > whatever is forbidden is clear in its own path, I feel like that could be
> > clearer (like what you did with gmemfd).
> 
> When the ramblock is created, we don't yet know if it is migratable. A
> ramblock that is not migratable does not block cpr. Migratable is not known
> until vmstate_register_ram calls qemu_ram_set_migratable.  Hence that is
> where I evaluate conditions and install a blocker.
> 
> Because that is the only place where ram_block_add_cpr_blocker is called,
> the test qemu_ram_is_migratable() inside ram_block_add_cpr_blocker is
> redundant, and I should delete it.

Hmm.. sounds reasonable.

> 
> > For example, if I start to convert some of your requirements above, then
> > memory_region_is_ram_device() implies RAM_PREALLOC.  Actually, ram_device
> > is not the only RAM_PREALLOC user..  Say, would it also not work with all
> > memory_region_init_ram_ptr() users (even if they're not ram_device)?  An
> > example is, looks like virtio-gpu can create random ramblocks on the fly
> > with prealloced buffers.  I am not sure whether they can be pinned by VFIO
> > too.  You may know better.
> 
> That memory is not visible to the guest.  It is not part of system_memory,
> and is not marked migratable.

I _think_ that can still be visible at least for the virtio-gpu use case,
which hangs under VirtIOGPUBase.hostmem.  Relevant code for reference:

virtio_gpu_virgl_map_resource_blob:
    memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size, data);
    memory_region_add_subregion(&b->hostmem, offset, mr);
    memory_region_set_enabled(mr, true);

virtio_gpu_pci_base_realize:
        memory_region_init(&g->hostmem, OBJECT(g), "virtio-gpu-hostmem",
                           g->conf.hostmem);
        pci_register_bar(&vpci_dev->pci_dev, 4,
                         PCI_BASE_ADDRESS_SPACE_MEMORY |
                         PCI_BASE_ADDRESS_MEM_PREFETCH |
                         PCI_BASE_ADDRESS_MEM_TYPE_64,
                         &g->hostmem);

pci_update_mappings:
            memory_region_add_subregion_overlap(r->address_space,
                                                r->addr, r->memory, 1);

but indeed I'm not sure how it work with migration so far, because it
doesn't have RAM_MIGRATABLE set. So at least cpr didn't make it more
special.  I assume this isn't something we must figure out as of now
then.. but if you do, please kindly share.

Thanks,

-- 
Peter Xu