[Qemu-devel] [PATCH v2 00/10] Make xbzrle_cache_size a migration parameter

[Qemu-devel] [PATCH v2 00/10] Make xbzrle_cache_size a migration parameter

[Juan Quintela]

In v2

- rebase
- fix comments about power of 2 instead of multiple of two
- minor typo on error message
- use Error * when setting values

After asking people of libvirt/ovirt/openstack, it appears that there
is no problem about restricting cache_size to real values.  We give
back one error and they had to handle errors already.

Please review, Juan.

[v1]

I started looking at xbzrle_cache_size and saw that it was not a parameter.
How difficult could it be?  HaHa

- x-multifd-page-count and x-multifd-channels were not o
  migrate_test_apply

- page_cache.c has parameters that were not used (max_item_age)

- page cache types were random.  Move almost all to int/size_t as
  appropiate

- error handling was split in three functions:
  xbzrle_cache_resize
  cache_init
  qmp_set_cache_size
  Make some sense of it and use Error*

- We pass one value to qmp_migrate_cache_size it asigned pow2log() of
  that value, and cache_init() used a power of two.  Make sure that
  the value passed is right.

- Now I am able to make xbzrle_cache_size a parameter!!!!

- Now that I have arrived here, I tried to make the json types for
  migartion parameters the real ones instead of everything is an int64.
  I will be happy if libvirt people checked this.

Please review, Juan.


*** BLURB HERE ***

Juan Quintela (10):
  migration: Fix migrate_test_apply for multifd parameters
  migratiom: Remove max_item_age parameter
  migration: Make cache size elements use the right types
  migration: Move xbzrle cache resize error handling to
    xbzrle_cache_resize
  migration: Make cache_init() take an error parameter
  migration: Make sure that we pass the right cache size
  migration: Don't play games with the requested cache size
  migration: No need to return the size of the cache
  migration: Make xbzrle_cache_size a migration parameter
  migration: [RFC] Use proper types in json

 hmp.c                  |  34 ++++++++++-----
 migration/migration.c  | 116 ++++++++++++++++++++++++-------------------------
 migration/migration.h  |   1 -
 migration/page_cache.c |  41 +++++++++--------
 migration/page_cache.h |   7 ++-
 migration/ram.c        |  36 +++++++--------
 migration/ram.h        |   2 +-
 qapi/migration.json    |  44 ++++++++++++++-----
 8 files changed, 160 insertions(+), 121 deletions(-)

-- 
2.13.6

[Qemu-devel] [PATCH v2 01/10] migration: Fix migrate_test_apply for multifd parameters

[Juan Quintela]

They were missing when introduced on the tree

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index 98429dc843..fb62a639d8 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -865,6 +865,12 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
     if (params->has_block_incremental) {
         dest->block_incremental = params->block_incremental;
     }
+    if (params->has_x_multifd_channels) {
+        dest->x_multifd_channels = params->x_multifd_channels;
+    }
+    if (params->has_x_multifd_page_count) {
+        dest->x_multifd_page_count = params->x_multifd_page_count;
+    }
 }
 
 static void migrate_params_apply(MigrateSetParameters *params)
-- 
2.13.6

[Qemu-devel] [PATCH v2 02/10] migratiom: Remove max_item_age parameter

[Juan Quintela]

It was not used at all since commit:

27af7d6ea5015e5ef1f7985eab94a8a218267a2b

which replaced its use by the dirty sync count.

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
---
 migration/page_cache.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/migration/page_cache.c b/migration/page_cache.c
index ba984c4858..381e555ddb 100644
--- a/migration/page_cache.c
+++ b/migration/page_cache.c
@@ -41,7 +41,6 @@ struct PageCache {
     CacheItem *page_cache;
     unsigned int page_size;
     int64_t max_num_items;
-    uint64_t max_item_age;
     int64_t num_items;
 };
 
@@ -69,7 +68,6 @@ PageCache *cache_init(int64_t num_pages, unsigned int page_size)
     }
     cache->page_size = page_size;
     cache->num_items = 0;
-    cache->max_item_age = 0;
     cache->max_num_items = num_pages;
 
     DPRINTF("Setting cache buckets to %" PRId64 "\n", cache->max_num_items);
-- 
2.13.6

[Qemu-devel] [PATCH v2 03/10] migration: Make cache size elements use the right types

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/page_cache.c | 8 ++++----
 migration/page_cache.h | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/migration/page_cache.c b/migration/page_cache.c
index 381e555ddb..6b2dd77cf0 100644
--- a/migration/page_cache.c
+++ b/migration/page_cache.c
@@ -39,12 +39,12 @@ struct CacheItem {
 
 struct PageCache {
     CacheItem *page_cache;
-    unsigned int page_size;
-    int64_t max_num_items;
-    int64_t num_items;
+    size_t page_size;
+    size_t max_num_items;
+    size_t num_items;
 };
 
-PageCache *cache_init(int64_t num_pages, unsigned int page_size)
+PageCache *cache_init(size_t num_pages, size_t page_size)
 {
     int64_t i;
 
diff --git a/migration/page_cache.h b/migration/page_cache.h
index 4fadd0c501..931868b857 100644
--- a/migration/page_cache.h
+++ b/migration/page_cache.h
@@ -28,7 +28,7 @@ typedef struct PageCache PageCache;
  * @num_pages: cache maximal number of cached pages
  * @page_size: cache page size
  */
-PageCache *cache_init(int64_t num_pages, unsigned int page_size);
+PageCache *cache_init(size_t num_pages, size_t page_size);
 
 /**
  * cache_fini: free all cache resources
-- 
2.13.6

[Qemu-devel] [PATCH v2 04/10] migration: Move xbzrle cache resize error handling to xbzrle_cache_resize

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 18 +-----------------
 migration/ram.c       | 22 ++++++++++++++++++++--
 migration/ram.h       |  2 +-
 3 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index fb62a639d8..3feffb5e26 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1373,24 +1373,8 @@ 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) {
-        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
-                   "exceeding address space");
-        return;
-    }
-
-    /* Cache should not be larger than guest ram size */
-    if (value > ram_bytes_total()) {
-        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
-                   "exceeds guest ram size ");
-        return;
-    }
-
-    new_size = xbzrle_cache_resize(value);
+    new_size = xbzrle_cache_resize(value, errp);
     if (new_size < 0) {
-        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
-                   "is smaller than page size");
         return;
     }
 
diff --git a/migration/ram.c b/migration/ram.c
index b83f8977c5..7c3acad029 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -42,6 +42,7 @@
 #include "postcopy-ram.h"
 #include "migration/page_cache.h"
 #include "qemu/error-report.h"
+#include "qapi/qmp/qerror.h"
 #include "trace.h"
 #include "exec/ram_addr.h"
 #include "qemu/rcu_queue.h"
@@ -113,13 +114,30 @@ static void XBZRLE_cache_unlock(void)
  * Returns the new_size or negative in case of error.
  *
  * @new_size: new cache size
+ * @errp: set *errp if the check failed, with reason
  */
-int64_t xbzrle_cache_resize(int64_t new_size)
+int64_t xbzrle_cache_resize(int64_t new_size, Error **errp)
 {
     PageCache *new_cache;
     int64_t ret;
 
+    /* Check for truncation */
+    if (new_size != (size_t)new_size) {
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
+                   "exceeding address space");
+        return -1;
+    }
+
+    /* Cache should not be larger than guest ram size */
+    if (new_size > ram_bytes_total()) {
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
+                   "exceeds guest ram size");
+        return -1;
+    }
+
     if (new_size < TARGET_PAGE_SIZE) {
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
+                   "is smaller than one target page size");
         return -1;
     }
 
@@ -132,7 +150,7 @@ int64_t xbzrle_cache_resize(int64_t new_size)
         new_cache = cache_init(new_size / TARGET_PAGE_SIZE,
                                         TARGET_PAGE_SIZE);
         if (!new_cache) {
-            error_report("Error creating cache");
+            error_setg(errp, "Error creating cache");
             ret = -1;
             goto out;
         }
diff --git a/migration/ram.h b/migration/ram.h
index 4a72d66503..511b3dc582 100644
--- a/migration/ram.h
+++ b/migration/ram.h
@@ -35,7 +35,7 @@
 extern MigrationStats ram_counters;
 extern XBZRLECacheStats xbzrle_counters;
 
-int64_t xbzrle_cache_resize(int64_t new_size);
+int64_t xbzrle_cache_resize(int64_t new_size, Error **errp);
 uint64_t ram_bytes_remaining(void);
 uint64_t ram_bytes_total(void);
 
-- 
2.13.6

[Qemu-devel] [PATCH v2 05/10] migration: Make cache_init() take an error parameter

[Juan Quintela]

Once there, take a total size instead of the size of the pages.  We
move the check that the new_size is bigger than one page from
xbzrle_cache_resize().

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>

--

Fix typo spotted by Peter Xu
---
 migration/page_cache.c | 17 +++++++++++------
 migration/page_cache.h |  7 +++----
 migration/ram.c        | 18 +++++-------------
 3 files changed, 19 insertions(+), 23 deletions(-)

diff --git a/migration/page_cache.c b/migration/page_cache.c
index 6b2dd77cf0..9a9d13d6a2 100644
--- a/migration/page_cache.c
+++ b/migration/page_cache.c
@@ -14,6 +14,8 @@
 
 #include "qemu/osdep.h"
 
+#include "qapi/qmp/qerror.h"
+#include "qapi/error.h"
 #include "qemu-common.h"
 #include "qemu/host-utils.h"
 #include "migration/page_cache.h"
@@ -44,21 +46,23 @@ struct PageCache {
     size_t num_items;
 };
 
-PageCache *cache_init(size_t num_pages, size_t page_size)
+PageCache *cache_init(int64_t new_size, size_t page_size, Error **errp)
 {
     int64_t i;
-
+    size_t num_pages = new_size / page_size;
     PageCache *cache;
 
-    if (num_pages <= 0) {
-        DPRINTF("invalid number of pages\n");
+    if (new_size < page_size) {
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
+                   "is smaller than one target page size");
         return NULL;
     }
 
     /* We prefer not to abort if there is no memory */
     cache = g_try_malloc(sizeof(*cache));
     if (!cache) {
-        DPRINTF("Failed to allocate cache\n");
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
+                   "Failed to allocate cache");
         return NULL;
     }
     /* round down to the nearest power of 2 */
@@ -76,7 +80,8 @@ PageCache *cache_init(size_t num_pages, size_t page_size)
     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");
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
+                   "Failed to allocate page cache");
         g_free(cache);
         return NULL;
     }
diff --git a/migration/page_cache.h b/migration/page_cache.h
index 931868b857..0cb94498a0 100644
--- a/migration/page_cache.h
+++ b/migration/page_cache.h
@@ -24,12 +24,11 @@ typedef struct PageCache PageCache;
  *
  * Returns new allocated cache or NULL on error
  *
- * @cache pointer to the PageCache struct
- * @num_pages: cache maximal number of cached pages
+ * @cache_size: cache size in bytes
  * @page_size: cache page size
+ * @errp: set *errp if the check failed, with reason
  */
-PageCache *cache_init(size_t num_pages, size_t page_size);
-
+PageCache *cache_init(int64_t cache_size, size_t page_size, Error **errp);
 /**
  * cache_fini: free all cache resources
  * @cache pointer to the PageCache struct
diff --git a/migration/ram.c b/migration/ram.c
index 7c3acad029..47501460c8 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -135,22 +135,14 @@ int64_t xbzrle_cache_resize(int64_t new_size, Error **errp)
         return -1;
     }
 
-    if (new_size < TARGET_PAGE_SIZE) {
-        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
-                   "is smaller than one target page size");
-        return -1;
-    }
-
     XBZRLE_cache_lock();
 
     if (XBZRLE.cache != NULL) {
         if (pow2floor(new_size) == migrate_xbzrle_cache_size()) {
             goto out_new_size;
         }
-        new_cache = cache_init(new_size / TARGET_PAGE_SIZE,
-                                        TARGET_PAGE_SIZE);
+        new_cache = cache_init(new_size, TARGET_PAGE_SIZE, errp);
         if (!new_cache) {
-            error_setg(errp, "Error creating cache");
             ret = -1;
             goto out;
         }
@@ -2028,6 +2020,7 @@ err:
 static int ram_state_init(RAMState **rsp)
 {
     *rsp = g_new0(RAMState, 1);
+    Error *local_err = NULL;
 
     qemu_mutex_init(&(*rsp)->bitmap_mutex);
     qemu_mutex_init(&(*rsp)->src_page_req_mutex);
@@ -2036,12 +2029,11 @@ static int ram_state_init(RAMState **rsp)
     if (migrate_use_xbzrle()) {
         XBZRLE_cache_lock();
         XBZRLE.zero_target_page = g_malloc0(TARGET_PAGE_SIZE);
-        XBZRLE.cache = cache_init(migrate_xbzrle_cache_size() /
-                                  TARGET_PAGE_SIZE,
-                                  TARGET_PAGE_SIZE);
+        XBZRLE.cache = cache_init(migrate_xbzrle_cache_size(),
+                                  TARGET_PAGE_SIZE, &local_err);
         if (!XBZRLE.cache) {
             XBZRLE_cache_unlock();
-            error_report("Error creating cache");
+            error_report_err(local_err);
             g_free(*rsp);
             *rsp = NULL;
             return -1;
-- 
2.13.6

[Qemu-devel] [PATCH v2 06/10] migration: Make sure that we pass the right cache size

[Juan Quintela]

Instead of passing silently round down the number of pages, make it an
error that the cache size is not a power of 2.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/page_cache.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/migration/page_cache.c b/migration/page_cache.c
index 9a9d13d6a2..96268c3aea 100644
--- a/migration/page_cache.c
+++ b/migration/page_cache.c
@@ -58,6 +58,13 @@ PageCache *cache_init(int64_t new_size, size_t page_size, Error **errp)
         return NULL;
     }
 
+    /* round down to the nearest power of 2 */
+    if (!is_power_of_2(num_pages)) {
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
+                   "is not a power of two number of pages");
+        return NULL;
+    }
+
     /* We prefer not to abort if there is no memory */
     cache = g_try_malloc(sizeof(*cache));
     if (!cache) {
@@ -65,11 +72,6 @@ PageCache *cache_init(int64_t new_size, size_t page_size, Error **errp)
                    "Failed to allocate cache");
         return NULL;
     }
-    /* round down to the nearest power of 2 */
-    if (!is_power_of_2(num_pages)) {
-        num_pages = pow2floor(num_pages);
-        DPRINTF("rounding down to %" PRId64 "\n", num_pages);
-    }
     cache->page_size = page_size;
     cache->num_items = 0;
     cache->max_num_items = num_pages;
-- 
2.13.6

[Qemu-devel] [PATCH v2 07/10] migration: Don't play games with the requested cache size

[Juan Quintela]

Now that we check that the value passed is a power of 2, we don't need
to play games when comparing what is the size that is going to take
the cache.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/ram.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 47501460c8..c84f22d759 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -135,12 +135,14 @@ int64_t xbzrle_cache_resize(int64_t new_size, Error **errp)
         return -1;
     }
 
+    if (new_size == migrate_xbzrle_cache_size()) {
+        /* nothing to do */
+        return new_size;
+    }
+
     XBZRLE_cache_lock();
 
     if (XBZRLE.cache != NULL) {
-        if (pow2floor(new_size) == migrate_xbzrle_cache_size()) {
-            goto out_new_size;
-        }
         new_cache = cache_init(new_size, TARGET_PAGE_SIZE, errp);
         if (!new_cache) {
             ret = -1;
@@ -151,8 +153,7 @@ int64_t xbzrle_cache_resize(int64_t new_size, Error **errp)
         XBZRLE.cache = new_cache;
     }
 
-out_new_size:
-    ret = pow2floor(new_size);
+    ret = new_size;
 out:
     XBZRLE_cache_unlock();
     return ret;
-- 
2.13.6

[Qemu-devel] [PATCH v2 08/10] migration: No need to return the size of the cache

[Juan Quintela]

After the previous commits, we make sure that the value passed is
right, or we just drop an error.  So now we return if there is one
error or we have setup correctly the value passed.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration.c | 6 ++----
 migration/ram.c       | 8 +++-----
 migration/ram.h       | 2 +-
 3 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 3feffb5e26..f3d4503ce2 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1371,14 +1371,12 @@ 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;
 
-    new_size = xbzrle_cache_resize(value, errp);
-    if (new_size < 0) {
+    if (xbzrle_cache_resize(value, errp) < 0) {
         return;
     }
 
-    s->xbzrle_cache_size = new_size;
+    s->xbzrle_cache_size = value;
 }
 
 int64_t qmp_query_migrate_cache_size(Error **errp)
diff --git a/migration/ram.c b/migration/ram.c
index c84f22d759..ed4d3c6295 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -111,15 +111,15 @@ static void XBZRLE_cache_unlock(void)
  * migration may be using the cache and might finish during this call,
  * hence changes to the cache are protected by XBZRLE.lock().
  *
- * Returns the new_size or negative in case of error.
+ * Returns the 0 or negative in case of error.
  *
  * @new_size: new cache size
  * @errp: set *errp if the check failed, with reason
  */
-int64_t xbzrle_cache_resize(int64_t new_size, Error **errp)
+int xbzrle_cache_resize(int64_t new_size, Error **errp)
 {
     PageCache *new_cache;
-    int64_t ret;
+    int64_t ret = 0;
 
     /* Check for truncation */
     if (new_size != (size_t)new_size) {
@@ -152,8 +152,6 @@ int64_t xbzrle_cache_resize(int64_t new_size, Error **errp)
         cache_fini(XBZRLE.cache);
         XBZRLE.cache = new_cache;
     }
-
-    ret = new_size;
 out:
     XBZRLE_cache_unlock();
     return ret;
diff --git a/migration/ram.h b/migration/ram.h
index 511b3dc582..c8ae382b5b 100644
--- a/migration/ram.h
+++ b/migration/ram.h
@@ -35,7 +35,7 @@
 extern MigrationStats ram_counters;
 extern XBZRLECacheStats xbzrle_counters;
 
-int64_t xbzrle_cache_resize(int64_t new_size, Error **errp);
+int xbzrle_cache_resize(int64_t new_size, Error **errp);
 uint64_t ram_bytes_remaining(void);
 uint64_t ram_bytes_total(void);
 
-- 
2.13.6

[Qemu-devel] [PATCH v2 09/10] migration: Make xbzrle_cache_size a migration parameter

[Juan Quintela]

Right now it is a variable in MigrationState instead of a
MigrationParameter.  The change allows to set it as the rest of the
Migration parameters, from the command line, with
query_migration_paramters, set_migrate_parameters, etc.

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

--

- Improve help message (dave)
- Use errp that we already have around instead of NULL (dave)
---
 hmp.c                 | 14 ++++++++++++++
 migration/migration.c | 43 ++++++++++++++++++++++++++++++++-----------
 migration/migration.h |  1 -
 migration/ram.c       |  7 -------
 qapi/migration.json   | 26 +++++++++++++++++++++++---
 5 files changed, 69 insertions(+), 22 deletions(-)

diff --git a/hmp.c b/hmp.c
index ec61329ebb..22516737a9 100644
--- a/hmp.c
+++ b/hmp.c
@@ -342,6 +342,9 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
         monitor_printf(mon, "%s: %" PRId64 "\n",
             MigrationParameter_str(MIGRATION_PARAMETER_X_MULTIFD_PAGE_COUNT),
             params->x_multifd_page_count);
+        monitor_printf(mon, "%s: %" PRId64 "\n",
+            MigrationParameter_str(MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE),
+            params->xbzrle_cache_size);
     }
 
     qapi_free_MigrationParameters(params);
@@ -1565,6 +1568,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
     Visitor *v = string_input_visitor_new(valuestr);
     MigrateSetParameters *p = g_new0(MigrateSetParameters, 1);
     uint64_t valuebw = 0;
+    uint64_t cache_size;
     Error *err = NULL;
     int val, ret;
 
@@ -1640,6 +1644,16 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
         p->has_x_multifd_page_count = true;
         visit_type_int(v, param, &p->x_multifd_page_count, &err);
         break;
+    case MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE:
+        p->has_xbzrle_cache_size = true;
+        visit_type_size(v, param, &cache_size, &err);
+        if (err || cache_size > INT64_MAX
+            || (size_t)cache_size != cache_size) {
+            error_setg(&err, "Invalid size %s", valuestr);
+            break;
+        }
+        p->xbzrle_cache_size = cache_size;
+        break;
     default:
         assert(0);
     }
diff --git a/migration/migration.c b/migration/migration.c
index f3d4503ce2..498bf942a3 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -71,7 +71,7 @@
 #define DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT 10
 
 /* Migration XBZRLE default cache size */
-#define DEFAULT_MIGRATE_CACHE_SIZE (64 * 1024 * 1024)
+#define DEFAULT_MIGRATE_XBZRLE_CACHE_SIZE (64 * 1024 * 1024)
 
 /* The delay time (in ms) between two COLO checkpoints
  * Note: Please change this default value to 10000 when we support hybrid mode.
@@ -512,6 +512,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
     params->x_multifd_channels = s->parameters.x_multifd_channels;
     params->has_x_multifd_page_count = true;
     params->x_multifd_page_count = s->parameters.x_multifd_page_count;
+    params->has_xbzrle_cache_size = true;
+    params->xbzrle_cache_size = s->parameters.xbzrle_cache_size;
 
     return params;
 }
@@ -810,6 +812,16 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp)
         return false;
     }
 
+    if (params->has_xbzrle_cache_size &&
+        (params->xbzrle_cache_size < qemu_target_page_size() ||
+         !is_power_of_2(params->xbzrle_cache_size))) {
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+                   "xbzrle_cache_size",
+                   "is invalid, it should be bigger than target page size"
+                   " and a power of two");
+        return false;
+    }
+
     return true;
 }
 
@@ -871,9 +883,12 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
     if (params->has_x_multifd_page_count) {
         dest->x_multifd_page_count = params->x_multifd_page_count;
     }
+    if (params->has_xbzrle_cache_size) {
+        dest->xbzrle_cache_size = params->xbzrle_cache_size;
+    }
 }
 
-static void migrate_params_apply(MigrateSetParameters *params)
+static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
 {
     MigrationState *s = migrate_get_current();
 
@@ -939,6 +954,10 @@ static void migrate_params_apply(MigrateSetParameters *params)
     if (params->has_x_multifd_page_count) {
         s->parameters.x_multifd_page_count = params->x_multifd_page_count;
     }
+    if (params->has_xbzrle_cache_size) {
+        s->parameters.xbzrle_cache_size = params->xbzrle_cache_size;
+        xbzrle_cache_resize(params->xbzrle_cache_size, errp);
+    }
 }
 
 void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
@@ -967,7 +986,7 @@ void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
         return;
     }
 
-    migrate_params_apply(params);
+    migrate_params_apply(params, errp);
 }
 
 
@@ -1370,13 +1389,12 @@ void qmp_migrate_cancel(Error **errp)
 
 void qmp_migrate_set_cache_size(int64_t value, Error **errp)
 {
-    MigrationState *s = migrate_get_current();
+    MigrateSetParameters p = {
+        .has_xbzrle_cache_size = true,
+        .xbzrle_cache_size = value,
+    };
 
-    if (xbzrle_cache_resize(value, errp) < 0) {
-        return;
-    }
-
-    s->xbzrle_cache_size = value;
+    qmp_migrate_set_parameters(&p, errp);
 }
 
 int64_t qmp_query_migrate_cache_size(Error **errp)
@@ -1542,7 +1560,7 @@ int64_t migrate_xbzrle_cache_size(void)
 
     s = migrate_get_current();
 
-    return s->xbzrle_cache_size;
+    return s->parameters.xbzrle_cache_size;
 }
 
 bool migrate_use_block(void)
@@ -2312,6 +2330,9 @@ static Property migration_properties[] = {
     DEFINE_PROP_INT64("x-multifd-page-count", MigrationState,
                       parameters.x_multifd_page_count,
                       DEFAULT_MIGRATE_MULTIFD_PAGE_COUNT),
+    DEFINE_PROP_SIZE("xbzrle-cache-size", MigrationState,
+                      parameters.xbzrle_cache_size,
+                      DEFAULT_MIGRATE_XBZRLE_CACHE_SIZE),
 
     /* Migration capabilities */
     DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE),
@@ -2353,7 +2374,6 @@ static void migration_instance_init(Object *obj)
     MigrationParameters *params = &ms->parameters;
 
     ms->state = MIGRATION_STATUS_NONE;
-    ms->xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE;
     ms->mbps = -1;
 
     params->tls_hostname = g_strdup("");
@@ -2371,6 +2391,7 @@ static void migration_instance_init(Object *obj)
     params->has_block_incremental = true;
     params->has_x_multifd_channels = true;
     params->has_x_multifd_page_count = true;
+    params->has_xbzrle_cache_size = true;
 }
 
 /*
diff --git a/migration/migration.h b/migration/migration.h
index b83cceadc4..d2a8d319f1 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -107,7 +107,6 @@ struct MigrationState
     int64_t downtime;
     int64_t expected_downtime;
     bool enabled_capabilities[MIGRATION_CAPABILITY__MAX];
-    int64_t xbzrle_cache_size;
     int64_t setup_time;
 
     /* Flag set once the migration has been asked to enter postcopy */
diff --git a/migration/ram.c b/migration/ram.c
index ed4d3c6295..78e3b80e39 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -128,13 +128,6 @@ int xbzrle_cache_resize(int64_t new_size, Error **errp)
         return -1;
     }
 
-    /* Cache should not be larger than guest ram size */
-    if (new_size > ram_bytes_total()) {
-        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
-                   "exceeds guest ram size");
-        return -1;
-    }
-
     if (new_size == migrate_xbzrle_cache_size()) {
         /* nothing to do */
         return new_size;
diff --git a/qapi/migration.json b/qapi/migration.json
index f8b365e3f5..5fe3dd2917 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -474,6 +474,11 @@
 # @x-multifd-page-count: Number of pages sent together to a thread
 #                        The default value is 16 (since 2.11)
 #
+# @xbzrle-cache-size: cache size to be used by XBZRLE migration.  It
+#                     needs to be a multiple of the target page size
+#                     and a power of 2
+#                     (Since 2.11)
+#
 # Since: 2.4
 ##
 { 'enum': 'MigrationParameter',
@@ -481,7 +486,8 @@
            'cpu-throttle-initial', 'cpu-throttle-increment',
            'tls-creds', 'tls-hostname', 'max-bandwidth',
            'downtime-limit', 'x-checkpoint-delay', 'block-incremental',
-           'x-multifd-channels', 'x-multifd-page-count' ] }
+           'x-multifd-channels', 'x-multifd-page-count',
+           'xbzrle-cache-size' ] }
 
 ##
 # @MigrateSetParameters:
@@ -545,6 +551,10 @@
 # @x-multifd-page-count: Number of pages sent together to a thread
 #                        The default value is 16 (since 2.11)
 #
+# @xbzrle-cache-size: cache size to be used by XBZRLE migration.  It
+#                     needs to be a multiple of the target page size
+#                     and a power of 2
+#                     (Since 2.11)
 # Since: 2.4
 ##
 # TODO either fuse back into MigrationParameters, or make
@@ -562,7 +572,8 @@
             '*x-checkpoint-delay': 'int',
             '*block-incremental': 'bool',
             '*x-multifd-channels': 'int',
-            '*x-multifd-page-count': 'int' } }
+            '*x-multifd-page-count': 'int',
+            '*xbzrle-cache-size': 'size' } }
 
 ##
 # @migrate-set-parameters:
@@ -641,6 +652,10 @@
 # @x-multifd-page-count: Number of pages sent together to a thread
 #                        The default value is 16 (since 2.11)
 #
+# @xbzrle-cache-size: cache size to be used by XBZRLE migration.  It
+#                     needs to be a multiple of the target page size
+#                     and a power of 2
+#                     (Since 2.11)
 # Since: 2.4
 ##
 { 'struct': 'MigrationParameters',
@@ -656,7 +671,8 @@
             '*x-checkpoint-delay': 'int',
             '*block-incremental': 'bool' ,
             '*x-multifd-channels': 'int',
-            '*x-multifd-page-count': 'int' } }
+            '*x-multifd-page-count': 'int',
+            '*xbzrle-cache-size': 'size' } }
 
 ##
 # @query-migrate-parameters:
@@ -921,6 +937,8 @@
 #
 # Returns: nothing on success
 #
+# Notes: This command is deprecated in favor of 'migrate-set-parameters'
+#
 # Since: 1.2
 #
 # Example:
@@ -939,6 +957,8 @@
 #
 # Returns: XBZRLE cache size in bytes
 #
+# Notes: This command is deprecated in favor of 'query-migrate-parameters'
+#
 # Since: 1.2
 #
 # Example:
-- 
2.13.6

[Qemu-devel] [PATCH v2 10/10] migration: [RFC] Use proper types in json

[Juan Quintela]

We use int for everything (int64_t), and then we check that value is
between 0 and 255.  Change it to the valid types.

For qmp, the only real change is that now max_bandwidth allows to use
the k/M/G suffixes.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hmp.c                 | 22 +++++++++++-----------
 migration/migration.c | 49 ++++++++++++++++++++-----------------------------
 qapi/migration.json   | 20 ++++++++++----------
 3 files changed, 41 insertions(+), 50 deletions(-)

diff --git a/hmp.c b/hmp.c
index 22516737a9..9eead3ebe1 100644
--- a/hmp.c
+++ b/hmp.c
@@ -293,23 +293,23 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
 
     if (params) {
         assert(params->has_compress_level);
-        monitor_printf(mon, "%s: %" PRId64 "\n",
+        monitor_printf(mon, "%s: %u\n",
             MigrationParameter_str(MIGRATION_PARAMETER_COMPRESS_LEVEL),
             params->compress_level);
         assert(params->has_compress_threads);
-        monitor_printf(mon, "%s: %" PRId64 "\n",
+        monitor_printf(mon, "%s: %u\n",
             MigrationParameter_str(MIGRATION_PARAMETER_COMPRESS_THREADS),
             params->compress_threads);
         assert(params->has_decompress_threads);
-        monitor_printf(mon, "%s: %" PRId64 "\n",
+        monitor_printf(mon, "%s: %u\n",
             MigrationParameter_str(MIGRATION_PARAMETER_DECOMPRESS_THREADS),
             params->decompress_threads);
         assert(params->has_cpu_throttle_initial);
-        monitor_printf(mon, "%s: %" PRId64 "\n",
+        monitor_printf(mon, "%s: %u\n",
             MigrationParameter_str(MIGRATION_PARAMETER_CPU_THROTTLE_INITIAL),
             params->cpu_throttle_initial);
         assert(params->has_cpu_throttle_increment);
-        monitor_printf(mon, "%s: %" PRId64 "\n",
+        monitor_printf(mon, "%s: %u\n",
             MigrationParameter_str(MIGRATION_PARAMETER_CPU_THROTTLE_INCREMENT),
             params->cpu_throttle_increment);
         assert(params->has_tls_creds);
@@ -321,28 +321,28 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
             MigrationParameter_str(MIGRATION_PARAMETER_TLS_HOSTNAME),
             params->tls_hostname);
         assert(params->has_max_bandwidth);
-        monitor_printf(mon, "%s: %" PRId64 " bytes/second\n",
+        monitor_printf(mon, "%s: %" PRIu64 " bytes/second\n",
             MigrationParameter_str(MIGRATION_PARAMETER_MAX_BANDWIDTH),
             params->max_bandwidth);
         assert(params->has_downtime_limit);
-        monitor_printf(mon, "%s: %" PRId64 " milliseconds\n",
+        monitor_printf(mon, "%s: %" PRIu64 " milliseconds\n",
             MigrationParameter_str(MIGRATION_PARAMETER_DOWNTIME_LIMIT),
             params->downtime_limit);
         assert(params->has_x_checkpoint_delay);
-        monitor_printf(mon, "%s: %" PRId64 "\n",
+        monitor_printf(mon, "%s: %u\n",
             MigrationParameter_str(MIGRATION_PARAMETER_X_CHECKPOINT_DELAY),
             params->x_checkpoint_delay);
         assert(params->has_block_incremental);
         monitor_printf(mon, "%s: %s\n",
             MigrationParameter_str(MIGRATION_PARAMETER_BLOCK_INCREMENTAL),
             params->block_incremental ? "on" : "off");
-        monitor_printf(mon, "%s: %" PRId64 "\n",
+        monitor_printf(mon, "%s: %u\n",
             MigrationParameter_str(MIGRATION_PARAMETER_X_MULTIFD_CHANNELS),
             params->x_multifd_channels);
-        monitor_printf(mon, "%s: %" PRId64 "\n",
+        monitor_printf(mon, "%s: %u\n",
             MigrationParameter_str(MIGRATION_PARAMETER_X_MULTIFD_PAGE_COUNT),
             params->x_multifd_page_count);
-        monitor_printf(mon, "%s: %" PRId64 "\n",
+        monitor_printf(mon, "%s: %" PRIu64 "\n",
             MigrationParameter_str(MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE),
             params->xbzrle_cache_size);
     }
diff --git a/migration/migration.c b/migration/migration.c
index 498bf942a3..1a3dda30ca 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -734,22 +734,20 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
 static bool migrate_params_check(MigrationParameters *params, Error **errp)
 {
     if (params->has_compress_level &&
-        (params->compress_level < 0 || params->compress_level > 9)) {
+        (params->compress_level > 9)) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "compress_level",
                    "is invalid, it should be in the range of 0 to 9");
         return false;
     }
 
-    if (params->has_compress_threads &&
-        (params->compress_threads < 1 || params->compress_threads > 255)) {
+    if (params->has_compress_threads && (params->compress_threads < 1)) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                    "compress_threads",
                    "is invalid, it should be in the range of 1 to 255");
         return false;
     }
 
-    if (params->has_decompress_threads &&
-        (params->decompress_threads < 1 || params->decompress_threads > 255)) {
+    if (params->has_decompress_threads && (params->decompress_threads < 1)) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                    "decompress_threads",
                    "is invalid, it should be in the range of 1 to 255");
@@ -774,38 +772,31 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp)
         return false;
     }
 
-    if (params->has_max_bandwidth &&
-        (params->max_bandwidth < 0 || params->max_bandwidth > SIZE_MAX)) {
+    if (params->has_max_bandwidth && (params->max_bandwidth > SIZE_MAX)) {
         error_setg(errp, "Parameter 'max_bandwidth' expects an integer in the"
                          " range of 0 to %zu bytes/second", SIZE_MAX);
         return false;
     }
 
     if (params->has_downtime_limit &&
-        (params->downtime_limit < 0 ||
-         params->downtime_limit > MAX_MIGRATE_DOWNTIME)) {
+        (params->downtime_limit > MAX_MIGRATE_DOWNTIME)) {
         error_setg(errp, "Parameter 'downtime_limit' expects an integer in "
                          "the range of 0 to %d milliseconds",
                          MAX_MIGRATE_DOWNTIME);
         return false;
     }
 
-    if (params->has_x_checkpoint_delay && (params->x_checkpoint_delay < 0)) {
-        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
-                    "x_checkpoint_delay",
-                    "is invalid, it should be positive");
-        return false;
-    }
-    if (params->has_x_multifd_channels &&
-        (params->x_multifd_channels < 1 || params->x_multifd_channels > 255)) {
+    /* x_checkpoint_delay is now always positive */
+
+    if (params->has_x_multifd_channels && (params->x_multifd_channels < 1)) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                    "multifd_channels",
                    "is invalid, it should be in the range of 1 to 255");
         return false;
     }
     if (params->has_x_multifd_page_count &&
-            (params->x_multifd_page_count < 1 ||
-             params->x_multifd_page_count > 10000)) {
+        (params->x_multifd_page_count < 1 ||
+         params->x_multifd_page_count > 10000)) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                    "multifd_page_count",
                    "is invalid, it should be in the range of 1 to 10000");
@@ -2301,33 +2292,33 @@ static Property migration_properties[] = {
                      send_section_footer, true),
 
     /* Migration parameters */
-    DEFINE_PROP_INT64("x-compress-level", MigrationState,
+    DEFINE_PROP_UINT8("x-compress-level", MigrationState,
                       parameters.compress_level,
                       DEFAULT_MIGRATE_COMPRESS_LEVEL),
-    DEFINE_PROP_INT64("x-compress-threads", MigrationState,
+    DEFINE_PROP_UINT8("x-compress-threads", MigrationState,
                       parameters.compress_threads,
                       DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT),
-    DEFINE_PROP_INT64("x-decompress-threads", MigrationState,
+    DEFINE_PROP_UINT8("x-decompress-threads", MigrationState,
                       parameters.decompress_threads,
                       DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT),
-    DEFINE_PROP_INT64("x-cpu-throttle-initial", MigrationState,
+    DEFINE_PROP_UINT8("x-cpu-throttle-initial", MigrationState,
                       parameters.cpu_throttle_initial,
                       DEFAULT_MIGRATE_CPU_THROTTLE_INITIAL),
-    DEFINE_PROP_INT64("x-cpu-throttle-increment", MigrationState,
+    DEFINE_PROP_UINT8("x-cpu-throttle-increment", MigrationState,
                       parameters.cpu_throttle_increment,
                       DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT),
-    DEFINE_PROP_INT64("x-max-bandwidth", MigrationState,
+    DEFINE_PROP_SIZE("x-max-bandwidth", MigrationState,
                       parameters.max_bandwidth, MAX_THROTTLE),
-    DEFINE_PROP_INT64("x-downtime-limit", MigrationState,
+    DEFINE_PROP_UINT64("x-downtime-limit", MigrationState,
                       parameters.downtime_limit,
                       DEFAULT_MIGRATE_SET_DOWNTIME),
-    DEFINE_PROP_INT64("x-checkpoint-delay", MigrationState,
+    DEFINE_PROP_UINT32("x-checkpoint-delay", MigrationState,
                       parameters.x_checkpoint_delay,
                       DEFAULT_MIGRATE_X_CHECKPOINT_DELAY),
-    DEFINE_PROP_INT64("x-multifd-channels", MigrationState,
+    DEFINE_PROP_UINT8("x-multifd-channels", MigrationState,
                       parameters.x_multifd_channels,
                       DEFAULT_MIGRATE_MULTIFD_CHANNELS),
-    DEFINE_PROP_INT64("x-multifd-page-count", MigrationState,
+    DEFINE_PROP_UINT32("x-multifd-page-count", MigrationState,
                       parameters.x_multifd_page_count,
                       DEFAULT_MIGRATE_MULTIFD_PAGE_COUNT),
     DEFINE_PROP_SIZE("xbzrle-cache-size", MigrationState,
diff --git a/qapi/migration.json b/qapi/migration.json
index 5fe3dd2917..1671468310 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -659,19 +659,19 @@
 # Since: 2.4
 ##
 { 'struct': 'MigrationParameters',
-  'data': { '*compress-level': 'int',
-            '*compress-threads': 'int',
-            '*decompress-threads': 'int',
-            '*cpu-throttle-initial': 'int',
-            '*cpu-throttle-increment': 'int',
+  'data': { '*compress-level': 'uint8',
+            '*compress-threads': 'uint8',
+            '*decompress-threads': 'uint8',
+            '*cpu-throttle-initial': 'uint8',
+            '*cpu-throttle-increment': 'uint8',
             '*tls-creds': 'str',
             '*tls-hostname': 'str',
-            '*max-bandwidth': 'int',
-            '*downtime-limit': 'int',
-            '*x-checkpoint-delay': 'int',
+            '*max-bandwidth': 'size',
+            '*downtime-limit': 'uint64',
+            '*x-checkpoint-delay': 'uint32',
             '*block-incremental': 'bool' ,
-            '*x-multifd-channels': 'int',
-            '*x-multifd-page-count': 'int',
+            '*x-multifd-channels': 'uint8',
+            '*x-multifd-page-count': 'uint32',
             '*xbzrle-cache-size': 'size' } }
 
 ##
-- 
2.13.6

Re: [Qemu-devel] [PATCH v2 06/10] migration: Make sure that we pass the right cache size

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> Instead of passing silently round down the number of pages, make it an
> error that the cache size is not a power of 2.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

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

> ---
>  migration/page_cache.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/migration/page_cache.c b/migration/page_cache.c
> index 9a9d13d6a2..96268c3aea 100644
> --- a/migration/page_cache.c
> +++ b/migration/page_cache.c
> @@ -58,6 +58,13 @@ PageCache *cache_init(int64_t new_size, size_t page_size, Error **errp)
>          return NULL;
>      }
>  
> +    /* round down to the nearest power of 2 */
> +    if (!is_power_of_2(num_pages)) {
> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
> +                   "is not a power of two number of pages");
> +        return NULL;
> +    }
> +
>      /* We prefer not to abort if there is no memory */
>      cache = g_try_malloc(sizeof(*cache));
>      if (!cache) {
> @@ -65,11 +72,6 @@ PageCache *cache_init(int64_t new_size, size_t page_size, Error **errp)
>                     "Failed to allocate cache");
>          return NULL;
>      }
> -    /* round down to the nearest power of 2 */
> -    if (!is_power_of_2(num_pages)) {
> -        num_pages = pow2floor(num_pages);
> -        DPRINTF("rounding down to %" PRId64 "\n", num_pages);
> -    }
>      cache->page_size = page_size;
>      cache->num_items = 0;
>      cache->max_num_items = num_pages;
> -- 
> 2.13.6
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 07/10] migration: Don't play games with the requested cache size

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> Now that we check that the value passed is a power of 2, we don't need
> to play games when comparing what is the size that is going to take
> the cache.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/ram.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 47501460c8..c84f22d759 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -135,12 +135,14 @@ int64_t xbzrle_cache_resize(int64_t new_size, Error **errp)
>          return -1;
>      }
>  
> +    if (new_size == migrate_xbzrle_cache_size()) {
> +        /* nothing to do */
> +        return new_size;
> +    }
> +

OK, that's interesting - this test is before the XBZRLE != NULL check;
so the old code would cause allocation if there was no cache;  but I
think ram_state_init makes that irrelevant, so we're OK.

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

>      XBZRLE_cache_lock();
>  
>      if (XBZRLE.cache != NULL) {
> -        if (pow2floor(new_size) == migrate_xbzrle_cache_size()) {
> -            goto out_new_size;
> -        }
>          new_cache = cache_init(new_size, TARGET_PAGE_SIZE, errp);
>          if (!new_cache) {
>              ret = -1;
> @@ -151,8 +153,7 @@ int64_t xbzrle_cache_resize(int64_t new_size, Error **errp)
>          XBZRLE.cache = new_cache;
>      }
>  
> -out_new_size:
> -    ret = pow2floor(new_size);
> +    ret = new_size;
>  out:
>      XBZRLE_cache_unlock();
>      return ret;
> -- 
> 2.13.6
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 08/10] migration: No need to return the size of the cache

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> After the previous commits, we make sure that the value passed is
> right, or we just drop an error.  So now we return if there is one
> error or we have setup correctly the value passed.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/migration.c | 6 ++----
>  migration/ram.c       | 8 +++-----
>  migration/ram.h       | 2 +-
>  3 files changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 3feffb5e26..f3d4503ce2 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1371,14 +1371,12 @@ 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;
>  
> -    new_size = xbzrle_cache_resize(value, errp);
> -    if (new_size < 0) {
> +    if (xbzrle_cache_resize(value, errp) < 0) {

That's not consistent with the function below; it returns
'0 or negative' for error; this check only tests for negative
as an error.

Dave

>          return;
>      }
>  
> -    s->xbzrle_cache_size = new_size;
> +    s->xbzrle_cache_size = value;
>  }
>  
>  int64_t qmp_query_migrate_cache_size(Error **errp)
> diff --git a/migration/ram.c b/migration/ram.c
> index c84f22d759..ed4d3c6295 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -111,15 +111,15 @@ static void XBZRLE_cache_unlock(void)
>   * migration may be using the cache and might finish during this call,
>   * hence changes to the cache are protected by XBZRLE.lock().
>   *
> - * Returns the new_size or negative in case of error.
> + * Returns the 0 or negative in case of error.
>   *
>   * @new_size: new cache size
>   * @errp: set *errp if the check failed, with reason
>   */
> -int64_t xbzrle_cache_resize(int64_t new_size, Error **errp)
> +int xbzrle_cache_resize(int64_t new_size, Error **errp)
>  {
>      PageCache *new_cache;
> -    int64_t ret;
> +    int64_t ret = 0;
>  
>      /* Check for truncation */
>      if (new_size != (size_t)new_size) {
> @@ -152,8 +152,6 @@ int64_t xbzrle_cache_resize(int64_t new_size, Error **errp)
>          cache_fini(XBZRLE.cache);
>          XBZRLE.cache = new_cache;
>      }
> -
> -    ret = new_size;
>  out:
>      XBZRLE_cache_unlock();
>      return ret;
> diff --git a/migration/ram.h b/migration/ram.h
> index 511b3dc582..c8ae382b5b 100644
> --- a/migration/ram.h
> +++ b/migration/ram.h
> @@ -35,7 +35,7 @@
>  extern MigrationStats ram_counters;
>  extern XBZRLECacheStats xbzrle_counters;
>  
> -int64_t xbzrle_cache_resize(int64_t new_size, Error **errp);
> +int xbzrle_cache_resize(int64_t new_size, Error **errp);
>  uint64_t ram_bytes_remaining(void);
>  uint64_t ram_bytes_total(void);
>  
> -- 
> 2.13.6
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 09/10] migration: Make xbzrle_cache_size a migration parameter

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> Right now it is a variable in MigrationState instead of a
> MigrationParameter.  The change allows to set it as the rest of the
> Migration parameters, from the command line, with
> query_migration_paramters, set_migrate_parameters, etc.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

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

> 
> --
> 
> - Improve help message (dave)
> - Use errp that we already have around instead of NULL (dave)
> ---
>  hmp.c                 | 14 ++++++++++++++
>  migration/migration.c | 43 ++++++++++++++++++++++++++++++++-----------
>  migration/migration.h |  1 -
>  migration/ram.c       |  7 -------
>  qapi/migration.json   | 26 +++++++++++++++++++++++---
>  5 files changed, 69 insertions(+), 22 deletions(-)
> 
> diff --git a/hmp.c b/hmp.c
> index ec61329ebb..22516737a9 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -342,6 +342,9 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
>          monitor_printf(mon, "%s: %" PRId64 "\n",
>              MigrationParameter_str(MIGRATION_PARAMETER_X_MULTIFD_PAGE_COUNT),
>              params->x_multifd_page_count);
> +        monitor_printf(mon, "%s: %" PRId64 "\n",
> +            MigrationParameter_str(MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE),
> +            params->xbzrle_cache_size);
>      }
>  
>      qapi_free_MigrationParameters(params);
> @@ -1565,6 +1568,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>      Visitor *v = string_input_visitor_new(valuestr);
>      MigrateSetParameters *p = g_new0(MigrateSetParameters, 1);
>      uint64_t valuebw = 0;
> +    uint64_t cache_size;
>      Error *err = NULL;
>      int val, ret;
>  
> @@ -1640,6 +1644,16 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>          p->has_x_multifd_page_count = true;
>          visit_type_int(v, param, &p->x_multifd_page_count, &err);
>          break;
> +    case MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE:
> +        p->has_xbzrle_cache_size = true;
> +        visit_type_size(v, param, &cache_size, &err);
> +        if (err || cache_size > INT64_MAX
> +            || (size_t)cache_size != cache_size) {
> +            error_setg(&err, "Invalid size %s", valuestr);
> +            break;
> +        }
> +        p->xbzrle_cache_size = cache_size;
> +        break;
>      default:
>          assert(0);
>      }
> diff --git a/migration/migration.c b/migration/migration.c
> index f3d4503ce2..498bf942a3 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -71,7 +71,7 @@
>  #define DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT 10
>  
>  /* Migration XBZRLE default cache size */
> -#define DEFAULT_MIGRATE_CACHE_SIZE (64 * 1024 * 1024)
> +#define DEFAULT_MIGRATE_XBZRLE_CACHE_SIZE (64 * 1024 * 1024)
>  
>  /* The delay time (in ms) between two COLO checkpoints
>   * Note: Please change this default value to 10000 when we support hybrid mode.
> @@ -512,6 +512,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>      params->x_multifd_channels = s->parameters.x_multifd_channels;
>      params->has_x_multifd_page_count = true;
>      params->x_multifd_page_count = s->parameters.x_multifd_page_count;
> +    params->has_xbzrle_cache_size = true;
> +    params->xbzrle_cache_size = s->parameters.xbzrle_cache_size;
>  
>      return params;
>  }
> @@ -810,6 +812,16 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp)
>          return false;
>      }
>  
> +    if (params->has_xbzrle_cache_size &&
> +        (params->xbzrle_cache_size < qemu_target_page_size() ||
> +         !is_power_of_2(params->xbzrle_cache_size))) {
> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> +                   "xbzrle_cache_size",
> +                   "is invalid, it should be bigger than target page size"
> +                   " and a power of two");
> +        return false;
> +    }
> +
>      return true;
>  }
>  
> @@ -871,9 +883,12 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
>      if (params->has_x_multifd_page_count) {
>          dest->x_multifd_page_count = params->x_multifd_page_count;
>      }
> +    if (params->has_xbzrle_cache_size) {
> +        dest->xbzrle_cache_size = params->xbzrle_cache_size;
> +    }
>  }
>  
> -static void migrate_params_apply(MigrateSetParameters *params)
> +static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
>  {
>      MigrationState *s = migrate_get_current();
>  
> @@ -939,6 +954,10 @@ static void migrate_params_apply(MigrateSetParameters *params)
>      if (params->has_x_multifd_page_count) {
>          s->parameters.x_multifd_page_count = params->x_multifd_page_count;
>      }
> +    if (params->has_xbzrle_cache_size) {
> +        s->parameters.xbzrle_cache_size = params->xbzrle_cache_size;
> +        xbzrle_cache_resize(params->xbzrle_cache_size, errp);
> +    }
>  }
>  
>  void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
> @@ -967,7 +986,7 @@ void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
>          return;
>      }
>  
> -    migrate_params_apply(params);
> +    migrate_params_apply(params, errp);
>  }
>  
>  
> @@ -1370,13 +1389,12 @@ void qmp_migrate_cancel(Error **errp)
>  
>  void qmp_migrate_set_cache_size(int64_t value, Error **errp)
>  {
> -    MigrationState *s = migrate_get_current();
> +    MigrateSetParameters p = {
> +        .has_xbzrle_cache_size = true,
> +        .xbzrle_cache_size = value,
> +    };
>  
> -    if (xbzrle_cache_resize(value, errp) < 0) {
> -        return;
> -    }
> -
> -    s->xbzrle_cache_size = value;
> +    qmp_migrate_set_parameters(&p, errp);
>  }
>  
>  int64_t qmp_query_migrate_cache_size(Error **errp)
> @@ -1542,7 +1560,7 @@ int64_t migrate_xbzrle_cache_size(void)
>  
>      s = migrate_get_current();
>  
> -    return s->xbzrle_cache_size;
> +    return s->parameters.xbzrle_cache_size;
>  }
>  
>  bool migrate_use_block(void)
> @@ -2312,6 +2330,9 @@ static Property migration_properties[] = {
>      DEFINE_PROP_INT64("x-multifd-page-count", MigrationState,
>                        parameters.x_multifd_page_count,
>                        DEFAULT_MIGRATE_MULTIFD_PAGE_COUNT),
> +    DEFINE_PROP_SIZE("xbzrle-cache-size", MigrationState,
> +                      parameters.xbzrle_cache_size,
> +                      DEFAULT_MIGRATE_XBZRLE_CACHE_SIZE),
>  
>      /* Migration capabilities */
>      DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE),
> @@ -2353,7 +2374,6 @@ static void migration_instance_init(Object *obj)
>      MigrationParameters *params = &ms->parameters;
>  
>      ms->state = MIGRATION_STATUS_NONE;
> -    ms->xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE;
>      ms->mbps = -1;
>  
>      params->tls_hostname = g_strdup("");
> @@ -2371,6 +2391,7 @@ static void migration_instance_init(Object *obj)
>      params->has_block_incremental = true;
>      params->has_x_multifd_channels = true;
>      params->has_x_multifd_page_count = true;
> +    params->has_xbzrle_cache_size = true;
>  }
>  
>  /*
> diff --git a/migration/migration.h b/migration/migration.h
> index b83cceadc4..d2a8d319f1 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -107,7 +107,6 @@ struct MigrationState
>      int64_t downtime;
>      int64_t expected_downtime;
>      bool enabled_capabilities[MIGRATION_CAPABILITY__MAX];
> -    int64_t xbzrle_cache_size;
>      int64_t setup_time;
>  
>      /* Flag set once the migration has been asked to enter postcopy */
> diff --git a/migration/ram.c b/migration/ram.c
> index ed4d3c6295..78e3b80e39 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -128,13 +128,6 @@ int xbzrle_cache_resize(int64_t new_size, Error **errp)
>          return -1;
>      }
>  
> -    /* Cache should not be larger than guest ram size */
> -    if (new_size > ram_bytes_total()) {
> -        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
> -                   "exceeds guest ram size");
> -        return -1;
> -    }
> -
>      if (new_size == migrate_xbzrle_cache_size()) {
>          /* nothing to do */
>          return new_size;
> diff --git a/qapi/migration.json b/qapi/migration.json
> index f8b365e3f5..5fe3dd2917 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -474,6 +474,11 @@
>  # @x-multifd-page-count: Number of pages sent together to a thread
>  #                        The default value is 16 (since 2.11)
>  #
> +# @xbzrle-cache-size: cache size to be used by XBZRLE migration.  It
> +#                     needs to be a multiple of the target page size
> +#                     and a power of 2
> +#                     (Since 2.11)
> +#
>  # Since: 2.4
>  ##
>  { 'enum': 'MigrationParameter',
> @@ -481,7 +486,8 @@
>             'cpu-throttle-initial', 'cpu-throttle-increment',
>             'tls-creds', 'tls-hostname', 'max-bandwidth',
>             'downtime-limit', 'x-checkpoint-delay', 'block-incremental',
> -           'x-multifd-channels', 'x-multifd-page-count' ] }
> +           'x-multifd-channels', 'x-multifd-page-count',
> +           'xbzrle-cache-size' ] }
>  
>  ##
>  # @MigrateSetParameters:
> @@ -545,6 +551,10 @@
>  # @x-multifd-page-count: Number of pages sent together to a thread
>  #                        The default value is 16 (since 2.11)
>  #
> +# @xbzrle-cache-size: cache size to be used by XBZRLE migration.  It
> +#                     needs to be a multiple of the target page size
> +#                     and a power of 2
> +#                     (Since 2.11)
>  # Since: 2.4
>  ##
>  # TODO either fuse back into MigrationParameters, or make
> @@ -562,7 +572,8 @@
>              '*x-checkpoint-delay': 'int',
>              '*block-incremental': 'bool',
>              '*x-multifd-channels': 'int',
> -            '*x-multifd-page-count': 'int' } }
> +            '*x-multifd-page-count': 'int',
> +            '*xbzrle-cache-size': 'size' } }
>  
>  ##
>  # @migrate-set-parameters:
> @@ -641,6 +652,10 @@
>  # @x-multifd-page-count: Number of pages sent together to a thread
>  #                        The default value is 16 (since 2.11)
>  #
> +# @xbzrle-cache-size: cache size to be used by XBZRLE migration.  It
> +#                     needs to be a multiple of the target page size
> +#                     and a power of 2
> +#                     (Since 2.11)
>  # Since: 2.4
>  ##
>  { 'struct': 'MigrationParameters',
> @@ -656,7 +671,8 @@
>              '*x-checkpoint-delay': 'int',
>              '*block-incremental': 'bool' ,
>              '*x-multifd-channels': 'int',
> -            '*x-multifd-page-count': 'int' } }
> +            '*x-multifd-page-count': 'int',
> +            '*xbzrle-cache-size': 'size' } }
>  
>  ##
>  # @query-migrate-parameters:
> @@ -921,6 +937,8 @@
>  #
>  # Returns: nothing on success
>  #
> +# Notes: This command is deprecated in favor of 'migrate-set-parameters'
> +#
>  # Since: 1.2
>  #
>  # Example:
> @@ -939,6 +957,8 @@
>  #
>  # Returns: XBZRLE cache size in bytes
>  #
> +# Notes: This command is deprecated in favor of 'query-migrate-parameters'
> +#
>  # Since: 1.2
>  #
>  # Example:
> -- 
> 2.13.6
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 08/10] migration: No need to return the size of the cache

[Juan Quintela]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
>> After the previous commits, we make sure that the value passed is
>> right, or we just drop an error.  So now we return if there is one
>> error or we have setup correctly the value passed.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  migration/migration.c | 6 ++----
>>  migration/ram.c       | 8 +++-----
>>  migration/ram.h       | 2 +-
>>  3 files changed, 6 insertions(+), 10 deletions(-)
>> 
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 3feffb5e26..f3d4503ce2 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -1371,14 +1371,12 @@ 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;
>>  
>> -    new_size = xbzrle_cache_resize(value, errp);
>> -    if (new_size < 0) {
>> +    if (xbzrle_cache_resize(value, errp) < 0) {
>
> That's not consistent with the function below; it returns
> '0 or negative' for error; this check only tests for negative
> as an error.

int xbzrle_cache_resize(int64_t new_size, Error **errp)
{
    PageCache *new_cache;
    int64_t ret = 0;

    /* Check for truncation */
    if (new_size != (size_t)new_size) {
        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
                   "exceeding address space");
        return -1;
    }

    if (new_size == migrate_xbzrle_cache_size()) {
        /* nothing to do */
        return new_size;
    }

    XBZRLE_cache_lock();

    if (XBZRLE.cache != NULL) {
        new_cache = cache_init(new_size, TARGET_PAGE_SIZE, errp);
        if (!new_cache) {
            ret = -1;
            goto out;
        }

        cache_fini(XBZRLE.cache);
        XBZRLE.cache = new_cache;
    }
out:
    XBZRLE_cache_unlock();
    return ret;
}


That is the function that we end having.
-1 means error
0 means success (yes, I should have changed the return new_size to
return 0).

I *think* it is ok.  Notice that this is after we have changed all users
to know that it returns an error or set the value that it has been
passed.

Later, Juan.


>
> Dave
>
>>          return;
>>      }
>>  
>> -    s->xbzrle_cache_size = new_size;
>> +    s->xbzrle_cache_size = value;
>>  }
>>  
>>  int64_t qmp_query_migrate_cache_size(Error **errp)
>> diff --git a/migration/ram.c b/migration/ram.c
>> index c84f22d759..ed4d3c6295 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -111,15 +111,15 @@ static void XBZRLE_cache_unlock(void)
>>   * migration may be using the cache and might finish during this call,
>>   * hence changes to the cache are protected by XBZRLE.lock().
>>   *
>> - * Returns the new_size or negative in case of error.
>> + * Returns the 0 or negative in case of error.
>>   *
>>   * @new_size: new cache size
>>   * @errp: set *errp if the check failed, with reason
>>   */
>> -int64_t xbzrle_cache_resize(int64_t new_size, Error **errp)
>> +int xbzrle_cache_resize(int64_t new_size, Error **errp)
>>  {
>>      PageCache *new_cache;
>> -    int64_t ret;
>> +    int64_t ret = 0;
>>  
>>      /* Check for truncation */
>>      if (new_size != (size_t)new_size) {
>> @@ -152,8 +152,6 @@ int64_t xbzrle_cache_resize(int64_t new_size, Error **errp)
>>          cache_fini(XBZRLE.cache);
>>          XBZRLE.cache = new_cache;
>>      }
>> -
>> -    ret = new_size;
>>  out:
>>      XBZRLE_cache_unlock();
>>      return ret;
>> diff --git a/migration/ram.h b/migration/ram.h
>> index 511b3dc582..c8ae382b5b 100644
>> --- a/migration/ram.h
>> +++ b/migration/ram.h
>> @@ -35,7 +35,7 @@
>>  extern MigrationStats ram_counters;
>>  extern XBZRLECacheStats xbzrle_counters;
>>  
>> -int64_t xbzrle_cache_resize(int64_t new_size, Error **errp);
>> +int xbzrle_cache_resize(int64_t new_size, Error **errp);
>>  uint64_t ram_bytes_remaining(void);
>>  uint64_t ram_bytes_total(void);
>>  
>> -- 
>> 2.13.6
>> 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 08/10] migration: No need to return the size of the cache

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> > * Juan Quintela (quintela@redhat.com) wrote:
> >> After the previous commits, we make sure that the value passed is
> >> right, or we just drop an error.  So now we return if there is one
> >> error or we have setup correctly the value passed.
> >> 
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> ---
> >>  migration/migration.c | 6 ++----
> >>  migration/ram.c       | 8 +++-----
> >>  migration/ram.h       | 2 +-
> >>  3 files changed, 6 insertions(+), 10 deletions(-)
> >> 
> >> diff --git a/migration/migration.c b/migration/migration.c
> >> index 3feffb5e26..f3d4503ce2 100644
> >> --- a/migration/migration.c
> >> +++ b/migration/migration.c
> >> @@ -1371,14 +1371,12 @@ 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;
> >>  
> >> -    new_size = xbzrle_cache_resize(value, errp);
> >> -    if (new_size < 0) {
> >> +    if (xbzrle_cache_resize(value, errp) < 0) {
> >
> > That's not consistent with the function below; it returns
> > '0 or negative' for error; this check only tests for negative
> > as an error.
> 
> int xbzrle_cache_resize(int64_t new_size, Error **errp)
> {
>     PageCache *new_cache;
>     int64_t ret = 0;
> 
>     /* Check for truncation */
>     if (new_size != (size_t)new_size) {
>         error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cache size",
>                    "exceeding address space");
>         return -1;
>     }
> 
>     if (new_size == migrate_xbzrle_cache_size()) {
>         /* nothing to do */
>         return new_size;
>     }
> 
>     XBZRLE_cache_lock();
> 
>     if (XBZRLE.cache != NULL) {
>         new_cache = cache_init(new_size, TARGET_PAGE_SIZE, errp);
>         if (!new_cache) {
>             ret = -1;
>             goto out;
>         }
> 
>         cache_fini(XBZRLE.cache);
>         XBZRLE.cache = new_cache;
>     }
> out:
>     XBZRLE_cache_unlock();
>     return ret;
> }
> 
> 
> That is the function that we end having.
> -1 means error
> 0 means success (yes, I should have changed the return new_size to
> return 0).
> 
> I *think* it is ok.  Notice that this is after we have changed all users
> to know that it returns an error or set the value that it has been
> passed.

There's two problems with that function (as shown):
  a) The comment (which isn't shown) is:
       + * Returns the 0 or negative in case of error.

     so the comment says 0 means error which doesn't match your check

  b) In the case of XBZRLE.cache == NULL; you end up returning 0 - that
  feels like an error?

Dave

> Later, Juan.
> 
> 
> >
> > Dave
> >
> >>          return;
> >>      }
> >>  
> >> -    s->xbzrle_cache_size = new_size;
> >> +    s->xbzrle_cache_size = value;
> >>  }
> >>  
> >>  int64_t qmp_query_migrate_cache_size(Error **errp)
> >> diff --git a/migration/ram.c b/migration/ram.c
> >> index c84f22d759..ed4d3c6295 100644
> >> --- a/migration/ram.c
> >> +++ b/migration/ram.c
> >> @@ -111,15 +111,15 @@ static void XBZRLE_cache_unlock(void)
> >>   * migration may be using the cache and might finish during this call,
> >>   * hence changes to the cache are protected by XBZRLE.lock().
> >>   *
> >> - * Returns the new_size or negative in case of error.
> >> + * Returns the 0 or negative in case of error.
> >>   *
> >>   * @new_size: new cache size
> >>   * @errp: set *errp if the check failed, with reason
> >>   */
> >> -int64_t xbzrle_cache_resize(int64_t new_size, Error **errp)
> >> +int xbzrle_cache_resize(int64_t new_size, Error **errp)
> >>  {
> >>      PageCache *new_cache;
> >> -    int64_t ret;
> >> +    int64_t ret = 0;
> >>  
> >>      /* Check for truncation */
> >>      if (new_size != (size_t)new_size) {
> >> @@ -152,8 +152,6 @@ int64_t xbzrle_cache_resize(int64_t new_size, Error **errp)
> >>          cache_fini(XBZRLE.cache);
> >>          XBZRLE.cache = new_cache;
> >>      }
> >> -
> >> -    ret = new_size;
> >>  out:
> >>      XBZRLE_cache_unlock();
> >>      return ret;
> >> diff --git a/migration/ram.h b/migration/ram.h
> >> index 511b3dc582..c8ae382b5b 100644
> >> --- a/migration/ram.h
> >> +++ b/migration/ram.h
> >> @@ -35,7 +35,7 @@
> >>  extern MigrationStats ram_counters;
> >>  extern XBZRLECacheStats xbzrle_counters;
> >>  
> >> -int64_t xbzrle_cache_resize(int64_t new_size, Error **errp);
> >> +int xbzrle_cache_resize(int64_t new_size, Error **errp);
> >>  uint64_t ram_bytes_remaining(void);
> >>  uint64_t ram_bytes_total(void);
> >>  
> >> -- 
> >> 2.13.6
> >> 
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK