From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48825) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bhuAz-0000Vb-19 for qemu-devel@nongnu.org; Thu, 08 Sep 2016 03:55:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bhuAw-0002w4-9T for qemu-devel@nongnu.org; Thu, 08 Sep 2016 03:55:12 -0400 Received: from mail-oi0-x241.google.com ([2607:f8b0:4003:c06::241]:33557) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bhuAw-0002vf-3d for qemu-devel@nongnu.org; Thu, 08 Sep 2016 03:55:10 -0400 Received: by mail-oi0-x241.google.com with SMTP id y2so404245oie.0 for ; Thu, 08 Sep 2016 00:55:09 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <01e7066e-b807-95a6-6a5e-3435651a949e@redhat.com> References: <1473169187-22750-1-git-send-email-ashijeetacharya@gmail.com> <01e7066e-b807-95a6-6a5e-3435651a949e@redhat.com> From: Ashijeet Acharya Date: Thu, 8 Sep 2016 13:25:08 +0530 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH v3] Move max-bandwidth and downtime-limit into migrate_set_parameter for both hmp and qmp List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: lcapitulino@redhat.com, quintela@redhat.com, amit.shah@redhat.com, armbru@redhat.com, "dgilbert@redhat.com" , Paolo Bonzini , "Daniel P. Berrange" , QEMU Developers On Thu, Sep 8, 2016 at 3:33 AM, Eric Blake wrote: > On 09/06/2016 08:39 AM, Ashijeet Acharya wrote: >> 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. >> >> Signed-off-by: Ashijeet Acharya >> --- > >> +++ b/migration/migration.c >> @@ -44,6 +44,9 @@ >> #define BUFFER_DELAY 100 >> #define XFER_LIMIT_RATIO (1000 / BUFFER_DELAY) >> >> +/* Time in nanoseconds we are allowed to stop the source for to send last part */ > > Long line. Also, a grammar issue: > > s/source for to send/source, for sending the/ Okay, I will change it. > >> @@ -832,6 +848,21 @@ void qmp_migrate_set_parameters(bool has_compress_level, >> g_free(s->parameters.tls_hostname); >> s->parameters.tls_hostname = g_strdup(tls_hostname); >> } >> + if (has_max_bandwidth) { >> + s->parameters.max_bandwidth = max_bandwidth; >> + if (s->to_dst_file) { >> + qemu_file_set_rate_limit(s->to_dst_file, >> + s->parameters.max_bandwidth / XFER_LIMIT_RATIO); >> + } >> + } >> + if (has_downtime_limit) { >> + downtime_limit *= 1000000; /* convert to nanoseconds */ > > Are you sure this won't overflow? Should I explicitly cast it? Like; downtime_limit *= (int64_t)1000000; This should work right? >> +void qmp_migrate_set_speed(int64_t valuebw, Error **errp) >> +{ >> + bool has_compress_level = false; >> + bool has_compress_threads = false; >> + bool has_decompress_threads = false; >> + bool has_cpu_throttle_initial = false; >> + bool has_cpu_throttle_increment = false; >> + bool has_tls_creds = false; >> + bool has_tls_hostname = false; >> + bool has_max_bandwidth = true; >> + bool has_downtime_limit = false; >> + const char *valuestr = NULL; >> + long valueint = 0; >> + Error *err = NULL; >> + >> + qmp_migrate_set_parameters(has_compress_level, valueint, >> + has_compress_threads, valueint, > > Ugg. This looks gross. No need to name a bunch of variables set to > false, when you can instead use QAPI's boxed conventions to pass a > pointer to a struct, and rely on 0-initialization of the struct to leave > all the parameters that you don't care about unmentioned. > I know. But I was not sure if I can do something else. 'hmp.c' does it in a similar way. I am not aware of the method you described. Maybe you can direct me to some place where I can code by example. (Sorry! I am a newbie) >> + has_decompress_threads, valueint, >> + has_cpu_throttle_initial, valueint, >> + has_cpu_throttle_increment, valueint, >> + has_tls_creds, valuestr, >> + has_tls_hostname, valuestr, >> + has_max_bandwidth, valuebw, >> + has_downtime_limit, valueint, >> + &err); >> + >> +} >> + >> +void qmp_migrate_set_downtime(double valuedowntime, Error **errp) >> +{ >> + bool has_compress_level = false; >> + bool has_compress_threads = false; >> + bool has_decompress_threads = false; >> + bool has_cpu_throttle_initial = false; >> + bool has_cpu_throttle_increment = false; >> + bool has_tls_creds = false; >> + bool has_tls_hostname = false; >> + bool has_max_bandwidth = false; >> + bool has_downtime_limit = true; > > Again, gross. > >> + const char *valuestr = NULL; >> + long valueint = 0; >> + int64_t valuebw = 0; >> + valuedowntime *= 1000; /* Convert to milliseconds */ >> + valuedowntime = MAX(0, MIN(INT64_MAX, valuedowntime)); >> + valuedowntime = (int64_t)valuedowntime; > > Useless statement; the cast would already implicitly happen when passing > it to the function call. Okay, I will remove that. > >> + Error *err = NULL; >> + >> + qmp_migrate_set_parameters(has_compress_level, valueint, >> + has_compress_threads, valueint, >> + has_decompress_threads, valueint, >> + has_cpu_throttle_initial, valueint, >> + has_cpu_throttle_increment, valueint, >> + has_tls_creds, valuestr, >> + has_tls_hostname, valuestr, >> + has_max_bandwidth, valuebw, >> + has_downtime_limit, valuedowntime, >> + &err); >> } >> >> bool migrate_postcopy_ram(void) >> @@ -1858,7 +1922,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 5658723..a8ee2d4 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 > > Long line. Please wrap to stay under 80 columns. Okay. > >> +# in milliseconds (Since 2.8) > > Are we sure milliseconds is the desired scale? I am really confused regarding the scale for downtime now. There seems to be a divided opinion on this one among developers with everyone having their valid points. I will stall the updated patch until we have a common ground on this one. > >> { 'command': 'migrate-set-parameters', >> @@ -687,7 +700,9 @@ >> '*cpu-throttle-initial': 'int', >> '*cpu-throttle-increment': 'int', >> '*tls-creds': 'str', >> - '*tls-hostname': 'str'} } >> + '*tls-hostname': 'str', >> + '*max-bandwidth': 'int', >> + '*downtime-limit': 'int'} } > > For that matter, would a floating point downtime-limit make any more > sense (in which case, I'd argue that having it be in seconds rather than > milliseconds may be nicer)? Yeah! Floating point seems useful only if we are talking in seconds. A floating point value in milliseconds seems impractical as far as I understand. > > >> @@ -1812,6 +1835,8 @@ >> # >> # Returns: nothing on success >> # >> +# Notes: This command is deprecated in favour of 'migrate-set-parameters' > > I don't know if we have a strong preference for US vs. UK spelling in > documentation. I can change it to 'favor' if you like. > >> @@ -3803,7 +3805,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?", > > Long line; you can break it up (but then again, we hope to get rid of > all .args_type lines once Marc-Andre's qapi work lands) Breaking it rejects the arguments after the break I guess and I get an error if I use them in qmp later. The line was long even before including 'max-bandwidth' and 'downtime-limit' anyway. Ashijeet > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >