Re: [Qemu-devel] [PATCH 07/15] postcopy: Plumb pagesize down into place helpers

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Juan Quintela (quintela@redhat.com) wrote:
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > Now we deal with normal size pages and huge pages we need
> > to tell the place handlers the size we're dealing with
> > and make sure the temporary page is large enough.
> >
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> I understand the what you are trying here, but ....
> 
> Why do we always map/upmap with the bigger pagesize?  I would assume
> that this deppends on the block we are dealing with, no?

It's only a temporary, so it seems easier to map it once at the start
of the migration at the size we need and clean it up at the end, that
way we don't have to allocate/deallocate on each block.

> > @@ -321,7 +321,7 @@ int postcopy_ram_incoming_cleanup(MigrationIncomingState *mis)
> >      migrate_send_rp_shut(mis, qemu_file_get_error(mis->from_src_file) != 0);
> >  
> >      if (mis->postcopy_tmp_page) {
> > -        munmap(mis->postcopy_tmp_page, getpagesize());
> > +        munmap(mis->postcopy_tmp_page, mis->largest_page_size);
> >          mis->postcopy_tmp_page = NULL;
> >      }
> >      trace_postcopy_ram_incoming_cleanup_exit();
> 
> Here
> 
> >  void *postcopy_get_tmp_page(MigrationIncomingState *mis)
> >  {
> >      if (!mis->postcopy_tmp_page) {
> > -        mis->postcopy_tmp_page = mmap(NULL, getpagesize(),
> > +        mis->postcopy_tmp_page = mmap(NULL, mis->largest_page_size,
> >                               PROT_READ | PROT_WRITE, MAP_PRIVATE |
> >                               MAP_ANONYMOUS, -1, 0);
> >          if (mis->postcopy_tmp_page == MAP_FAILED) {
> 
> And here ...
> 
> >              if (all_zero) {
> >                  ret = postcopy_place_page_zero(mis,
> >                                                 host + TARGET_PAGE_SIZE -
> > -                                               qemu_host_page_size);
> > +                                               block->page_size,
> > +                                               block->page_size);
> >              } else {
> >                  ret = postcopy_place_page(mis, host + TARGET_PAGE_SIZE -
> > -                                               qemu_host_page_size,
> > -                                               place_source);
> > +                                               block->page_size,
> > +                                               place_source, block->page_size);
> >              }
> >          }
> >          if (!ret) {
> 
> creating a temp for
> 
> addr = host + TARGET_PAGE_SIZE - block->page_size;
> 
> would make things more readable IMHO.  I was missreading the - by a ,
> and didn't understand so many parameters O:-)

Done.

Dave

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK