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

[Qemu-devel] [PATCH 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.

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                    | 47 +++++++++++++++++++++++++++++++-----------
 include/migration/migration.h  |  1 +
 include/migration/page_cache.h |  4 +++-
 migration.c                    | 18 +++++++++++++++-
 page_cache.c                   | 34 ++++++++++++++++++++++--------
 5 files changed, 81 insertions(+), 23 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 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

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 812 bytes --]

On 01/30/2014 12:14 AM, Orit Wasserman wrote:
> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> ---
>  arch_init.c | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> 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;
>      }
>  }
>  
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 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 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

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

[Juan Quintela]

Orit Wasserman <owasserm@redhat.com> wrote:
> 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>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Good catch O:-)

[Qemu-devel] [PATCH 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

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

[Juan Quintela]

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

Reviewed-by: Juan Quintela <quintela@redhat.com>

[Qemu-devel] [PATCH 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  | 16 ++++++++++++++--
 page_cache.c | 18 ++++++++++++++----
 2 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 5eff80b..806d096 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -664,8 +664,20 @@ 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");
+            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

Re: [Qemu-devel] [PATCH 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  | 16 ++++++++++++++--
>  page_cache.c | 18 ++++++++++++++----
>  2 files changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 5eff80b..806d096 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -664,8 +664,20 @@ 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");
> +            return -1;
> +        }

Would it be best to free encoded_buf in this second exit case?

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

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

[Orit Wasserman]

On 01/30/2014 05:48 PM, Dr. David Alan Gilbert wrote:
> * Orit Wasserman (owasserm@redhat.com) wrote:
>> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
>> ---
>>   arch_init.c  | 16 ++++++++++++++--
>>   page_cache.c | 18 ++++++++++++++----
>>   2 files changed, 28 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch_init.c b/arch_init.c
>> index 5eff80b..806d096 100644
>> --- a/arch_init.c
>> +++ b/arch_init.c
>> @@ -664,8 +664,20 @@ 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");
>> +            return -1;
>> +        }
>
> Would it be best to free encoded_buf in this second exit case?
>

It is freed in migration_end (that is called when migration fails or canceled).

Orit

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

[Qemu-devel] [PATCH 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 806d096..0bfbc5a 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..69e8329 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) {
         cache->num_items++;
+        it->it_data = g_try_malloc(cache->page_size);
+        if (!it->it_data) {
+            DPRINTF("Error allocating page\n");
+            return -1;
+        }
     }
 
-    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 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

Does this actually fail migration or just drop back to sending an uncompressed
page? (I think the latter, and that's an even better result).

> 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 806d096..0bfbc5a 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..69e8329 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) {
>          cache->num_items++;
> +        it->it_data = g_try_malloc(cache->page_size);
> +        if (!it->it_data) {
> +            DPRINTF("Error allocating page\n");
> +            return -1;
> +        }

It feels like it would have been correct to do the num_items++ after the 
allocation test (although since the num_items doesn't seem to be used
anywhere that's not a show-stopper, but worth fixing sometime).

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

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

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

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

[Orit Wasserman]

On 01/30/2014 06:08 PM, Dr. David Alan Gilbert wrote:
> * Orit Wasserman (owasserm@redhat.com) wrote:
>> It is better to fail migration in case of failure to
>> allocate new cache item
>
> Does this actually fail migration or just drop back to sending an uncompressed
> page? (I think the latter, and that's an even better result).
>

It fails the migration, if the users wants to disable compression it can do it
and start migration again.

>> 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 806d096..0bfbc5a 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..69e8329 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) {
>>           cache->num_items++;
>> +        it->it_data = g_try_malloc(cache->page_size);
>> +        if (!it->it_data) {
>> +            DPRINTF("Error allocating page\n");
>> +            return -1;
>> +        }
>
> It feels like it would have been correct to do the num_items++ after the
> allocation test (although since the num_items doesn't seem to be used
> anywhere that's not a show-stopper, but worth fixing sometime).
>

You are correct, I will move it after allocation.

Orit
>>       }
>>
>> -    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
>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>
> Dave
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>

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

[Juan Quintela]

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>

Reviewed-by: Juan Quintela <quintela@redhat.com>