[Qemu-devel] [PATCH v2 0/6] XBZRLE Fixes

[Qemu-devel] [PATCH v2 0/6] XBZRLE Fixes

[Orit Wasserman]

Fix memory leak and missing NULLs.
Better cache size validation checks.
Fail migration instead of aborting QEMU in case there is not
enough memory for the page cache.

Changes from v1:
Free encoded_buf in case of failure to allocate 
current_buf.
Update num_items after successful allocation.

Gonglei (Arei) (1):
  migration:fix free XBZRLE decoded_buf wrong

Orit Wasserman (5):
  Set xbzrle buffers to NULL after freeing them to avoid double free
    errors
  Add check for cache size smaller than page size
  XBZRLE cache size should not be larger than guest memory size
  Don't abort on out of memory when creating page cache
  Don't abort on memory allocation error

 arch_init.c                    | 49 +++++++++++++++++++++++++++++++-----------
 include/migration/migration.h  |  1 +
 include/migration/page_cache.h |  4 +++-
 migration.c                    | 18 +++++++++++++++-
 page_cache.c                   | 34 +++++++++++++++++++++--------
 5 files changed, 83 insertions(+), 23 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v2 1/6] Set xbzrle buffers to NULL after freeing them to avoid double free errors

[Orit Wasserman]

Signed-off-by: Orit Wasserman <owasserm@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
---
 arch_init.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch_init.c b/arch_init.c
index 77912e7..66f5e82 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -617,6 +617,9 @@ static void migration_end(void)
         g_free(XBZRLE.current_buf);
         g_free(XBZRLE.decoded_buf);
         XBZRLE.cache = NULL;
+        XBZRLE.encoded_buf = NULL;
+        XBZRLE.current_buf = NULL;
+        XBZRLE.decoded_buf = NULL;
     }
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 2/6] Add check for cache size smaller than page size

[Orit Wasserman]

Signed-off-by: Orit Wasserman <owasserm@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
---
 arch_init.c |  4 ++++
 migration.c | 10 +++++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/arch_init.c b/arch_init.c
index 66f5e82..8edeabe 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -178,6 +178,10 @@ static struct {
 
 int64_t xbzrle_cache_resize(int64_t new_size)
 {
+    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;
diff --git a/migration.c b/migration.c
index 7235c23..84587e9 100644
--- a/migration.c
+++ b/migration.c
@@ -469,6 +469,7 @@ void qmp_migrate_cancel(Error **errp)
 void qmp_migrate_set_cache_size(int64_t value, Error **errp)
 {
     MigrationState *s = migrate_get_current();
+    int64_t new_size;
 
     /* Check for truncation */
     if (value != (size_t)value) {
@@ -477,7 +478,14 @@ void qmp_migrate_set_cache_size(int64_t value, Error **errp)
         return;
     }
 
-    s->xbzrle_cache_size = xbzrle_cache_resize(value);
+    new_size = xbzrle_cache_resize(value);
+    if (new_size < 0) {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
+                  "is smaller than page size");
+        return;
+    }
+
+    s->xbzrle_cache_size = new_size;
 }
 
 int64_t qmp_query_migrate_cache_size(Error **errp)
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 3/6] migration:fix free XBZRLE decoded_buf wrong

[Orit Wasserman]

From: "Gonglei (Arei)" <arei.gonglei@huawei.com>

When qemu do live migration with xbzrle, qemu malloc decoded_buf
at destination end but free it at source end. It will crash qemu
by double free error in some scenarios. Splitting the XBZRLE structure
for clear logic distinguishing src/dst side.

Signed-off-by: ChenLiang <chenliang88@huawei.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Orit Wasserman <owasserm@redhat.com>
Signed-off-by: GongLei <arei.gonglei@huawei.com>
---
 arch_init.c                   | 22 ++++++++++++----------
 include/migration/migration.h |  1 +
 migration.c                   |  1 +
 3 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 8edeabe..5eff80b 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -164,17 +164,15 @@ static struct {
     uint8_t *encoded_buf;
     /* buffer for storing page content */
     uint8_t *current_buf;
-    /* buffer used for XBZRLE decoding */
-    uint8_t *decoded_buf;
     /* Cache for XBZRLE */
     PageCache *cache;
 } XBZRLE = {
     .encoded_buf = NULL,
     .current_buf = NULL,
-    .decoded_buf = NULL,
     .cache = NULL,
 };
-
+/* buffer used for XBZRLE decoding */
+static uint8_t *xbzrle_decoded_buf;
 
 int64_t xbzrle_cache_resize(int64_t new_size)
 {
@@ -606,6 +604,12 @@ uint64_t ram_bytes_total(void)
     return total;
 }
 
+void free_xbzrle_decoded_buf(void)
+{
+    g_free(xbzrle_decoded_buf);
+    xbzrle_decoded_buf = NULL;
+}
+
 static void migration_end(void)
 {
     if (migration_bitmap) {
@@ -619,11 +623,9 @@ static void migration_end(void)
         g_free(XBZRLE.cache);
         g_free(XBZRLE.encoded_buf);
         g_free(XBZRLE.current_buf);
-        g_free(XBZRLE.decoded_buf);
         XBZRLE.cache = NULL;
         XBZRLE.encoded_buf = NULL;
         XBZRLE.current_buf = NULL;
-        XBZRLE.decoded_buf = NULL;
     }
 }
 
@@ -814,8 +816,8 @@ static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host)
     unsigned int xh_len;
     int xh_flags;
 
-    if (!XBZRLE.decoded_buf) {
-        XBZRLE.decoded_buf = g_malloc(TARGET_PAGE_SIZE);
+    if (!xbzrle_decoded_buf) {
+        xbzrle_decoded_buf = g_malloc(TARGET_PAGE_SIZE);
     }
 
     /* extract RLE header */
@@ -832,10 +834,10 @@ static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host)
         return -1;
     }
     /* load data and decode */
-    qemu_get_buffer(f, XBZRLE.decoded_buf, xh_len);
+    qemu_get_buffer(f, xbzrle_decoded_buf, xh_len);
 
     /* decode RLE */
-    ret = xbzrle_decode_buffer(XBZRLE.decoded_buf, xh_len, host,
+    ret = xbzrle_decode_buffer(xbzrle_decoded_buf, xh_len, host,
                                TARGET_PAGE_SIZE);
     if (ret == -1) {
         fprintf(stderr, "Failed to load XBZRLE page - decode error!\n");
diff --git a/include/migration/migration.h b/include/migration/migration.h
index bfa3951..3e1e6c7 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -109,6 +109,7 @@ MigrationState *migrate_get_current(void);
 uint64_t ram_bytes_remaining(void);
 uint64_t ram_bytes_transferred(void);
 uint64_t ram_bytes_total(void);
+void free_xbzrle_decoded_buf(void);
 
 void acct_update_position(QEMUFile *f, size_t size, bool zero);
 
diff --git a/migration.c b/migration.c
index 84587e9..46a7305 100644
--- a/migration.c
+++ b/migration.c
@@ -105,6 +105,7 @@ static void process_incoming_migration_co(void *opaque)
 
     ret = qemu_loadvm_state(f);
     qemu_fclose(f);
+    free_xbzrle_decoded_buf();
     if (ret < 0) {
         fprintf(stderr, "load of migration failed\n");
         exit(EXIT_FAILURE);
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 4/6] XBZRLE cache size should not be larger than guest memory size

[Orit Wasserman]

Signed-off-by: Orit Wasserman <owasserm@redhat.com>
---
 migration.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/migration.c b/migration.c
index 46a7305..25add6f 100644
--- a/migration.c
+++ b/migration.c
@@ -479,6 +479,13 @@ void qmp_migrate_set_cache_size(int64_t value, Error **errp)
         return;
     }
 
+    /* Cache should not be larger than guest ram size */
+    if (value > ram_bytes_total()) {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
+                  "exceeds guest ram size ");
+        return;
+    }
+
     new_size = xbzrle_cache_resize(value);
     if (new_size < 0) {
         error_set(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 5/6] Don't abort on out of memory when creating page cache

[Orit Wasserman]

Signed-off-by: Orit Wasserman <owasserm@redhat.com>
---
 arch_init.c  | 18 ++++++++++++++++--
 page_cache.c | 18 ++++++++++++++----
 2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 5eff80b..1fa5f1f 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -664,8 +664,22 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
             DPRINTF("Error creating cache\n");
             return -1;
         }
-        XBZRLE.encoded_buf = g_malloc0(TARGET_PAGE_SIZE);
-        XBZRLE.current_buf = g_malloc(TARGET_PAGE_SIZE);
+
+        /* We prefer not to abort if there is no memory */
+        XBZRLE.encoded_buf = g_try_malloc0(TARGET_PAGE_SIZE);
+        if (!XBZRLE.encoded_buf) {
+            DPRINTF("Error allocating encoded_buf\n");
+            return -1;
+        }
+
+        XBZRLE.current_buf = g_try_malloc(TARGET_PAGE_SIZE);
+        if (!XBZRLE.current_buf) {
+            DPRINTF("Error allocating current_buf\n");
+            g_free(XBZRLE.encoded_buf);
+            XBZRLE.encoded_buf = NULL;
+            return -1;
+        }
+
         acct_clear();
     }
 
diff --git a/page_cache.c b/page_cache.c
index a05db64..62a53f8 100644
--- a/page_cache.c
+++ b/page_cache.c
@@ -60,8 +60,12 @@ PageCache *cache_init(int64_t num_pages, unsigned int page_size)
         return NULL;
     }
 
-    cache = g_malloc(sizeof(*cache));
-
+    /* We prefer not to abort if there is no memory */
+    cache = g_try_malloc(sizeof(*cache));
+    if (!cache) {
+        DPRINTF("Failed to allocate cache\n");
+        return NULL;
+    }
     /* round down to the nearest power of 2 */
     if (!is_power_of_2(num_pages)) {
         num_pages = pow2floor(num_pages);
@@ -74,8 +78,14 @@ PageCache *cache_init(int64_t num_pages, unsigned int page_size)
 
     DPRINTF("Setting cache buckets to %" PRId64 "\n", cache->max_num_items);
 
-    cache->page_cache = g_malloc((cache->max_num_items) *
-                                 sizeof(*cache->page_cache));
+    /* We prefer not to abort if there is no memory */
+    cache->page_cache = g_try_malloc((cache->max_num_items) *
+                                     sizeof(*cache->page_cache));
+    if (!cache->page_cache) {
+        DPRINTF("Failed to allocate cache->page_cache\n");
+        g_free(cache);
+        return NULL;
+    }
 
     for (i = 0; i < cache->max_num_items; i++) {
         cache->page_cache[i].it_data = NULL;
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 6/6] Don't abort on memory allocation error

[Orit Wasserman]

It is better to fail migration in case of failure to
allocate new cache item

Signed-off-by: Orit Wasserman <owasserm@redhat.com>
---
 arch_init.c                    |  4 +++-
 include/migration/page_cache.h |  4 +++-
 page_cache.c                   | 16 +++++++++++-----
 3 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 1fa5f1f..80574a0 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -284,7 +284,9 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t *current_data,
 
     if (!cache_is_cached(XBZRLE.cache, current_addr)) {
         if (!last_stage) {
-            cache_insert(XBZRLE.cache, current_addr, current_data);
+            if (cache_insert(XBZRLE.cache, current_addr, current_data) == -1) {
+                return -1;
+            }
         }
         acct_info.xbzrle_cache_miss++;
         return -1;
diff --git a/include/migration/page_cache.h b/include/migration/page_cache.h
index 87894fe..d156f0d 100644
--- a/include/migration/page_cache.h
+++ b/include/migration/page_cache.h
@@ -60,11 +60,13 @@ uint8_t *get_cached_data(const PageCache *cache, uint64_t addr);
  * cache_insert: insert the page into the cache. the page cache
  * will dup the data on insert. the previous value will be overwritten
  *
+ * Returns -1 on error
+ *
  * @cache pointer to the PageCache struct
  * @addr: page address
  * @pdata: pointer to the page
  */
-void cache_insert(PageCache *cache, uint64_t addr, uint8_t *pdata);
+int cache_insert(PageCache *cache, uint64_t addr, 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 62a53f8..3ef6ee7 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;
 }
 
-void cache_insert(PageCache *cache, uint64_t addr, uint8_t *pdata)
+int cache_insert(PageCache *cache, uint64_t addr, uint8_t *pdata)
 {
 
     CacheItem *it = NULL;
@@ -161,16 +161,22 @@ void cache_insert(PageCache *cache, uint64_t addr, uint8_t *pdata)
     /* actual update of entry */
     it = cache_get_by_addr(cache, addr);
 
-    /* free old cached data if any */
-    g_free(it->it_data);
-
+    /* allocate page */
     if (!it->it_data) {
+        it->it_data = g_try_malloc(cache->page_size);
+        if (!it->it_data) {
+            DPRINTF("Error allocating page\n");
+            return -1;
+        }
         cache->num_items++;
     }
 
-    it->it_data = g_memdup(pdata, cache->page_size);
+    memcpy(it->it_data, pdata, cache->page_size);
+
     it->it_age = ++cache->max_item_age;
     it->it_addr = addr;
+
+    return 0;
 }
 
 int64_t cache_resize(PageCache *cache, int64_t new_num_pages)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 6/6] Don't abort on memory allocation error

[Dr. David Alan Gilbert]

* Orit Wasserman (owasserm@redhat.com) wrote:
> It is better to fail migration in case of failure to
> allocate new cache item
> 
> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
> ---
>  arch_init.c                    |  4 +++-
>  include/migration/page_cache.h |  4 +++-
>  page_cache.c                   | 16 +++++++++++-----
>  3 files changed, 17 insertions(+), 7 deletions(-)


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> 
> diff --git a/arch_init.c b/arch_init.c
> index 1fa5f1f..80574a0 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -284,7 +284,9 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t *current_data,
>  
>      if (!cache_is_cached(XBZRLE.cache, current_addr)) {
>          if (!last_stage) {
> -            cache_insert(XBZRLE.cache, current_addr, current_data);
> +            if (cache_insert(XBZRLE.cache, current_addr, current_data) == -1) {
> +                return -1;
> +            }
>          }
>          acct_info.xbzrle_cache_miss++;
>          return -1;
> diff --git a/include/migration/page_cache.h b/include/migration/page_cache.h
> index 87894fe..d156f0d 100644
> --- a/include/migration/page_cache.h
> +++ b/include/migration/page_cache.h
> @@ -60,11 +60,13 @@ uint8_t *get_cached_data(const PageCache *cache, uint64_t addr);
>   * cache_insert: insert the page into the cache. the page cache
>   * will dup the data on insert. the previous value will be overwritten
>   *
> + * Returns -1 on error
> + *
>   * @cache pointer to the PageCache struct
>   * @addr: page address
>   * @pdata: pointer to the page
>   */
> -void cache_insert(PageCache *cache, uint64_t addr, uint8_t *pdata);
> +int cache_insert(PageCache *cache, uint64_t addr, 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 62a53f8..3ef6ee7 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;
>  }
>  
> -void cache_insert(PageCache *cache, uint64_t addr, uint8_t *pdata)
> +int cache_insert(PageCache *cache, uint64_t addr, uint8_t *pdata)
>  {
>  
>      CacheItem *it = NULL;
> @@ -161,16 +161,22 @@ void cache_insert(PageCache *cache, uint64_t addr, uint8_t *pdata)
>      /* actual update of entry */
>      it = cache_get_by_addr(cache, addr);
>  
> -    /* free old cached data if any */
> -    g_free(it->it_data);
> -
> +    /* allocate page */
>      if (!it->it_data) {
> +        it->it_data = g_try_malloc(cache->page_size);
> +        if (!it->it_data) {
> +            DPRINTF("Error allocating page\n");
> +            return -1;
> +        }
>          cache->num_items++;
>      }
>  
> -    it->it_data = g_memdup(pdata, cache->page_size);
> +    memcpy(it->it_data, pdata, cache->page_size);
> +
>      it->it_age = ++cache->max_item_age;
>      it->it_addr = addr;
> +
> +    return 0;
>  }
>  
>  int64_t cache_resize(PageCache *cache, int64_t new_num_pages)
> -- 
> 1.8.3.1

> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 5/6] Don't abort on out of memory when creating page cache

[Dr. David Alan Gilbert]

* Orit Wasserman (owasserm@redhat.com) wrote:
> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
> ---
>  arch_init.c  | 18 ++++++++++++++++--
>  page_cache.c | 18 ++++++++++++++----
>  2 files changed, 30 insertions(+), 6 deletions(-)

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> 
> diff --git a/arch_init.c b/arch_init.c
> index 5eff80b..1fa5f1f 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -664,8 +664,22 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>              DPRINTF("Error creating cache\n");
>              return -1;
>          }
> -        XBZRLE.encoded_buf = g_malloc0(TARGET_PAGE_SIZE);
> -        XBZRLE.current_buf = g_malloc(TARGET_PAGE_SIZE);
> +
> +        /* We prefer not to abort if there is no memory */
> +        XBZRLE.encoded_buf = g_try_malloc0(TARGET_PAGE_SIZE);
> +        if (!XBZRLE.encoded_buf) {
> +            DPRINTF("Error allocating encoded_buf\n");
> +            return -1;
> +        }
> +
> +        XBZRLE.current_buf = g_try_malloc(TARGET_PAGE_SIZE);
> +        if (!XBZRLE.current_buf) {
> +            DPRINTF("Error allocating current_buf\n");
> +            g_free(XBZRLE.encoded_buf);
> +            XBZRLE.encoded_buf = NULL;
> +            return -1;
> +        }
> +
>          acct_clear();
>      }
>  
> diff --git a/page_cache.c b/page_cache.c
> index a05db64..62a53f8 100644
> --- a/page_cache.c
> +++ b/page_cache.c
> @@ -60,8 +60,12 @@ PageCache *cache_init(int64_t num_pages, unsigned int page_size)
>          return NULL;
>      }
>  
> -    cache = g_malloc(sizeof(*cache));
> -
> +    /* We prefer not to abort if there is no memory */
> +    cache = g_try_malloc(sizeof(*cache));
> +    if (!cache) {
> +        DPRINTF("Failed to allocate cache\n");
> +        return NULL;
> +    }
>      /* round down to the nearest power of 2 */
>      if (!is_power_of_2(num_pages)) {
>          num_pages = pow2floor(num_pages);
> @@ -74,8 +78,14 @@ PageCache *cache_init(int64_t num_pages, unsigned int page_size)
>  
>      DPRINTF("Setting cache buckets to %" PRId64 "\n", cache->max_num_items);
>  
> -    cache->page_cache = g_malloc((cache->max_num_items) *
> -                                 sizeof(*cache->page_cache));
> +    /* We prefer not to abort if there is no memory */
> +    cache->page_cache = g_try_malloc((cache->max_num_items) *
> +                                     sizeof(*cache->page_cache));
> +    if (!cache->page_cache) {
> +        DPRINTF("Failed to allocate cache->page_cache\n");
> +        g_free(cache);
> +        return NULL;
> +    }
>  
>      for (i = 0; i < cache->max_num_items; i++) {
>          cache->page_cache[i].it_data = NULL;
> -- 
> 1.8.3.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 4/6] XBZRLE cache size should not be larger than guest memory size

[Dr. David Alan Gilbert]

* Orit Wasserman (owasserm@redhat.com) wrote:
> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
> ---
>  migration.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/migration.c b/migration.c
> index 46a7305..25add6f 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -479,6 +479,13 @@ void qmp_migrate_set_cache_size(int64_t value, Error **errp)
>          return;
>      }
>  
> +    /* Cache should not be larger than guest ram size */

Why? (It's admittedly odd, but does it actually break something if it's larger?)

Dave

> +    if (value > ram_bytes_total()) {
> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
> +                  "exceeds guest ram size ");
> +        return;
> +    }
> +
>      new_size = xbzrle_cache_resize(value);
>      if (new_size < 0) {
>          error_set(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
> -- 
> 1.8.3.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 4/6] XBZRLE cache size should not be larger than guest memory size

[Orit Wasserman]

On 01/30/2014 08:23 PM, Dr. David Alan Gilbert wrote:
> * Orit Wasserman (owasserm@redhat.com) wrote:
>> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
>> ---
>>   migration.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/migration.c b/migration.c
>> index 46a7305..25add6f 100644
>> --- a/migration.c
>> +++ b/migration.c
>> @@ -479,6 +479,13 @@ void qmp_migrate_set_cache_size(int64_t value, Error **errp)
>>           return;
>>       }
>>
>> +    /* Cache should not be larger than guest ram size */
>
> Why? (It's admittedly odd, but does it actually break something if it's larger?)
>

Because how XBZRLE works, the idea is that for workload that changes the same pages
frequently, we can reduce the amount of transferred data sent by sending only the diff.
We also compress the diff itself.

The cache is used to store the previous page so we can calculate the diff, so at most it will
contain all the guest pages.

Orit

> Dave
>
>> +    if (value > ram_bytes_total()) {
>> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
>> +                  "exceeds guest ram size ");
>> +        return;
>> +    }
>> +
>>       new_size = xbzrle_cache_resize(value);
>>       if (new_size < 0) {
>>           error_set(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
>> --
>> 1.8.3.1
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>

Re: [Qemu-devel] [PATCH v2 4/6] XBZRLE cache size should not be larger than guest memory size

[Dr. David Alan Gilbert]

* Orit Wasserman (owasserm@redhat.com) wrote:
> Signed-off-by: Orit Wasserman <owasserm@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/migration.c b/migration.c
> index 46a7305..25add6f 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -479,6 +479,13 @@ void qmp_migrate_set_cache_size(int64_t value, Error **errp)
>          return;
>      }
>  
> +    /* Cache should not be larger than guest ram size */
> +    if (value > ram_bytes_total()) {
> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
> +                  "exceeds guest ram size ");
> +        return;
> +    }
> +
>      new_size = xbzrle_cache_resize(value);
>      if (new_size < 0) {
>          error_set(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
> -- 
> 1.8.3.1
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 4/6] XBZRLE cache size should not be larger than guest memory size

[Dr. David Alan Gilbert]

* Orit Wasserman (owasserm@redhat.com) wrote:
> On 01/30/2014 08:23 PM, Dr. David Alan Gilbert wrote:
> >* Orit Wasserman (owasserm@redhat.com) wrote:
> >>Signed-off-by: Orit Wasserman <owasserm@redhat.com>
> >>---
> >>  migration.c | 7 +++++++
> >>  1 file changed, 7 insertions(+)
> >>
> >>diff --git a/migration.c b/migration.c
> >>index 46a7305..25add6f 100644
> >>--- a/migration.c
> >>+++ b/migration.c
> >>@@ -479,6 +479,13 @@ void qmp_migrate_set_cache_size(int64_t value, Error **errp)
> >>          return;
> >>      }
> >>
> >>+    /* Cache should not be larger than guest ram size */
> >
> >Why? (It's admittedly odd, but does it actually break something if it's larger?)
> >
> 
> Because how XBZRLE works, the idea is that for workload that changes the same pages
> frequently, we can reduce the amount of transferred data sent by sending only the diff.
> We also compress the diff itself.
>
> The cache is used to store the previous page so we can calculate the diff, so at most it will
> contain all the guest pages.

It's a hash based cache though isn't it - so there will be some contention for
a cache size==ram size case?

Also this does mean that you have to be a little careful to pick a sane XBZRLE cache
size, since one that's too large will now fail; I can only see that being
a problem on a machine with a mix of huge and tiny VMs.

( I sent the reviewd-by tag separately.)

Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 4/6] XBZRLE cache size should not be larger than guest memory size

[Orit Wasserman]

On 01/30/2014 08:48 PM, Dr. David Alan Gilbert wrote:
> * Orit Wasserman (owasserm@redhat.com) wrote:
>> On 01/30/2014 08:23 PM, Dr. David Alan Gilbert wrote:
>>> * Orit Wasserman (owasserm@redhat.com) wrote:
>>>> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
>>>> ---
>>>>   migration.c | 7 +++++++
>>>>   1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/migration.c b/migration.c
>>>> index 46a7305..25add6f 100644
>>>> --- a/migration.c
>>>> +++ b/migration.c
>>>> @@ -479,6 +479,13 @@ void qmp_migrate_set_cache_size(int64_t value, Error **errp)
>>>>           return;
>>>>       }
>>>>
>>>> +    /* Cache should not be larger than guest ram size */
>>>
>>> Why? (It's admittedly odd, but does it actually break something if it's larger?)
>>>
>>
>> Because how XBZRLE works, the idea is that for workload that changes the same pages
>> frequently, we can reduce the amount of transferred data sent by sending only the diff.
>> We also compress the diff itself.
>>
>> The cache is used to store the previous page so we can calculate the diff, so at most it will
>> contain all the guest pages.
>
> It's a hash based cache though isn't it - so there will be some contention for
> a cache size==ram size case?
>

not really because the hash function is so simple it becomes like an index into array.


> Also this does mean that you have to be a little careful to pick a sane XBZRLE cache
> size, since one that's too large will now fail; I can only see that being
> a problem on a machine with a mix of huge and tiny VMs.
>

Yes if you pick the cache too small than you will have lots of missing and the compression
wont be useful. if you pick too large than you wasted memory for nothing.
What also problematic is picking the right workload, it will only be effective if between each
migration iteration the same pages are changed.

> ( I sent the reviewd-by tag separately.)

Thanks,
Orit
>
> Dave
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>