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

[Dr. David Alan Gilbert]

* 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<mailto:address@hidden>>
> Signed-off-by: Gonglei <address@hidden<mailto: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.

> +        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