| Message ID | 20200226155304.60219-10-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | migrate/ram: Fix resizing RAM blocks while migrating | expand |
* David Hildenbrand (david@redhat.com) wrote: > Let's consolidate resetting the variables. > > Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > Cc: Juan Quintela <quintela@redhat.com> > Cc: Peter Xu <peterx@redhat.com> > Signed-off-by: David Hildenbrand <david@redhat.com> Thanks, I think that's actually fixing a case where huge zero pages weren't placed as zero pages? Dave > --- > migration/ram.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/migration/ram.c b/migration/ram.c > index f815f4e532..1a5ff07997 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -3126,7 +3126,7 @@ static int ram_load_postcopy(QEMUFile *f) > /* Temporary page that is later 'placed' */ > void *postcopy_host_page = mis->postcopy_tmp_page; > void *host_page = NULL; > - bool all_zero = false; > + bool all_zero = true; > int target_pages = 0; > > while (!ret && !(flags & RAM_SAVE_FLAG_EOS)) { > @@ -3152,7 +3152,6 @@ static int ram_load_postcopy(QEMUFile *f) > addr &= TARGET_PAGE_MASK; > > trace_ram_load_postcopy_loop((uint64_t)addr, flags); > - place_needed = false; > if (flags & (RAM_SAVE_FLAG_ZERO | RAM_SAVE_FLAG_PAGE | > RAM_SAVE_FLAG_COMPRESS_PAGE)) { > block = ram_block_from_stream(f, flags); > @@ -3180,9 +3179,7 @@ static int ram_load_postcopy(QEMUFile *f) > */ > page_buffer = postcopy_host_page + > host_page_offset_from_ram_block_offset(block, addr); > - /* If all TP are zero then we can optimise the place */ > if (target_pages == 1) { > - all_zero = true; > host_page = host_page_from_ram_block_offset(block, addr); > } else if (host_page != host_page_from_ram_block_offset(block, > addr)) { > @@ -3199,7 +3196,6 @@ static int ram_load_postcopy(QEMUFile *f) > */ > if (target_pages == (block->page_size / TARGET_PAGE_SIZE)) { > place_needed = true; > - target_pages = 0; > } > place_source = postcopy_host_page; > } > @@ -3276,6 +3272,10 @@ static int ram_load_postcopy(QEMUFile *f) > ret = postcopy_place_page(mis, host_page, place_source, > block); > } > + place_needed = false; > + target_pages = 0; > + /* Assume we have a zero page until we detect something different */ > + all_zero = true; > } > } > > -- > 2.24.1 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 06.03.20 17:30, Dr. David Alan Gilbert wrote: > * David Hildenbrand (david@redhat.com) wrote: >> Let's consolidate resetting the variables. >> >> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com> >> Cc: Juan Quintela <quintela@redhat.com> >> Cc: Peter Xu <peterx@redhat.com> >> Signed-off-by: David Hildenbrand <david@redhat.com> > > Thanks, I think that's actually fixing a case where huge zero pages > weren't placed as zero pages? I don't see it :) Can you point out in which receive sequence it would go wrong? We used to set "all_zero = true" when processing the first sub-page. Now, we set "all_zero = true" before we start to process the first sub-page. Thanks!
* David Hildenbrand (david@redhat.com) wrote: > On 06.03.20 17:30, Dr. David Alan Gilbert wrote: > > * David Hildenbrand (david@redhat.com) wrote: > >> Let's consolidate resetting the variables. > >> > >> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > >> Cc: Juan Quintela <quintela@redhat.com> > >> Cc: Peter Xu <peterx@redhat.com> > >> Signed-off-by: David Hildenbrand <david@redhat.com> > > > > Thanks, I think that's actually fixing a case where huge zero pages > > weren't placed as zero pages? > > I don't see it :) Can you point out in which receive sequence it would > go wrong? > > We used to set "all_zero = true" when processing the first sub-page. > Now, we set "all_zero = true" before we start to process the first sub-page. No, you're right - no change. Dave > Thanks! > > -- > Thanks, > > David / dhildenb -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/migration/ram.c b/migration/ram.c index f815f4e532..1a5ff07997 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -3126,7 +3126,7 @@ static int ram_load_postcopy(QEMUFile *f) /* Temporary page that is later 'placed' */ void *postcopy_host_page = mis->postcopy_tmp_page; void *host_page = NULL; - bool all_zero = false; + bool all_zero = true; int target_pages = 0; while (!ret && !(flags & RAM_SAVE_FLAG_EOS)) { @@ -3152,7 +3152,6 @@ static int ram_load_postcopy(QEMUFile *f) addr &= TARGET_PAGE_MASK; trace_ram_load_postcopy_loop((uint64_t)addr, flags); - place_needed = false; if (flags & (RAM_SAVE_FLAG_ZERO | RAM_SAVE_FLAG_PAGE | RAM_SAVE_FLAG_COMPRESS_PAGE)) { block = ram_block_from_stream(f, flags); @@ -3180,9 +3179,7 @@ static int ram_load_postcopy(QEMUFile *f) */ page_buffer = postcopy_host_page + host_page_offset_from_ram_block_offset(block, addr); - /* If all TP are zero then we can optimise the place */ if (target_pages == 1) { - all_zero = true; host_page = host_page_from_ram_block_offset(block, addr); } else if (host_page != host_page_from_ram_block_offset(block, addr)) { @@ -3199,7 +3196,6 @@ static int ram_load_postcopy(QEMUFile *f) */ if (target_pages == (block->page_size / TARGET_PAGE_SIZE)) { place_needed = true; - target_pages = 0; } place_source = postcopy_host_page; } @@ -3276,6 +3272,10 @@ static int ram_load_postcopy(QEMUFile *f) ret = postcopy_place_page(mis, host_page, place_source, block); } + place_needed = false; + target_pages = 0; + /* Assume we have a zero page until we detect something different */ + all_zero = true; } }
Let's consolidate resetting the variables. Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com> Cc: Juan Quintela <quintela@redhat.com> Cc: Peter Xu <peterx@redhat.com> Signed-off-by: David Hildenbrand <david@redhat.com> --- migration/ram.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)