Re: [Qemu-devel] [PATCH 1/2] Postcopy: Force allocation of all-zero precopy pages

[Dr. David Alan Gilbert]

* Juan Quintela (address@hidden) wrote:
> "Dr. David Alan Gilbert (git)" <address@hidden> wrote:
> > From: "Dr. David Alan Gilbert" <address@hidden>
> >
> > When an all-zero page is received during the precopy
> > phase of a postcopy-enabled migration we must force
> > allocation otherwise accesses to the page will still
> > get blocked by userfault.
> >
> > Symptom:
> >   a) If the page is accessed by a device during device-load
> >     then we get a deadlock as the source finishes sending
> >     all its pages but the destination device-load is still
> >     paused and so doesn't clean up.
> >
> >   b) If the page is accessed later, then the thread will stay
> >     paused until the end of migration rather than carrying on
> >     running, until we release userfault at the end.
> >
> > Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> > Reported-by: Christian Borntraeger <address@hidden>
> > ---
> >  include/migration/migration.h |  3 ++-
> >  migration/ram.c               | 12 ++++++++----
> >  migration/rdma.c              |  2 +-
> >  3 files changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/migration/migration.h b/include/migration/migration.h
> > index ba1a16cbc1..b47904033c 100644
> > --- a/include/migration/migration.h
> > +++ b/include/migration/migration.h
> > @@ -265,7 +265,8 @@ uint64_t xbzrle_mig_pages_overflow(void);
> >  uint64_t xbzrle_mig_pages_cache_miss(void);
> >  double xbzrle_mig_cache_miss_rate(void);
> >  
> > -void ram_handle_compressed(void *host, uint8_t ch, uint64_t size);
> > +void ram_handle_compressed(void *host, uint8_t ch, uint64_t size,
> > +                           bool always_write);
> >  void ram_debug_dump_bitmap(unsigned long *todump, bool expected);
> >  /* For outgoing discard bitmap */
> >  int ram_postcopy_send_discard_bitmap(MigrationState *ms);
> > diff --git a/migration/ram.c b/migration/ram.c
> > index f48664ec62..b4ed41c725 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -2274,10 +2274,12 @@ static inline void 
> > *host_from_ram_block_offset(RAMBlock *block,
> >   * @host: host address for the zero page
> >   * @ch: what the page is filled from.  We only support zero
> >   * @size: size of the zero page
> > + * @always_write: Always perform the memset even if it's zero
> >   */
> > -void ram_handle_compressed(void *host, uint8_t ch, uint64_t size)
> > +void ram_handle_compressed(void *host, uint8_t ch, uint64_t size,
> > +                           bool always_write)
> >  {
> > -    if (ch != 0 || !is_zero_range(host, size)) {
> > +    if (ch != 0 || always_write || !is_zero_range(host, size)) {
> >          memset(host, ch, size);
> >      }
> >  }
> > @@ -2514,7 +2516,8 @@ static int ram_load_postcopy(QEMUFile *f)
> >          switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
> >          case RAM_SAVE_FLAG_COMPRESS:
> >              ch = qemu_get_byte(f);
> > -            memset(page_buffer, ch, TARGET_PAGE_SIZE);
> > +            ram_handle_compressed(page_buffer, ch, TARGET_PAGE_SIZE,
> > +                                  true);
> 
> This is weird, if we are in postcopy, we were already doing the memset
> unconditionally, so .... weird as it can be.

No, we were already doing the memset once we had switched into postcopy mode,
this code does it while we're still in the precopy phase.

Dave

> I agree with Andrea that if the is_zero_range() don't assure that the
> page is there, it is really strange.
> 
> Can you look at utils/bufferiszero() and see what that function is doing
> in power?  File is a mess of ifdefs to optimize for intel as far as I
> can see.
> 
> Later, Juan.
> 
> 
> >              if (ch) {
> >                  all_zero = false;
> >              }
> > @@ -2664,7 +2667,8 @@ static int ram_load(QEMUFile *f, void *opaque, int 
> > version_id)
> >  
> >          case RAM_SAVE_FLAG_COMPRESS:
> >              ch = qemu_get_byte(f);
> > -            ram_handle_compressed(host, ch, TARGET_PAGE_SIZE);
> > +            ram_handle_compressed(host, ch, TARGET_PAGE_SIZE,
> > +                                  postcopy_advised);
> >              break;
> >  
> >          case RAM_SAVE_FLAG_PAGE:
> > diff --git a/migration/rdma.c b/migration/rdma.c
> > index fe0a4b5a83..07a9bd75d8 100644
> > --- a/migration/rdma.c
> > +++ b/migration/rdma.c
> > @@ -3164,7 +3164,7 @@ static int qemu_rdma_registration_handle(QEMUFile *f, 
> > void *opaque)
> >              host_addr = block->local_host_addr +
> >                              (comp->offset - block->offset);
> >  
> > -            ram_handle_compressed(host_addr, comp->value, comp->length);
> > +            ram_handle_compressed(host_addr, comp->value, comp->length, 
> > false);
> >              break;
> >  
> >          case RDMA_CONTROL_REGISTER_FINISHED:
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK