Re: [PATCH 3/3] migration: mapped-ram: handle zero pages

[Peter Xu <peterx@redhat.com>]

On Thu, Oct 02, 2025 at 10:49:45AM +0200, Marco Cavenati wrote:
> Please note that there are a couple of errors (swapped parameters), which are
> detailed below.
> I will address these in the next iteration, along with any additional changes
> based on your feedback.
> 
> Thank you
> Marco
> 
> On Wednesday, October 01, 2025 18:18 CEST, Marco Cavenati <Marco.Cavenati@eurecom.fr> wrote:
> 
> > Make mapped-ram compatible with loadvm snapshot restoring by explicitly
> > zeroing memory pages in this case.
> > Skip zeroing for -incoming and -loadvm migrations to preserve performance.
> > 
> > Signed-off-by: Marco Cavenati <Marco.Cavenati@eurecom.fr>
> > ---
> >  migration/ram.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 55 insertions(+), 1 deletion(-)
> > 
> > diff --git a/migration/ram.c b/migration/ram.c
> > index e238c9233f..597d5ffe9e 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -3958,12 +3958,55 @@ static size_t ram_load_multifd_pages(void *host_addr, size_t size,
> >      return size;
> >  }
> >  
> > +/**
> > + * handle_zero_mapped_ram: Zero out a range of RAM pages if required during
> > + * mapped-ram load
> > + *
> > + * Zeroing is only performed when restoring from a snapshot (HMP loadvm).
> > + * During incoming migration or -loadvm cli snapshot load, the function is a
> > + * no-op and returns true as in those cases the pages are already guaranteed to
> > + * be zeroed.
> > + *
> > + * Returns: true on success, false on error (with @errp set).
> > + * @from_bit_idx: Starting index relative to the map of the page (inclusive)
> > + * @to_bit_idx:   Ending index relative to the map of the page (exclusive)
> > + */
> > +static bool handle_zero_mapped_ram(RAMBlock *block, unsigned long from_bit_idx,
> > +                                   unsigned long to_bit_idx, Error **errp)
> > +{
> > +    ERRP_GUARD();
> > +    ram_addr_t offset;
> > +    size_t size;
> > +    void *host;
> > +
> > +    if (runstate_check(RUN_STATE_INMIGRATE) ||
> > +        runstate_check(RUN_STATE_PRELAUNCH)) {

Should we check RUN_STATE_RESTORE_VM directly here?

I think it's still good to spell out the rest, we could put it in a
comment, e.g.:

  /*
   * Zeroing is not needed for either -loadvm (RUN_STATE_PRELAUNCH), or
   * -incoming (RUN_STATE_INMIGRATE).
   */

> > +        return true;
> > +    }
> > +
> > +    if (from_bit_idx == to_bit_idx) {

Might be safer to check >= rather than ==.

> > +        return true;
> > +    }
> > +
> > +    size = TARGET_PAGE_SIZE * (to_bit_idx - from_bit_idx);
> > +    offset = from_bit_idx << TARGET_PAGE_BITS;
> > +    host = host_from_ram_block_offset(block, offset);
> > +    if (!host) {
> > +        error_setg(errp, "zero page outside of ramblock %s range",
> > +                   block->idstr);
> > +        return false;
> > +    }
> > +    ram_handle_zero(host, size);
> > +
> > +    return true;
> > +}
> > +
> >  static bool read_ramblock_mapped_ram(QEMUFile *f, RAMBlock *block,
> >                                       long num_pages, unsigned long *bitmap,
> >                                       Error **errp)
> >  {
> >      ERRP_GUARD();
> > -    unsigned long set_bit_idx, clear_bit_idx;
> > +    unsigned long set_bit_idx, clear_bit_idx = 0;
> >      ram_addr_t offset;
> >      void *host;
> >      size_t read, unread, size;
> > @@ -3972,6 +4015,12 @@ static bool read_ramblock_mapped_ram(QEMUFile *f, RAMBlock *block,
> >           set_bit_idx < num_pages;
> >           set_bit_idx = find_next_bit(bitmap, num_pages, clear_bit_idx + 1)) {
> >  
> > +        /* Zero pages */
> > +        if (!handle_zero_mapped_ram(block, set_bit_idx, clear_bit_idx, errp)) {
> 
> This should be
> +         if (!handle_zero_mapped_ram(block, clear_bit_idx, set_bit_idx, errp)) {
> 
> > +            return false;
> > +        }
> > +
> > +        /* Non-zero pages */
> >          clear_bit_idx = find_next_zero_bit(bitmap, num_pages, set_bit_idx + 1);
> >  
> >          unread = TARGET_PAGE_SIZE * (clear_bit_idx - set_bit_idx);
> > @@ -4003,6 +4052,11 @@ static bool read_ramblock_mapped_ram(QEMUFile *f, RAMBlock *block,
> >          }
> >      }
> >  
> > +    /* Handle trailing 0 pages */
> > +    if (!handle_zero_mapped_ram(block, num_pages, clear_bit_idx, errp)) {
> 
> This should be
> +    if (!handle_zero_mapped_ram(block, clear_bit_idx, num_pages, errp)) {

The rest looks all good.

I can queue patch 2 now, which is trivial.  Please repost patch 1+3 after
rebasing to Fabiano's patch here:

https://lore.kernel.org/r/20251007184213.5990-1-farosas@suse.de

Then in patch 3 you can remove the MAPPED_RAM cap in the list.

Fabiano could also be posting some test patches too that he got for
snapshots.  You can either respin before that, or wait for it (then you can
also add a mapped-ram test for snapshots).

Thanks,

-- 
Peter Xu