Re: [Qemu-devel] [PATCH] XBZRLE: Fix qemu crash when resize the xbzrle cache during migration

[Gonglei (Arei)]

> -----Original Message-----
> From: Dr. David Alan Gilbert [mailto:address@hidden
> Sent: Wednesday, February 19, 2014 6:54 PM
> To: Gonglei (Arei)
> Cc: address@hidden; Peter Maydell; chenliang (T); Juan Quintela;
> Luonengjun; address@hidden; Huangweidong (Hardware)
> Subject: Re: [Qemu-devel] [PATCH] XBZRLE: Fix qemu crash when resize the
> xbzrle cache during migration
> 
> * Gonglei (Arei) (address@hidden) wrote:
> 
> Hi Arei,
> 
> > It is likely to crash qemu when resize the xbzrle cache
> > during migration. Because the xbzrle cache will be modified
> > by migration thread and resize thread.
> 
> Thanks - we was just thinking about this last night after
> we hit it.
> 
> I was thinking about doing it by just moving the resize
> into the ram save loop; but I think your lock looks about right.
> 
> > Test scene
> > step one: set the size of xbzrle cache 1GB.
> > step two: migrate vm which dirty memory continuously.
> > step three: set the size of xbzrle cache 0.125GB.
> >
> > Signed-off-by: ChenLiang <address@hidden >
> > Signed-off-by: Gonglei <address@hidden >
> > ---
> > arch_init.c                    |   42
> ++++++++++++++++++++++++++++++++++++---
> > include/migration/page_cache.h |   14 +++++++++++++
> > page_cache.c                   |   13 ++++++++++++
> > 3 files changed, 65 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch_init.c b/arch_init.c
> > index 80574a0..e2d2c72 100644
> > --- a/arch_init.c
> > +++ b/arch_init.c
> > @@ -164,26 +164,56 @@ static struct {
> >      uint8_t *encoded_buf;
> >      /* buffer for storing page content */
> >      uint8_t *current_buf;
> > -    /* Cache for XBZRLE */
> > +    /* Cache for XBZRLE, Protected by lock. */
> >      PageCache *cache;
> > +    QemuMutex lock;
> > } XBZRLE = {
> >      .encoded_buf = NULL,
> >      .current_buf = NULL,
> >      .cache = NULL,
> > };
> > +
> > /* buffer used for XBZRLE decoding */
> > static uint8_t *xbzrle_decoded_buf;
> >
> > +static void XBZRLE_cache_lock(void)
> > +{
> > +    qemu_mutex_lock(&XBZRLE.lock);
> > +}
> > +
> > +static void XBZRLE_cache_unlock(void)
> > +{
> > +    qemu_mutex_unlock(&XBZRLE.lock);
> > +}
> > +
> > +
> > int64_t xbzrle_cache_resize(int64_t new_size)
> > {
> > +    PageCache *new_cache, *old_cache;
> > +
> >      if (new_size < TARGET_PAGE_SIZE) {
> >          return -1;
> >      }
> >
> >      if (XBZRLE.cache != NULL) {
> > -        return cache_resize(XBZRLE.cache, new_size /
> TARGET_PAGE_SIZE) *
> > -            TARGET_PAGE_SIZE;
> > +        if (pow2floor(new_size) == cache_max_num_items(XBZRLE.cache)
> > +
> *TARGET_PAGE_SIZE) {
> > +            goto ret;
> > +        }
> > +        new_cache = cache_init(new_size / TARGET_PAGE_SIZE,
> > +
> cache_page_size(XBZRLE.cache));
> 
> I wonder whether it's safe to even check the size of the XBZRLE.cache here;
> it's a short race, but I think if you're unlucky and migration completes
> between the NULL check and here then it would break.
> Also 'migration_end' calls cache_fini, so it probably also needs to have
> the lock around it.

Thanks, I will think about that, and commit a new patch to handle the issue 
that you refer to.
> 
> > +        if (!new_cache) {
> > +            DPRINTF("Error creating cache\n");
> > +            return -1;
> > +        }
> > +        XBZRLE_cache_lock();
> > +        old_cache = XBZRLE.cache;
> > +        XBZRLE.cache = new_cache;
> > +        XBZRLE_cache_unlock();
> > +        cache_fini(old_cache);
> > +        goto ret;
> >      }
> > +ret:
> >      return pow2floor(new_size);
> > }
> >
> > @@ -522,6 +552,8 @@ static int ram_save_block(QEMUFile *f, bool
> last_stage)
> >              ret = ram_control_save_page(f, block->offset,
> >                                 offset, TARGET_PAGE_SIZE,
> &bytes_sent);
> >
> > +            XBZRLE_cache_lock();
> > +
> >              if (ret != RAM_SAVE_CONTROL_NOT_SUPP) {
> >                  if (ret != RAM_SAVE_CONTROL_DELAYED) {
> >                      if (bytes_sent > 0) {
> > @@ -553,6 +585,8 @@ static int ram_save_block(QEMUFile *f, bool
> last_stage)
> >                  acct_info.norm_pages++;
> >              }
> >
> > +            XBZRLE_cache_unlock();
> > +
> >              /* if page is unmodified, continue to the next */
> >              if (bytes_sent > 0) {
> >                  last_sent_block = block;
> > @@ -681,7 +715,7 @@ static int ram_save_setup(QEMUFile *f, void
> *opaque)
> >              XBZRLE.encoded_buf = NULL;
> >              return -1;
> >          }
> > -
> > +        qemu_mutex_init(&XBZRLE.lock);
> >          acct_clear();
> >      }
> >
> > diff --git a/include/migration/page_cache.h
> b/include/migration/page_cache.h
> > index d156f0d..6be0103 100644
> > --- a/include/migration/page_cache.h
> > +++ b/include/migration/page_cache.h
> > @@ -79,4 +79,18 @@ int cache_insert(PageCache *cache, uint64_t addr,
> uint8_t *pdata);
> >   */
> > int64_t cache_resize(PageCache *cache, int64_t num_pages);
> >
> > +/**
> > + * cache_max_num_items: return the cache max num items.
> > + *
> > + * @cache pointer to the PageCache struct
> > + */
> > +int64_t cache_max_num_items(PageCache *cache);
> > +
> > +/**
> > + * cache_page_size: return the cache page size
> > + *
> > + * @cache pointer to the PageCache struct
> > + */
> > +unsigned int cache_page_size(PageCache *cache);
> > +
> > #endif
> > diff --git a/page_cache.c b/page_cache.c
> > index 3ef6ee7..92e401c 100644
> > --- a/page_cache.c
> > +++ b/page_cache.c
> > @@ -234,3 +234,16 @@ int64_t cache_resize(PageCache *cache, int64_t
> new_num_pages)
> >
> >      return cache->max_num_items;
> > }
> > +
> > +int64_t cache_max_num_items(PageCache *cache)
> > +{
> > +    g_assert(cache);
> > +    return cache->max_num_items;
> > +}
> > +
> > +unsigned int cache_page_size(PageCache *cache)
> > +{
> > +    g_assert(cache);
> > +    return cache->page_size;
> > +}
> > +
> > --
> > 1.6.0.2
> >
> >
> > Best regards,
> > -Gonglei
> >
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK