[Qemu-devel] [PATCH v3 0/5] Make xbzrle_cache_size a migration parameter

[Qemu-devel] [PATCH v3 0/5] Make xbzrle_cache_size a migration parameter

[Juan Quintela]

Hi

On v3:
- rebase on top of last migration pull requset
- improve comments (dave suggestion)
- always return 0 in case of error
- checked with ovirt/openstack/libvirt people that making value a
  power of two is not a problem.
- should I drop the last patch, or sent it?


Please revieww.

[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 (5):
  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  | 94 ++++++++++++++++++++++++++++----------------------
 migration/migration.h  |  1 -
 migration/page_cache.c | 12 ++++---
 migration/ram.c        | 20 ++++-------
 migration/ram.h        |  2 +-
 qapi/migration.json    | 44 ++++++++++++++++-------
 7 files changed, 122 insertions(+), 85 deletions(-)

-- 
2.13.6

[Qemu-devel] [PATCH v3 1/5] 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>
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

[Qemu-devel] [PATCH v3 2/5] 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>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/ram.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 7f6327f708..42f3b7cb28 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -136,12 +136,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;
@@ -152,8 +154,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 v3 3/5] 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>

--

Improve error messasge
Return 0 always for success
---
 migration/migration.c |  6 ++----
 migration/ram.c       | 10 ++++------
 migration/ram.h       |  2 +-
 3 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 62761d5705..6bbd4715d3 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1406,14 +1406,12 @@ void qmp_migrate_continue(MigrationStatus state, 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 42f3b7cb28..997340c7c2 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -112,15 +112,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 0 for success or -1 for 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) {
@@ -138,7 +138,7 @@ int64_t xbzrle_cache_resize(int64_t new_size, Error **errp)
 
     if (new_size == migrate_xbzrle_cache_size()) {
         /* nothing to do */
-        return new_size;
+        return 0;
     }
 
     XBZRLE_cache_lock();
@@ -153,8 +153,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 f9f7eef894..64d81e9f1d 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 v3 4/5] 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>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 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 41fcce6f5a..a01be50daa 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);
@@ -1578,6 +1581,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;
 
@@ -1653,6 +1657,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 6bbd4715d3..4de3b551fe 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.
@@ -515,6 +515,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;
 }
@@ -817,6 +819,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;
 }
 
@@ -878,9 +890,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();
 
@@ -946,6 +961,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)
@@ -974,7 +993,7 @@ void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
         return;
     }
 
-    migrate_params_apply(params);
+    migrate_params_apply(params, errp);
 }
 
 
@@ -1405,13 +1424,12 @@ void qmp_migrate_continue(MigrationStatus state, 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)
@@ -1587,7 +1605,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)
@@ -2405,6 +2423,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),
@@ -2448,7 +2469,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;
     qemu_sem_init(&ms->pause_sem, 0);
     qemu_mutex_init(&ms->error_mutex);
@@ -2468,6 +2488,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 8ccdd7a577..663415fe48 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 997340c7c2..8620aa400a 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -129,13 +129,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 0;
diff --git a/qapi/migration.json b/qapi/migration.json
index 6ae866e1aa..bbc4671ded 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -483,6 +483,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',
@@ -490,7 +495,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:
@@ -554,6 +560,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
@@ -571,7 +581,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:
@@ -650,6 +661,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',
@@ -665,7 +680,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:
@@ -947,6 +963,8 @@
 #
 # Returns: nothing on success
 #
+# Notes: This command is deprecated in favor of 'migrate-set-parameters'
+#
 # Since: 1.2
 #
 # Example:
@@ -965,6 +983,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 v3 5/5] 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 a01be50daa..7670f102c3 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 4de3b551fe..8d2372394c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -741,22 +741,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");
@@ -781,38 +779,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");
@@ -2394,33 +2385,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 bbc4671ded..ca4563dbd7 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -668,19 +668,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 v3 3/5] 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>
> 
> --
> 
> Improve error messasge
> Return 0 always for success
> ---
>  migration/migration.c |  6 ++----
>  migration/ram.c       | 10 ++++------
>  migration/ram.h       |  2 +-
>  3 files changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 62761d5705..6bbd4715d3 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1406,14 +1406,12 @@ void qmp_migrate_continue(MigrationStatus state, 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 42f3b7cb28..997340c7c2 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -112,15 +112,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 0 for success or -1 for 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) {
> @@ -138,7 +138,7 @@ int64_t xbzrle_cache_resize(int64_t new_size, Error **errp)
>  
>      if (new_size == migrate_xbzrle_cache_size()) {
>          /* nothing to do */
> -        return new_size;
> +        return 0;
>      }
>  
>      XBZRLE_cache_lock();
> @@ -153,8 +153,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;

OK, this shares the same oddity as the original which is
  XBZRLE.cache = NULL    is not an error.

but, since that is the same as the original;


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

> diff --git a/migration/ram.h b/migration/ram.h
> index f9f7eef894..64d81e9f1d 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 v3 3/5] 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>

> OK, this shares the same oddity as the original which is
>   XBZRLE.cache = NULL    is not an error.
>
> but, since that is the same as the original;

Hi

It is a normal case.  You can change the cache size in the middle of
migration or outside migration.

If we are not in the middle of migration, cache is NULL.

Later, Juan.

Re: [Qemu-devel] [PATCH v3 3/5] 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>
> 
> > OK, this shares the same oddity as the original which is
> >   XBZRLE.cache = NULL    is not an error.
> >
> > but, since that is the same as the original;
> 
> Hi
> 
> It is a normal case.  You can change the cache size in the middle of
> migration or outside migration.
> 
> If we are not in the middle of migration, cache is NULL.

Ah OK, so yes,


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

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