Re: [PATCH RFC 12/21] migration: Introduce page size for-migration-only

[Peter Xu <peterx@redhat.com>]

On Tue, Jan 24, 2023 at 04:36:20PM -0500, Peter Xu wrote:
> On Tue, Jan 24, 2023 at 01:20:37PM +0000, Dr. David Alan Gilbert wrote:
> > > @@ -3970,7 +3984,8 @@ int ram_load_postcopy(QEMUFile *f, int channel)
> > >                  break;
> > >              }
> > >              tmp_page->target_pages++;
> > > -            matches_target_page_size = block->page_size == TARGET_PAGE_SIZE;
> > > +            matches_target_page_size =
> > > +                migration_ram_pagesize(block) == TARGET_PAGE_SIZE;
> > >              /*
> > >               * Postcopy requires that we place whole host pages atomically;
> > >               * these may be huge pages for RAMBlocks that are backed by
> > 
> > Hmm do you really want this change?
> 
> Yes that's intended.  I want to reuse the same logic here when receiving
> small pages from huge pages, just like when we're receiving small pages on
> non-hugetlb mappings.
> 
> matches_target_page_size majorly affects two things:
> 
>   1) For a small zero page, whether we want to pre-set the page_buffer, or
>      simply use postcopy_place_page_zero():
>   
>         case RAM_SAVE_FLAG_ZERO:
>             ch = qemu_get_byte(f);
>             /*
>              * Can skip to set page_buffer when
>              * this is a zero page and (block->page_size == TARGET_PAGE_SIZE).
>              */
>             if (ch || !matches_target_page_size) {
>                 memset(page_buffer, ch, TARGET_PAGE_SIZE);
>             }
> 
>   2) For normal page, whether we need to use a page buffer or we can
>      directly reuse the page buffer in QEMUFile:
> 
>             if (!matches_target_page_size) {
>                 /* For huge pages, we always use temporary buffer */
>                 qemu_get_buffer(f, page_buffer, TARGET_PAGE_SIZE);
>             } else {
>                 /*
>                  * For small pages that matches target page size, we
>                  * avoid the qemu_file copy.  Instead we directly use
>                  * the buffer of QEMUFile to place the page.  Note: we
>                  * cannot do any QEMUFile operation before using that
>                  * buffer to make sure the buffer is valid when
>                  * placing the page.
>                  */
>                 qemu_get_buffer_in_place(f, (uint8_t **)&place_source,
>                                          TARGET_PAGE_SIZE);
>             }
> 
> Here:
> 
> I want 1) to reuse postcopy_place_page_zero().  For the doublemap case,
> it'll reuse postcopy_tmp_zero_page() (because qemu_ram_is_uf_zeroable()
> will return false for such a ramblock).
> 
> I want 2) to reuse qemu_get_buffer_in_place(), so we avoid a copy process
> for the small page which is faster (even if it's hugetlb backed, now we can
> reuse the qemufile buffer safely).

Since at it, one more thing worth mentioning is I didn't actually know
whether the original code is always correct when target and host small
psizes don't match..  This is the original line:

  matches_target_page_size = block->page_size == TARGET_PAGE_SIZE;

The problem is we're comparing block page size against target page size,
however block page size should be in host page size granule:

  RAMBlock *qemu_ram_alloc_internal()
  {
    new_block->page_size = qemu_real_host_page_size();

IOW, I am not sure whether postcopy will run at all in that case.  For
example, when we run an Alpha emulator upon x86_64, we can have target
psize 8K while host psize 4K.

The migration protocol should be TARGET_PAGE_SIZE based.  It means, for
postcopy when receiving a single page for Alpha VM being migrated, maybe we
should call UFFDIO_COPY (or UFFDIO_CONTINUE; doesn't matter here) twice
because one guest page contains two host pages.

I'm not sure whether I get all these right.. if so, we have two options:

  a) Forbid postcopy as a whole when detecting qemu_real_host_page_size()
     != TARGET_PAGE_SIZE.

  b) Implement postcopy for that case

I'd go with a) even if it's an issue because it means no one is migrating
that thing in postcopy way in the past N years, so it justifies that maybe
b) doesn't worth it.

-- 
Peter Xu