Re: [Qemu-devel] [PATCH] Fix two XBZRLE corruption issues

[Dr. David Alan Gilbert]

* Juan Quintela (address@hidden) wrote:
> "Dr. David Alan Gilbert (git)" <address@hidden> wrote:
> > From: "Dr. David Alan Gilbert" <address@hidden>
> >
> > Push zero'd pages into the XBZRLE cache
> >     A page that was cached by XBZRLE, zero'd and then XBZRLE'd again
> >     was being compared against a stale cache value
> >
> > Don't use 'qemu_put_buffer_async' to put pages from the XBZRLE cache
> >     Since the cache might change before the data hits the wire
> >
> > Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> > ---
> >  arch_init.c                    | 64 
> > ++++++++++++++++++++++++++++++++----------
> >  include/migration/page_cache.h |  2 +-
> >  page_cache.c                   |  2 +-
> >  3 files changed, 51 insertions(+), 17 deletions(-)
> >
> > diff --git a/arch_init.c b/arch_init.c
> > index 80574a0..fe17279 100644
> > --- a/arch_init.c
> > +++ b/arch_init.c
> > @@ -122,7 +122,6 @@ static void check_guest_throttling(void);
> >  #define RAM_SAVE_FLAG_XBZRLE   0x40
> >  /* 0x80 is reserved in migration.h start with 0x100 next */
> >  
> > -
> >  static struct defconfig_file {
> >      const char *filename;
> >      /* Indicates it is an user config file (disabled by -no-user-config) */
> > @@ -133,6 +132,7 @@ static struct defconfig_file {
> >      { NULL }, /* end of list */
> >  };
> >  
> > +static const uint8_t ZERO_TARGET_PAGE[TARGET_PAGE_SIZE];
> >  
> >  int qemu_read_default_config_files(bool userconfig)
> >  {
> > @@ -273,6 +273,34 @@ static size_t save_block_hdr(QEMUFile *f, RAMBlock 
> > *block, ram_addr_t offset,
> >      return size;
> >  }
> >  
> > +/* This is the last block that we have visited serching for dirty pages
> > + */
> > +static RAMBlock *last_seen_block;
> > +/* This is the last block from where we have sent data */
> > +static RAMBlock *last_sent_block;
> > +static ram_addr_t last_offset;
> > +static unsigned long *migration_bitmap;
> > +static uint64_t migration_dirty_pages;
> > +static uint32_t last_version;
> > +static bool ram_bulk_stage;
> > +
> > +/* Update the xbzrle cache to reflect a page that's been sent as all 0.
> > + * The important thing is that a stale (not-yet-0'd) page be replaced
> > + * by the new data.
> > + * As a bonus, if the page wasn't in the cache it gets added so that
> > + * when a small write is made into the 0'd page it gets XBZRLE sent
> > + */
> > +static void xbzrle_cache_zero_page(ram_addr_t current_addr)
> > +{
> > +    if (ram_bulk_stage || !migrate_use_xbzrle()) {
> > +        return;
> > +    }
> > +
> > +    /* We don't care if this fails to allocate a new cache page
> > +     * as long as it updated an old one */
> > +    cache_insert(XBZRLE.cache, current_addr, ZERO_TARGET_PAGE);
> > +}
> > +
> 
> Can we use a cache_insert_zero() page to avoid the copy?
> 
> Right now, we are inserting a zero page on the cache

Yes, I think a 'cache_insert_zero' would be a great idea;
I don't see one in the code base yet though, and I thought
it would be overkill to add it for a fix to a corruption bug.

> >  #define ENCODING_FLAG_XBZRLE 0x1
> >  
> >  static int save_xbzrle_page(QEMUFile *f, uint8_t *current_data,
> > @@ -329,18 +357,6 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t 
> > *current_data,
> >      return bytes_sent;
> >  }
> >  
> > -
> > -/* This is the last block that we have visited serching for dirty pages
> > - */
> > -static RAMBlock *last_seen_block;
> > -/* This is the last block from where we have sent data */
> > -static RAMBlock *last_sent_block;
> > -static ram_addr_t last_offset;
> > -static unsigned long *migration_bitmap;
> > -static uint64_t migration_dirty_pages;
> > -static uint32_t last_version;
> > -static bool ram_bulk_stage;
> > -
> >  static inline
> >  ram_addr_t migration_bitmap_find_and_reset_dirty(MemoryRegion *mr,
> >                                                   ram_addr_t start)
> > @@ -512,6 +528,7 @@ static int ram_save_block(QEMUFile *f, bool last_stage)
> >          } else {
> >              int ret;
> >              uint8_t *p;
> > +            bool send_async = true;
> >              int cont = (block == last_sent_block) ?
> >                  RAM_SAVE_FLAG_CONTINUE : 0;
> >  
> > @@ -522,6 +539,7 @@ static int ram_save_block(QEMUFile *f, bool last_stage)
> >              ret = ram_control_save_page(f, block->offset,
> >                                 offset, TARGET_PAGE_SIZE, &bytes_sent);
> >  
> > +            current_addr = block->offset + offset;
> >              if (ret != RAM_SAVE_CONTROL_NOT_SUPP) {
> >                  if (ret != RAM_SAVE_CONTROL_DELAYED) {
> >                      if (bytes_sent > 0) {
> > @@ -536,19 +554,35 @@ static int ram_save_block(QEMUFile *f, bool 
> > last_stage)
> >                                              RAM_SAVE_FLAG_COMPRESS);
> >                  qemu_put_byte(f, 0);
> >                  bytes_sent++;
> > +                /* Must let xbzrle know, otherwise a previous (now 0'd) 
> > cached
> > +                 * page would be stale
> > +                 */
> > +                xbzrle_cache_zero_page(current_addr);
> 
> Shouldn't it be easy to just create a:
> 
> cache_forget(current_addr)
> 
> function and call it a day?
> 
> I told that because if there is a different page on that slot on the
> cache, we are dropping it for no help?

Right, so I have two version of the code for xbzrle_cache_zero_page,
one that drops it, one that inserts the zero page - both work, but
I've not done any tests to see which has the best performance results.
Discussions with Orit came down to preferring this route, but I'm not
particularly tied to this way.

> >              } else if (!ram_bulk_stage && migrate_use_xbzrle()) {
> > -                current_addr = block->offset + offset;
> >                  bytes_sent = save_xbzrle_page(f, p, current_addr, block,
> >                                                offset, cont, last_stage);
> >                  if (!last_stage) {
> > +                    /* We must send exactly what's in the xbzrle cache
> > +                     * even if the page wasn't xbzrle compressed, so that
> > +                     * it's right next time.
> > +                     */
> 
> The problem here wasn't that we weren't forgotten it?  If we forgot it,
> data is not stalled to start with?

This comment was just explaining why we're doing the cache fetch at
this point, since it wasn't obvious to me when I read the code, and
relates to the conditional async send, and not the zeroing.

> And yes, I understand that if next change to the page is just writting a
> few bytes it is better to know that the page has been sent as all
> zeros.  But if we don't resent the page, we have just dropped one item
> in the cache for no good reason.
> 
> Ideas?

If you think that inserting the zero page would be particularly
bad, I'm happy to send the version that just forgets the old page;
although I think it would be good then to try and get some performance
results to validate which is best.

We should initially get something in that fixes it so it doesn't
send stuff that's corrupt.

> From now on, this is about xbzrle, not about previous page.
> 
> Further thought of the day: perhaps it is a good idea when sending a
> page to check if it is shorter from a zero page or from the previous
> page?
> 
> And once that we are looking into this:
> 
> In function save_xbzrle_page()
> 
>     } else if (encoded_len == -1) {
>         DPRINTF("Overflow\n");
>         acct_info.xbzrle_overflows++;
>         /* update data in the cache */
>         memcpy(prev_cached_page, current_data, TARGET_PAGE_SIZE);
>                                  ^^^^^^^^^^^^
> 
>         return -1;
>     }
> 
> Why aren't we using XBZRLE.current_buf there?

Hmm - I'm not sure it matters, we could do, the code at current_data
was copied into current_data a few lines before, so current_data could
be a bit newer if the CPU is still writing to it, and thus using the
latest version has the lowest chance of having to send an update later;
however I don't think it effects correctness since anything
later is compared to what we copy into cache at this point.

Dave

> 
> >                      p = get_cached_data(XBZRLE.cache, current_addr);
> > +
> > +                    /* Can't send this cached data async, since the cache 
> > page
> > +                     * might get updated before it gets to the wire
> > +                     */
> > +                    send_async = false;
> >                  }
> >              }
> >  
> >              /* XBZRLE overflow or normal page */
> >              if (bytes_sent == -1) {
> >                  bytes_sent = save_block_hdr(f, block, offset, cont, 
> > RAM_SAVE_FLAG_PAGE);
> > -                qemu_put_buffer_async(f, p, TARGET_PAGE_SIZE);
> > +                if (send_async) {
> > +                    qemu_put_buffer_async(f, p, TARGET_PAGE_SIZE);
> > +                } else {
> > +                    qemu_put_buffer(f, p, TARGET_PAGE_SIZE);
> > +                }
> >                  bytes_sent += TARGET_PAGE_SIZE;
> >                  acct_info.norm_pages++;
> >              }
> > diff --git a/include/migration/page_cache.h b/include/migration/page_cache.h
> > index d156f0d..2d5ce2d 100644
> > --- a/include/migration/page_cache.h
> > +++ b/include/migration/page_cache.h
> > @@ -66,7 +66,7 @@ uint8_t *get_cached_data(const PageCache *cache, uint64_t 
> > addr);
> >   * @addr: page address
> >   * @pdata: pointer to the page
> >   */
> > -int cache_insert(PageCache *cache, uint64_t addr, uint8_t *pdata);
> > +int cache_insert(PageCache *cache, uint64_t addr, const uint8_t *pdata);
> >  
> >  /**
> >   * cache_resize: resize the page cache. In case of size reduction the extra
> > diff --git a/page_cache.c b/page_cache.c
> > index 3ef6ee7..b033681 100644
> > --- a/page_cache.c
> > +++ b/page_cache.c
> > @@ -150,7 +150,7 @@ uint8_t *get_cached_data(const PageCache *cache, 
> > uint64_t addr)
> >      return cache_get_by_addr(cache, addr)->it_data;
> >  }
> >  
> > -int cache_insert(PageCache *cache, uint64_t addr, uint8_t *pdata)
> > +int cache_insert(PageCache *cache, uint64_t addr, const uint8_t *pdata)
> >  {
> >  
> >      CacheItem *it = NULL;
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK