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

[Juan Quintela <quintela@redhat.com>]

"Gonglei (Arei)" <arei.gonglei@huawei.com> wrote:
> Resizing the xbzrle cache during migration causes qemu-crash,
> because the main-thread and migration-thread modify the xbzrle
> cache size concurrently without lock-protection.
>
> Signed-off-by: ChenLiang <chenliang88@huawei.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Sorry for the late review.

> ---
> Changes against the previous version:
>  *Remove the temporary variable cache in the xbzrle_cache_resize.
>
>  arch_init.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 55 insertions(+), 3 deletions(-)
>
> diff --git a/arch_init.c b/arch_init.c
> index 80574a0..003640f 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -164,26 +164,69 @@ 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,
> +    /* use the lock carefully */
> +    .lock = {PTHREAD_MUTEX_INITIALIZER},
>  };

Have you tried to compile for windows?  I think this would make it
break.  We need to init it with qemu_mutex_init() in ram_save_setup()
after the call to cache_init()?



>  /* buffer used for XBZRLE decoding */
>  static uint8_t *xbzrle_decoded_buf;
>  
> +static void XBZRLE_cache_lock(void)
> +{
> +    qemu_mutex_lock(&XBZRLE.lock);
> +}
> +

if we change this to:

    if (migrate_use_xbzrle())
        qemu_mutex_lock(&XBZRLE.lock);

On both cache operations, we would remove the overhead in the no XBRLE
case, right?

> +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;
>      }
>  
> +    XBZRLE_cache_lock();
>      if (XBZRLE.cache != NULL) {
> -        return cache_resize(XBZRLE.cache, new_size / TARGET_PAGE_SIZE) *
> -            TARGET_PAGE_SIZE;
> +        /* check XBZRLE.cache again later */
> +        XBZRLE_cache_unlock();
> +        if (pow2floor(new_size) == migrate_xbzrle_cache_size()) {
> +            return pow2floor(new_size);
> +        }
> +        new_cache = cache_init(new_size / TARGET_PAGE_SIZE,
> +                                        TARGET_PAGE_SIZE);
> +        if (!new_cache) {
> +            DPRINTF("Error creating cache\n");
> +            return -1;
> +        }
> +
> +        XBZRLE_cache_lock();
> +        /* the XBZRLE.cache may have be destroyed, check it again */
> +        if (XBZRLE.cache != NULL) {
> +            old_cache = XBZRLE.cache;
> +            XBZRLE.cache = new_cache;
> +            new_cache = NULL;
> +        }
> +        XBZRLE_cache_unlock();
> +
> +        if (NULL == new_cache) {
> +            cache_fini(old_cache);
> +        } else {
> +            cache_fini(new_cache);
> +        }
> +    } else {
> +        XBZRLE_cache_unlock();
>      }

Can we change this to:

        if (pow2floor(new_size) == migrate_xbzrle_cache_size()) {
            return pow2floor(new_size);
        }

        new_cache = cache_init(new_size / TARGET_PAGE_SIZE,
                                        TARGET_PAGE_SIZE);
        if (!new_cache) {
            DPRINTF("Error creating cache\n");
            return -1;
        }

        XBZRLE_cache_lock();
        /* the XBZRLE.cache may have be destroyed, check it again */
        if (XBZRLE.cache != NULL) {
            cache_to_free = XBZRLE.cache;
            XBZRLE.cache = new_cache;
        } else {
            cache_to_free = new_cache;
        }
        XBZRLE_cache_unlock();

        cache_fini(cache_to_free);

I think that looking is clearer this way.  BTW, we are losing
_everything_ that is on the cache if we resize it.  I don't know if that
is what we want.  If we have a big cache, and make it bigger because we
are having too many misses, just dropping the whole cache looks like a
bad idea :-(


And my understanding is that we should do a:

migrate_get_current()->xbzrle_cache_size = new_size;

independently of what CACHE is equal or different from NULL?
(this was already wrong before your patch, just asking because you are
looking at the code).

Thanks, Juan.