From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59289) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YFo3t-0004IR-4e for qemu-devel@nongnu.org; Mon, 26 Jan 2015 13:06:58 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YFo3o-0002X6-UY for qemu-devel@nongnu.org; Mon, 26 Jan 2015 13:06:57 -0500 Date: Mon, 26 Jan 2015 18:06:47 +0000 From: "Dr. David Alan Gilbert" Message-ID: <20150126180647.GD2291@work-vm> References: <1422270747-23994-1-git-send-email-pbonzini@redhat.com> <1422270747-23994-8-git-send-email-pbonzini@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1422270747-23994-8-git-send-email-pbonzini@redhat.com> Subject: Re: [Qemu-devel] [PATCH 7/7] migration: do floating-point division List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-trivial@nongnu.org, quintela@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com * Paolo Bonzini (pbonzini@redhat.com) wrote: > Dividing integer expressions transferred_bytes and time_spent, and then converting > the integer quotient to type double. Any remainder, or fractional part of the > quotient, is ignored. Fix this. > > Signed-off-by: Paolo Bonzini > --- > migration/migration.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/migration/migration.c b/migration/migration.c > index b3adbc6..6db75b8 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -646,7 +646,7 @@ static void *migration_thread(void *opaque) > if (current_time >= initial_time + BUFFER_DELAY) { > uint64_t transferred_bytes = qemu_ftell(s->file) - initial_bytes; > uint64_t time_spent = current_time - initial_time; > - double bandwidth = transferred_bytes / time_spent; > + double bandwidth = (double)transferred_bytes / time_spent; > max_size = bandwidth * migrate_max_downtime() / 1000000; > > s->mbps = time_spent ? (((double) transferred_bytes * 8.0) / This feels like it would be better to fix this by merging it into the s->mbps calculation just off the bottom; we currently have: uint64_t transferred_bytes = qemu_ftell(s->file) - initial_bytes; uint64_t time_spent = current_time - initial_time; double bandwidth = transferred_bytes / time_spent; max_size = bandwidth * migrate_max_downtime() / 1000000; s->mbps = time_spent ? (((double) transferred_bytes * 8.0) / ((double) time_spent / 1000.0)) / 1000.0 / 1000.0 : -1; Note that the mbps has a check for time_spent being 0 - if that can ever happen, how come 'bandwidth' has never triggered it? transferred_bytes - bytes time_spent - ms bandwidth - bytes/ms migrate_max_downtime - in ns s->mbps - mbit/s giving max_size = bytes/ms * time-in-ns / 1000000 = bytes/ms * time-in-ms = bytes so how about something like: uint64_t transferred_bytes = qemu_ftell(s->file) - initial_bytes; uint64_t time_spent = current_time - initial_time; double bytes_s = (double) transferred_bytes / ((double) time_spent / 1000.0)); s->mbps = (bytes_s * 8.0) / 1000000.0; max_size = bytes_s * (migrate_max_downtime() / 1000000000.0); that also needs the trace fixing and the line a few lines below, I *think* we have dirty_bytes_rate is in bytes/second ? (arch_init.c) expected_downtime in ms ? s->expected_downtime = s->dirty_bytes_rate / bandwidth; so, bytes/s / bytes/ms erm that's supposed to come out as time in ms s->expected_downtime = (int64_t)(1000 * (double)s->dirty_bytes_rate / bytes_s); Yeuch. Dave > -- > 1.8.3.1 > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK