[Qemu-devel] [PATCH v5] migrate: Move max-bandwidth and downtime-limit to migrate_set_parameter

[Qemu-devel] [PATCH v5] migrate: Move max-bandwidth and downtime-limit to migrate_set_parameter

[Ashijeet Acharya]

Mark old-commands for speed and downtime as deprecated.
Move max-bandwidth and downtime-limit into migrate-set-parameters for
setting maximum migration speed and expected downtime limit parameters
respectively.
Change downtime units to milliseconds (only for new-command) and update
the query part in both hmp and qmp qemu control interfaces.
NOTE: This patch is solely based on Eric's new boxed parameters from QAPI
patch series.

Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
---
Changes in v5:
- Include units in the output of 'info migrte_parameters'
- Change 'old-commands' to 'backwards' in deprecation comments
- Include a error check for overflow of downtime_limit value
- Solve long line issues
---
 hmp.c                         | 27 +++++++++++++++
 include/migration/migration.h |  1 -
 migration/migration.c         | 76 +++++++++++++++++++++++++++++++------------
 qapi-schema.json              | 23 +++++++++++--
 qmp-commands.hx               | 14 ++++++--
 5 files changed, 113 insertions(+), 28 deletions(-)

diff --git a/hmp.c b/hmp.c
index 0893b8e..f9c81b1 100644
--- a/hmp.c
+++ b/hmp.c
@@ -309,6 +309,14 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
         monitor_printf(mon, " %s: '%s'",
             MigrationParameter_lookup[MIGRATION_PARAMETER_TLS_HOSTNAME],
             params->has_tls_hostname ? params->tls_hostname : "");
+        assert(params->has_max_bandwidth);
+        monitor_printf(mon, " %s: %" PRId64 " bytes/second",
+            MigrationParameter_lookup[MIGRATION_PARAMETER_MAX_BANDWIDTH],
+            params->max_bandwidth);
+        assert(params->has_downtime_limit);
+        monitor_printf(mon, " %s: %" PRId64 " milliseconds",
+            MigrationParameter_lookup[MIGRATION_PARAMETER_DOWNTIME_LIMIT],
+            params->downtime_limit);
         monitor_printf(mon, "\n");
     }
 
@@ -1200,6 +1208,7 @@ void hmp_migrate_incoming(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, &err);
 }
 
+/* Kept for backwards compatibility */
 void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict)
 {
     double value = qdict_get_double(qdict, "value");
@@ -1218,6 +1227,7 @@ void hmp_migrate_set_cache_size(Monitor *mon, const QDict *qdict)
     }
 }
 
+/* Kept for backwards compatibility */
 void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict)
 {
     int64_t value = qdict_get_int(qdict, "value");
@@ -1258,7 +1268,9 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
 {
     const char *param = qdict_get_str(qdict, "parameter");
     const char *valuestr = qdict_get_str(qdict, "value");
+    int64_t valuebw = 0;
     long valueint = 0;
+    char *endp;
     Error *err = NULL;
     bool use_int_value = false;
     int i;
@@ -1295,6 +1307,20 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
                 p.has_tls_hostname = true;
                 p.tls_hostname = (char *) valuestr;
                 break;
+            case MIGRATION_PARAMETER_MAX_BANDWIDTH:
+                p.has_max_bandwidth = true;
+                valuebw = qemu_strtosz(valuestr, &endp);
+                if (valuebw < 0 || (size_t)valuebw != valuebw
+                    || *endp != '\0') {
+                    error_setg(&err, "Invalid size %s", valuestr);
+                goto cleanup;
+                }
+                p.max_bandwidth = valuebw;
+                break;
+            case MIGRATION_PARAMETER_DOWNTIME_LIMIT:
+                p.has_downtime_limit = true;
+                use_int_value = true;
+                break;
             }
 
             if (use_int_value) {
@@ -1310,6 +1336,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
                 p.decompress_threads = valueint;
                 p.cpu_throttle_initial = valueint;
                 p.cpu_throttle_increment = valueint;
+                p.downtime_limit = valueint;
             }
 
             qmp_migrate_set_parameters(&p, &err);
diff --git a/include/migration/migration.h b/include/migration/migration.h
index 3c96623..a5429ee 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -129,7 +129,6 @@ struct MigrationSrcPageRequest {
 
 struct MigrationState
 {
-    int64_t bandwidth_limit;
     size_t bytes_xfer;
     size_t xfer_limit;
     QemuThread thread;
diff --git a/migration/migration.c b/migration/migration.c
index 42336e3..9b4aa55 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -44,6 +44,10 @@
 #define BUFFER_DELAY     100
 #define XFER_LIMIT_RATIO (1000 / BUFFER_DELAY)
 
+/* Time in nanoseconds we are allowed to stop the source,
+ * for sending the last part */
+#define DEFAULT_MIGRATE_SET_DOWNTIME 300000000
+
 /* Default compression thread count */
 #define DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT 8
 /* Default decompression thread count, usually decompression is at
@@ -80,7 +84,6 @@ MigrationState *migrate_get_current(void)
     static bool once;
     static MigrationState current_migration = {
         .state = MIGRATION_STATUS_NONE,
-        .bandwidth_limit = MAX_THROTTLE,
         .xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE,
         .mbps = -1,
         .parameters = {
@@ -89,6 +92,8 @@ MigrationState *migrate_get_current(void)
             .decompress_threads = DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT,
             .cpu_throttle_initial = DEFAULT_MIGRATE_CPU_THROTTLE_INITIAL,
             .cpu_throttle_increment = DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT,
+            .max_bandwidth = MAX_THROTTLE,
+            .downtime_limit = DEFAULT_MIGRATE_SET_DOWNTIME,
         },
     };
 
@@ -573,6 +578,10 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
     params->tls_creds = g_strdup(s->parameters.tls_creds);
     params->has_tls_hostname = !!s->parameters.tls_hostname;
     params->tls_hostname = g_strdup(s->parameters.tls_hostname);
+    params->has_max_bandwidth = true;
+    params->max_bandwidth = s->parameters.max_bandwidth;
+    params->has_downtime_limit = true;
+    params->downtime_limit = s->parameters.downtime_limit / 1000000;
 
     return params;
 }
@@ -804,6 +813,20 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
                    "cpu_throttle_increment",
                    "an integer in the range of 1 to 99");
     }
+    if (params->has_max_bandwidth &&
+        (params->max_bandwidth < 0 || params->max_bandwidth > SIZE_MAX)) {
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+                   "max_bandwidth",
+                   "an integer in the range of 0 to SIZE_MAX");
+        return;
+    }
+    if (params->has_downtime_limit &&
+        (params->downtime_limit < 0 || params->downtime_limit > 2000000)) {
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+                   "downtime_limit",
+                   "an integer in the range of 0 to 2000000 milliseconds");
+        return;
+    }
 
     if (params->has_compress_level) {
         s->parameters.compress_level = params->compress_level;
@@ -828,6 +851,20 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
         g_free(s->parameters.tls_hostname);
         s->parameters.tls_hostname = g_strdup(params->tls_hostname);
     }
+    if (params->has_max_bandwidth) {
+        s->parameters.max_bandwidth = params->max_bandwidth;
+        if (s->to_dst_file) {
+            qemu_file_set_rate_limit(s->to_dst_file,
+                                s->parameters.max_bandwidth / XFER_LIMIT_RATIO);
+        }
+    }
+    if (params->has_downtime_limit) {
+        params->downtime_limit *= 1000000; /* convert to nanoseconds */
+
+        s = migrate_get_current();
+        max_downtime = params->downtime_limit;
+        s->parameters.downtime_limit = max_downtime;
+    }
 }
 
 
@@ -1159,30 +1196,27 @@ int64_t qmp_query_migrate_cache_size(Error **errp)
     return migrate_xbzrle_cache_size();
 }
 
-void qmp_migrate_set_speed(int64_t value, Error **errp)
+void qmp_migrate_set_speed(int64_t valuebw, Error **errp)
 {
-    MigrationState *s;
-
-    if (value < 0) {
-        value = 0;
-    }
-    if (value > SIZE_MAX) {
-        value = SIZE_MAX;
-    }
+    MigrationParameters p = {
+        .has_max_bandwidth = true,
+        .max_bandwidth = valuebw,
+    };
 
-    s = migrate_get_current();
-    s->bandwidth_limit = value;
-    if (s->to_dst_file) {
-        qemu_file_set_rate_limit(s->to_dst_file,
-                                 s->bandwidth_limit / XFER_LIMIT_RATIO);
-    }
+    qmp_migrate_set_parameters(&p, errp);
 }
 
-void qmp_migrate_set_downtime(double value, Error **errp)
+void qmp_migrate_set_downtime(double valuedowntime, Error **errp)
 {
-    value *= 1e9;
-    value = MAX(0, MIN(UINT64_MAX, value));
-    max_downtime = (uint64_t)value;
+    valuedowntime *= 1000; /* Convert to milliseconds */
+    valuedowntime = MAX(0, MIN(INT64_MAX, valuedowntime));
+
+    MigrationParameters p = {
+        .has_downtime_limit = true,
+        .downtime_limit = valuedowntime,
+    };
+
+    qmp_migrate_set_parameters(&p, errp);
 }
 
 bool migrate_postcopy_ram(void)
@@ -1854,7 +1888,7 @@ void migrate_fd_connect(MigrationState *s)
 
     qemu_file_set_blocking(s->to_dst_file, true);
     qemu_file_set_rate_limit(s->to_dst_file,
-                             s->bandwidth_limit / XFER_LIMIT_RATIO);
+                             s->parameters.max_bandwidth / XFER_LIMIT_RATIO);
 
     /* Notify before starting migration thread */
     notifier_list_notify(&migration_state_notifiers, s);
diff --git a/qapi-schema.json b/qapi-schema.json
index 88b4888..f92ec88 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -637,12 +637,19 @@
 #                hostname must be provided so that the server's x509
 #                certificate identity can be validated. (Since 2.7)
 #
+# @max-bandwidth: to set maximum speed for migration. maximum speed in
+#                 bytes per second. (Since 2.8)
+#
+# @downtime-limit: set maximum tolerated downtime for migration. maximum
+#                  downtime in milliseconds (Since 2.8)
+#
 # Since: 2.4
 ##
 { 'enum': 'MigrationParameter',
   'data': ['compress-level', 'compress-threads', 'decompress-threads',
            'cpu-throttle-initial', 'cpu-throttle-increment',
-           'tls-creds', 'tls-hostname'] }
+           'tls-creds', 'tls-hostname', 'max-bandwidth',
+           'downtime-limit'] }
 
 #
 # @migrate-set-parameters
@@ -691,6 +698,12 @@
 #                hostname must be provided so that the server's x509
 #                certificate identity can be validated. (Since 2.7)
 #
+# @max-bandwidth: to set maximum speed for migration. maximum speed in
+#                 bytes per second. (Since 2.8)
+#
+# @downtime-limit: set maximum tolerated downtime for migration. maximum
+#                  downtime in milliseconds (Since 2.8)
+#
 # Since: 2.4
 ##
 { 'struct': 'MigrationParameters',
@@ -700,7 +713,9 @@
             '*cpu-throttle-initial': 'int',
             '*cpu-throttle-increment': 'int',
             '*tls-creds': 'str',
-            '*tls-hostname': 'str'} }
+            '*tls-hostname': 'str',
+            '*max-bandwidth': 'int',
+            '*downtime-limit': 'int'} }
 ##
 # @query-migrate-parameters
 #
@@ -1782,6 +1797,8 @@
 #
 # Returns: nothing on success
 #
+# Notes: This command is deprecated in favor of 'migrate-set-parameters'
+#
 # Since: 0.14.0
 ##
 { 'command': 'migrate_set_downtime', 'data': {'value': 'number'} }
@@ -1795,7 +1812,7 @@
 #
 # Returns: nothing on success
 #
-# Notes: A value lesser than zero will be automatically round up to zero.
+# Notes: This command is deprecated in favor of 'migrate-set-parameters'
 #
 # Since: 0.14.0
 ##
diff --git a/qmp-commands.hx b/qmp-commands.hx
index e6c9193..9c277dd 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3800,7 +3800,9 @@ Set migration parameters
                           throttled for auto-converge (json-int)
 - "cpu-throttle-increment": set throttle increasing percentage for
                             auto-converge (json-int)
-
+- "max-bandwidth": set maximum speed for migrations (in bytes/sec) (json-int)
+- "downtime-limit": set maximum tolerated downtime (in milliseconds) for
+                    migrations (json-int)
 Arguments:
 
 Example:
@@ -3813,7 +3815,7 @@ EQMP
     {
         .name       = "migrate-set-parameters",
         .args_type  =
-            "compress-level:i?,compress-threads:i?,decompress-threads:i?,cpu-throttle-initial:i?,cpu-throttle-increment:i?",
+            "compress-level:i?,compress-threads:i?,decompress-threads:i?,cpu-throttle-initial:i?,cpu-throttle-increment:i?,max-bandwidth:i?,downtime-limit:i?",
         .mhandler.cmd_new = qmp_marshal_migrate_set_parameters,
     },
 SQMP
@@ -3830,6 +3832,10 @@ Query current migration parameters
                                     throttled (json-int)
          - "cpu-throttle-increment" : throttle increasing percentage for
                                       auto-converge (json-int)
+         - "max-bandwidth" : maximium migration speed in bytes per second
+                             (json-int)
+         - "downtime-limit" : maximum tolerated downtime of migration in
+                              milliseconds (json-int)
 
 Arguments:
 
@@ -3842,7 +3848,9 @@ Example:
          "cpu-throttle-increment": 10,
          "compress-threads": 8,
          "compress-level": 1,
-         "cpu-throttle-initial": 20
+         "cpu-throttle-initial": 20,
+         "max-bandwidth": 33554432,
+         "downtime-limit": 300
       }
    }
 
-- 
2.6.2

Re: [Qemu-devel] [PATCH v5] migrate: Move max-bandwidth and downtime-limit to migrate_set_parameter

[Eric Blake]

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

On 09/09/2016 02:02 PM, Ashijeet Acharya wrote:
> Mark old-commands for speed and downtime as deprecated.

Maybe s/old-commands for speed and downtime/the old commands
'migrate_set_speed' and 'migrate_set_downtime'/

> Move max-bandwidth and downtime-limit into migrate-set-parameters for
> setting maximum migration speed and expected downtime limit parameters
> respectively.
> Change downtime units to milliseconds (only for new-command) and update
> the query part in both hmp and qmp qemu control interfaces.

This part is fine.

> NOTE: This patch is solely based on Eric's new boxed parameters from QAPI
> patch series.
> 

This paragraph is useful to reviewers, but won't make sense in the long
run (as my patch series will presumably already be in git first, since
yours does indeed depend on mine), so it can go better...

> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
> ---

...here, along with your changelog.

> @@ -1295,6 +1307,20 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>                  p.has_tls_hostname = true;
>                  p.tls_hostname = (char *) valuestr;
>                  break;
> +            case MIGRATION_PARAMETER_MAX_BANDWIDTH:
> +                p.has_max_bandwidth = true;
> +                valuebw = qemu_strtosz(valuestr, &endp);
> +                if (valuebw < 0 || (size_t)valuebw != valuebw
> +                    || *endp != '\0') {
> +                    error_setg(&err, "Invalid size %s", valuestr);
> +                goto cleanup;

Indentation is off for the goto.

> 
> +++ b/migration/migration.c
> @@ -44,6 +44,10 @@
>  #define BUFFER_DELAY     100
>  #define XFER_LIMIT_RATIO (1000 / BUFFER_DELAY)
>  
> +/* Time in nanoseconds we are allowed to stop the source,
> + * for sending the last part */
> +#define DEFAULT_MIGRATE_SET_DOWNTIME 300000000
> +

I would have expected that the 'static uint64_t max_downtime'
declaration would completely disappear, now that you are moving all the
data to live inside the rest of the migration parameters.  More on that
below [1]

> @@ -804,6 +813,20 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
>                     "cpu_throttle_increment",
>                     "an integer in the range of 1 to 99");
>      }
> +    if (params->has_max_bandwidth &&
> +        (params->max_bandwidth < 0 || params->max_bandwidth > SIZE_MAX)) {
> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> +                   "max_bandwidth",
> +                   "an integer in the range of 0 to SIZE_MAX");

Might be nicer to give a numeric value instead of assuming the reader
knows what value SIZE_MAX has, but not the end of the world.

> +        return;
> +    }
> +    if (params->has_downtime_limit &&
> +        (params->downtime_limit < 0 || params->downtime_limit > 2000000)) {

You are setting a new maximum limit smaller than what the code
previously allowed; while I think that 2000 seconds as a maximum
downtime is indeed more generous than anyone will ever use in real life,
you should at least document in the commit message that your new smaller
upper limit is intentional.

> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> +                   "downtime_limit",
> +                   "an integer in the range of 0 to 2000000 milliseconds");
> +        return;
> +    }
>  
>      if (params->has_compress_level) {
>          s->parameters.compress_level = params->compress_level;
> @@ -828,6 +851,20 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
>          g_free(s->parameters.tls_hostname);
>          s->parameters.tls_hostname = g_strdup(params->tls_hostname);
>      }
> +    if (params->has_max_bandwidth) {
> +        s->parameters.max_bandwidth = params->max_bandwidth;
> +        if (s->to_dst_file) {
> +            qemu_file_set_rate_limit(s->to_dst_file,
> +                                s->parameters.max_bandwidth / XFER_LIMIT_RATIO);
> +        }
> +    }
> +    if (params->has_downtime_limit) {
> +        params->downtime_limit *= 1000000; /* convert to nanoseconds */

I'd update the comment to include an additional phrase: "safe from
overflow, since we capped the upper limit above"

> +
> +        s = migrate_get_current();
> +        max_downtime = params->downtime_limit;
> +        s->parameters.downtime_limit = max_downtime;

[1] Uggh.  s->parameters shares the same type as params
(MigrationParameters), but we are using it to hold a limit in
nanoseconds in one instance and a limit in milliseconds in the other.
Which means we have to scale any time we assign from one field to the
other, and cannot use simple copies between the two locations.  What's
more, you are then STILL using the file-level variable 'max_downtime'
(in nanoseconds) rather than the new s->parameters.downtime_limit.  If
s->parameters is good enough, then we don't need the file-scope
'max_downtime'.

I would MUCH rather see us consistently use the SAME scale (milliseconds
is fine), where a single point of documentation in the .json file
correctly describes ALL uses of the downtime_limit member of
MigrationParameters.  Even if that means splitting this into a
multi-patch series (one to convert all existing uses of max_downtime to
a scale of milliseconds, the second to then convert from max_downtime
over to the new s->parameters.downtime_limit).

>  
> @@ -1159,30 +1196,27 @@ int64_t qmp_query_migrate_cache_size(Error **errp)
>      return migrate_xbzrle_cache_size();
>  }
>  
> -void qmp_migrate_set_speed(int64_t value, Error **errp)
> +void qmp_migrate_set_speed(int64_t valuebw, Error **errp)

Why did we need to rename value to valuebw?

>  {
> -    MigrationState *s;
> -
> -    if (value < 0) {
> -        value = 0;
> -    }
> -    if (value > SIZE_MAX) {
> -        value = SIZE_MAX;
> -    }
> +    MigrationParameters p = {
> +        .has_max_bandwidth = true,
> +        .max_bandwidth = valuebw,
> +    };
>  
> -    s = migrate_get_current();
> -    s->bandwidth_limit = value;
> -    if (s->to_dst_file) {
> -        qemu_file_set_rate_limit(s->to_dst_file,
> -                                 s->bandwidth_limit / XFER_LIMIT_RATIO);
> -    }
> +    qmp_migrate_set_parameters(&p, errp);
>  }
>  
> -void qmp_migrate_set_downtime(double value, Error **errp)
> +void qmp_migrate_set_downtime(double valuedowntime, Error **errp)

Again, why rename value?

> +++ b/qapi-schema.json

> @@ -1782,6 +1797,8 @@
>  #
>  # Returns: nothing on success
>  #
> +# Notes: This command is deprecated in favor of 'migrate-set-parameters'
> +#
>  # Since: 0.14.0
>  ##
>  { 'command': 'migrate_set_downtime', 'data': {'value': 'number'} }
> @@ -1795,7 +1812,7 @@
>  #
>  # Returns: nothing on success
>  #
> -# Notes: A value lesser than zero will be automatically round up to zero.
> +# Notes: This command is deprecated in favor of 'migrate-set-parameters'

Do we need to drop the old note about behavior on negative inputs?  On
the one hand, the new interface doesn't suffer from that interface wart,
so the old interface is the only place where the note is even useful; on
the other hand, since we don't want people to use the old interface, not
documenting the wart seems fine to me.

> @@ -3813,7 +3815,7 @@ EQMP
>      {
>          .name       = "migrate-set-parameters",
>          .args_type  =
> -            "compress-level:i?,compress-threads:i?,decompress-threads:i?,cpu-throttle-initial:i?,cpu-throttle-increment:i?",
> +            "compress-level:i?,compress-threads:i?,decompress-threads:i?,cpu-throttle-initial:i?,cpu-throttle-increment:i?,max-bandwidth:i?,downtime-limit:i?",

Trivial conflict with Marc-Andre's series on qapi-next that removes
.args_type altogether.

Getting closer, but I still think re-scaling max_downtime should be done
separately from moving it into MigrationParameters, and that we
absolutely do not want to use two different scales for various
MigrationParameters->downtime_limit uses.

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


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

Re: [Qemu-devel] [PATCH v5] migrate: Move max-bandwidth and downtime-limit to migrate_set_parameter

[Ashijeet Acharya]

On Wed, Sep 14, 2016 at 12:52 AM, Eric Blake <eblake@redhat.com> wrote:
> On 09/09/2016 02:02 PM, Ashijeet Acharya wrote:
>> Mark old-commands for speed and downtime as deprecated.
>
> Maybe s/old-commands for speed and downtime/the old commands
> 'migrate_set_speed' and 'migrate_set_downtime'/
>
>> Move max-bandwidth and downtime-limit into migrate-set-parameters for
>> setting maximum migration speed and expected downtime limit parameters
>> respectively.
>> Change downtime units to milliseconds (only for new-command) and update
>> the query part in both hmp and qmp qemu control interfaces.
>
> This part is fine.
>
>> NOTE: This patch is solely based on Eric's new boxed parameters from QAPI
>> patch series.
>>
>
> This paragraph is useful to reviewers, but won't make sense in the long
> run (as my patch series will presumably already be in git first, since
> yours does indeed depend on mine), so it can go better...
>

Okay, I will improve the commit message.

>> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
>> ---
>
> ...here, along with your changelog.
>
>> @@ -1295,6 +1307,20 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>>                  p.has_tls_hostname = true;
>>                  p.tls_hostname = (char *) valuestr;
>>                  break;
>> +            case MIGRATION_PARAMETER_MAX_BANDWIDTH:
>> +                p.has_max_bandwidth = true;
>> +                valuebw = qemu_strtosz(valuestr, &endp);
>> +                if (valuebw < 0 || (size_t)valuebw != valuebw
>> +                    || *endp != '\0') {
>> +                    error_setg(&err, "Invalid size %s", valuestr);
>> +                goto cleanup;
>
> Indentation is off for the goto.
>
>>
>> +++ b/migration/migration.c
>> @@ -44,6 +44,10 @@
>>  #define BUFFER_DELAY     100
>>  #define XFER_LIMIT_RATIO (1000 / BUFFER_DELAY)
>>
>> +/* Time in nanoseconds we are allowed to stop the source,
>> + * for sending the last part */
>> +#define DEFAULT_MIGRATE_SET_DOWNTIME 300000000
>> +
>
> I would have expected that the 'static uint64_t max_downtime'
> declaration would completely disappear, now that you are moving all the
> data to live inside the rest of the migration parameters.  More on that
> below [1]
>
>> @@ -804,6 +813,20 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
>>                     "cpu_throttle_increment",
>>                     "an integer in the range of 1 to 99");
>>      }
>> +    if (params->has_max_bandwidth &&
>> +        (params->max_bandwidth < 0 || params->max_bandwidth > SIZE_MAX)) {
>> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>> +                   "max_bandwidth",
>> +                   "an integer in the range of 0 to SIZE_MAX");
>
> Might be nicer to give a numeric value instead of assuming the reader
> knows what value SIZE_MAX has, but not the end of the world.

I used SIZE_MAX because substituting it with its value looked a bit messy.
Should I use its actual value?

>
>> +        return;
>> +    }
>> +    if (params->has_downtime_limit &&
>> +        (params->downtime_limit < 0 || params->downtime_limit > 2000000)) {
>
> You are setting a new maximum limit smaller than what the code
> previously allowed; while I think that 2000 seconds as a maximum
> downtime is indeed more generous than anyone will ever use in real life,
> you should at least document in the commit message that your new smaller
> upper limit is intentional.

Yeah, I had a discussion with Dave about this one on the IRC.
Although 2000 seconds is a sufficiently large value for downtime as we
practically only use values in range of milliseconds but I will
include it in the commit message.

>
>> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>> +                   "downtime_limit",
>> +                   "an integer in the range of 0 to 2000000 milliseconds");
>> +        return;
>> +    }
>>
>>      if (params->has_compress_level) {
>>          s->parameters.compress_level = params->compress_level;
>> @@ -828,6 +851,20 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
>>          g_free(s->parameters.tls_hostname);
>>          s->parameters.tls_hostname = g_strdup(params->tls_hostname);
>>      }
>> +    if (params->has_max_bandwidth) {
>> +        s->parameters.max_bandwidth = params->max_bandwidth;
>> +        if (s->to_dst_file) {
>> +            qemu_file_set_rate_limit(s->to_dst_file,
>> +                                s->parameters.max_bandwidth / XFER_LIMIT_RATIO);
>> +        }
>> +    }
>> +    if (params->has_downtime_limit) {
>> +        params->downtime_limit *= 1000000; /* convert to nanoseconds */
>
> I'd update the comment to include an additional phrase: "safe from
> overflow, since we capped the upper limit above"
>

Okay.

>> +
>> +        s = migrate_get_current();
>> +        max_downtime = params->downtime_limit;
>> +        s->parameters.downtime_limit = max_downtime;
>
> [1] Uggh.  s->parameters shares the same type as params
> (MigrationParameters), but we are using it to hold a limit in
> nanoseconds in one instance and a limit in milliseconds in the other.
> Which means we have to scale any time we assign from one field to the
> other, and cannot use simple copies between the two locations.  What's
> more, you are then STILL using the file-level variable 'max_downtime'
> (in nanoseconds) rather than the new s->parameters.downtime_limit.  If
> s->parameters is good enough, then we don't need the file-scope
> 'max_downtime'.

I understand, but I left 'max_downtime' as it is so that I don't have
to substitute it everywhere in the file and add unnecessary additions
and deletions in the patch but I will remove it now as you recommend.

>
> I would MUCH rather see us consistently use the SAME scale (milliseconds
> is fine), where a single point of documentation in the .json file
> correctly describes ALL uses of the downtime_limit member of
> MigrationParameters.  Even if that means splitting this into a
> multi-patch series (one to convert all existing uses of max_downtime to
> a scale of milliseconds, the second to then convert from max_downtime
> over to the new s->parameters.downtime_limit).
>
>>
>> @@ -1159,30 +1196,27 @@ int64_t qmp_query_migrate_cache_size(Error **errp)
>>      return migrate_xbzrle_cache_size();
>>  }
>>
>> -void qmp_migrate_set_speed(int64_t value, Error **errp)
>> +void qmp_migrate_set_speed(int64_t valuebw, Error **errp)
>
> Why did we need to rename value to valuebw?
>
>>  {
>> -    MigrationState *s;
>> -
>> -    if (value < 0) {
>> -        value = 0;
>> -    }
>> -    if (value > SIZE_MAX) {
>> -        value = SIZE_MAX;
>> -    }
>> +    MigrationParameters p = {
>> +        .has_max_bandwidth = true,
>> +        .max_bandwidth = valuebw,
>> +    };
>>
>> -    s = migrate_get_current();
>> -    s->bandwidth_limit = value;
>> -    if (s->to_dst_file) {
>> -        qemu_file_set_rate_limit(s->to_dst_file,
>> -                                 s->bandwidth_limit / XFER_LIMIT_RATIO);
>> -    }
>> +    qmp_migrate_set_parameters(&p, errp);
>>  }
>>
>> -void qmp_migrate_set_downtime(double value, Error **errp)
>> +void qmp_migrate_set_downtime(double valuedowntime, Error **errp)
>
> Again, why rename value?
>
>> +++ b/qapi-schema.json
>
>> @@ -1782,6 +1797,8 @@
>>  #
>>  # Returns: nothing on success
>>  #
>> +# Notes: This command is deprecated in favor of 'migrate-set-parameters'
>> +#
>>  # Since: 0.14.0
>>  ##
>>  { 'command': 'migrate_set_downtime', 'data': {'value': 'number'} }
>> @@ -1795,7 +1812,7 @@
>>  #
>>  # Returns: nothing on success
>>  #
>> -# Notes: A value lesser than zero will be automatically round up to zero.
>> +# Notes: This command is deprecated in favor of 'migrate-set-parameters'
>
> Do we need to drop the old note about behavior on negative inputs?  On
> the one hand, the new interface doesn't suffer from that interface wart,
> so the old interface is the only place where the note is even useful; on
> the other hand, since we don't want people to use the old interface, not
> documenting the wart seems fine to me.

Dropping the old note seemed reasonable to me as the old-commands are
now only a wrapper around the new ones.
So the bounds check error applies on both.

>
>> @@ -3813,7 +3815,7 @@ EQMP
>>      {
>>          .name       = "migrate-set-parameters",
>>          .args_type  =
>> -            "compress-level:i?,compress-threads:i?,decompress-threads:i?,cpu-throttle-initial:i?,cpu-throttle-increment:i?",
>> +            "compress-level:i?,compress-threads:i?,decompress-threads:i?,cpu-throttle-initial:i?,cpu-throttle-increment:i?,max-bandwidth:i?,downtime-limit:i?",
>
> Trivial conflict with Marc-Andre's series on qapi-next that removes
> .args_type altogether.
>
> Getting closer, but I still think re-scaling max_downtime should be done
> separately from moving it into MigrationParameters, and that we
> absolutely do not want to use two different scales for various
> MigrationParameters->downtime_limit uses.

Okay, I will send the updated patch soon.

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

Re: [Qemu-devel] [PATCH v5] migrate: Move max-bandwidth and downtime-limit to migrate_set_parameter

[Eric Blake]

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

On 09/14/2016 03:05 AM, Ashijeet Acharya wrote:

>>> +    if (params->has_max_bandwidth &&
>>> +        (params->max_bandwidth < 0 || params->max_bandwidth > SIZE_MAX)) {
>>> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>>> +                   "max_bandwidth",
>>> +                   "an integer in the range of 0 to SIZE_MAX");
>>
>> Might be nicer to give a numeric value instead of assuming the reader
>> knows what value SIZE_MAX has, but not the end of the world.
> 
> I used SIZE_MAX because substituting it with its value looked a bit messy.
> Should I use its actual value?

as in ("...%zu", ..., SIZE_MAX), since not all platforms have the same
value of SIZE_MAX.  I'm okay if you don't bother, but if you do, see if
any other code does similar, and copy how they do it (I seem to recall
past complaints about a cast being needed because at least one system
has a broken header that defines SIZE_MAX with the wrong type).

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


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

Re: [Qemu-devel] [PATCH v5] migrate: Move max-bandwidth and downtime-limit to migrate_set_parameter

[Ashijeet Acharya]

On Wed, Sep 14, 2016 at 7:28 PM, Eric Blake <eblake@redhat.com> wrote:
> On 09/14/2016 03:05 AM, Ashijeet Acharya wrote:
>
>>>> +    if (params->has_max_bandwidth &&
>>>> +        (params->max_bandwidth < 0 || params->max_bandwidth > SIZE_MAX)) {
>>>> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>>>> +                   "max_bandwidth",
>>>> +                   "an integer in the range of 0 to SIZE_MAX");
>>>
>>> Might be nicer to give a numeric value instead of assuming the reader
>>> knows what value SIZE_MAX has, but not the end of the world.
>>
>> I used SIZE_MAX because substituting it with its value looked a bit messy.
>> Should I use its actual value?
>
> as in ("...%zu", ..., SIZE_MAX), since not all platforms have the same
> value of SIZE_MAX.  I'm okay if you don't bother, but if you do, see if
> any other code does similar, and copy how they do it (I seem to recall
> past complaints about a cast being needed because at least one system
> has a broken header that defines SIZE_MAX with the wrong type).
>

Well, I did grep around to find any similar occurrences but
unfortunately I found none.
So, I am gonna use the method you described above.

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

Re: [Qemu-devel] [PATCH v5] migrate: Move max-bandwidth and downtime-limit to migrate_set_parameter

[Ashijeet Acharya]

>> +++ b/migration/migration.c
>> @@ -44,6 +44,10 @@
>>  #define BUFFER_DELAY     100
>>  #define XFER_LIMIT_RATIO (1000 / BUFFER_DELAY)
>>
>> +/* Time in nanoseconds we are allowed to stop the source,
>> + * for sending the last part */
>> +#define DEFAULT_MIGRATE_SET_DOWNTIME 300000000
>> +
>
> I would have expected that the 'static uint64_t max_downtime'
> declaration would completely disappear, now that you are moving all the
> data to live inside the rest of the migration parameters.  More on that
> below [1]
>

>> +    if (params->has_max_bandwidth) {
>> +        s->parameters.max_bandwidth = params->max_bandwidth;
>> +        if (s->to_dst_file) {
>> +            qemu_file_set_rate_limit(s->to_dst_file,
>> +                                s->parameters.max_bandwidth / XFER_LIMIT_RATIO);
>> +        }
>> +    }
>> +    if (params->has_downtime_limit) {
>> +        params->downtime_limit *= 1000000; /* convert to nanoseconds */
>
> I'd update the comment to include an additional phrase: "safe from
> overflow, since we capped the upper limit above"
>
>> +
>> +        s = migrate_get_current();
>> +        max_downtime = params->downtime_limit;
>> +        s->parameters.downtime_limit = max_downtime;
>
> [1] Uggh.  s->parameters shares the same type as params
> (MigrationParameters), but we are using it to hold a limit in
> nanoseconds in one instance and a limit in milliseconds in the other.
> Which means we have to scale any time we assign from one field to the
> other, and cannot use simple copies between the two locations.  What's
> more, you are then STILL using the file-level variable 'max_downtime'
> (in nanoseconds) rather than the new s->parameters.downtime_limit.  If
> s->parameters is good enough, then we don't need the file-scope
> 'max_downtime'.
>
> I would MUCH rather see us consistently use the SAME scale (milliseconds
> is fine), where a single point of documentation in the .json file
> correctly describes ALL uses of the downtime_limit member of
> MigrationParameters.  Even if that means splitting this into a
> multi-patch series (one to convert all existing uses of max_downtime to
> a scale of milliseconds, the second to then convert from max_downtime
> over to the new s->parameters.downtime_limit).
>

> Getting closer, but I still think re-scaling max_downtime should be done
> separately from moving it into MigrationParameters, and that we
> absolutely do not want to use two different scales for various
> MigrationParameters->downtime_limit uses.
>

Thinking about what you said about re-scaling a bit more, since I will
be dropping the conversion of 'downtime' to nanoseconds part, there
will be no issue of overflow anymore at that point and can drop the
bounds check along with the comments. Although, I think its
appropriate to move the bounds check code below to
qmp_migrate_set_downtime() while multiplying with '1000' to convert to
milliseconds (this is applicable only for the old commads). After this
dropping 'max_downtime' and other substitutions will work quite fine.

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