* Laurent Vivier (lvivier@redhat.com) wrote:
> On 06/02/2017 18:33, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > The existing postcopy RAM load loop already ensures that it
> > glues together whole host-pages from the target page size chunks sent
> > over the wire. Modify the definition of host page that it uses
> > to be the RAM block page size and thus be huge pages where appropriate.
> >
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Reviewed-by: Juan Quintela <quintela@redhat.com>
> > ---
> > migration/ram.c | 13 ++++++++-----
> > 1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/migration/ram.c b/migration/ram.c
> > index ff448ef..88d9444 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -2342,7 +2342,7 @@ static int ram_load_postcopy(QEMUFile *f)
> > {
> > int flags = 0, ret = 0;
> > bool place_needed = false;
> > - bool matching_page_sizes = qemu_host_page_size == TARGET_PAGE_SIZE;
> > + bool matching_page_sizes = false;
>
> The false value is not obvious.
> Is gcc smart enough to detect you use "matching_page_sizes" (in the
> "switch ()") only when it has been really initialized (in the "if ()")?
4.8.5-8 on RHEL 7 doesn't like it if I drop the false.
(That took a bit of searching, RHEL 6 is OK, f25 is OK)
But generally I've found these ram-load loops are really good
for tickling gcc's paranoia.
> > MigrationIncomingState *mis = migration_incoming_get_current();
> > /* Temporary page that is later 'placed' */
> > void *postcopy_host_page = postcopy_get_tmp_page(mis);
> > @@ -2372,8 +2372,11 @@ static int ram_load_postcopy(QEMUFile *f)
> > ret = -EINVAL;
> > break;
> > }
> > + matching_page_sizes = block->page_size == TARGET_PAGE_SIZE;
> > /*
> > - * Postcopy requires that we place whole host pages atomically.
> > + * Postcopy requires that we place whole host pages atomically;
> > + * these may be huge pages for RAMBlocks that are backed by
> > + * hugetlbfs.
> > * To make it atomic, the data is read into a temporary page
> > * that's moved into place later.
> > * The migration protocol uses, possibly smaller, target-pages
> > @@ -2381,9 +2384,9 @@ static int ram_load_postcopy(QEMUFile *f)
> > * of a host page in order.
> > */
> > page_buffer = postcopy_host_page +
> > - ((uintptr_t)host & ~qemu_host_page_mask);
> > + ((uintptr_t)host & (block->page_size - 1));
> > /* If all TP are zero then we can optimise the place */
> > - if (!((uintptr_t)host & ~qemu_host_page_mask)) {
> > + if (!((uintptr_t)host & (block->page_size - 1))) {
> > all_zero = true;
> > } else {
> > /* not the 1st TP within the HP */
> > @@ -2401,7 +2404,7 @@ static int ram_load_postcopy(QEMUFile *f)
> > * page
> > */
> > place_needed = (((uintptr_t)host + TARGET_PAGE_SIZE) &
> > - ~qemu_host_page_mask) == 0;
> > + (block->page_size - 1)) == 0;
> > place_source = postcopy_host_page;
> > }
> > last_host = host;
> >
>
> Reviewed-by: Laurent Vivier <lvivier@redhat.com>
Thanks.
Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK