From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47196) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZEbV3-000735-Sk for qemu-devel@nongnu.org; Mon, 13 Jul 2015 07:02:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZEbUz-0005M5-Or for qemu-devel@nongnu.org; Mon, 13 Jul 2015 07:02:17 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42976) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZEbUz-0005Lw-H2 for qemu-devel@nongnu.org; Mon, 13 Jul 2015 07:02:13 -0400 From: Juan Quintela In-Reply-To: <1434450415-11339-19-git-send-email-dgilbert@redhat.com> (David Alan Gilbert's message of "Tue, 16 Jun 2015 11:26:31 +0100") References: <1434450415-11339-1-git-send-email-dgilbert@redhat.com> <1434450415-11339-19-git-send-email-dgilbert@redhat.com> Date: Mon, 13 Jul 2015 13:02:09 +0200 Message-ID: <87k2u4l3ri.fsf@neno.neno> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v7 18/42] Add wrappers and handlers for sending/receiving the postcopy-ram migration messages. Reply-To: quintela@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Dr. David Alan Gilbert (git)" Cc: aarcange@redhat.com, yamahata@private.email.ne.jp, liang.z.li@intel.com, qemu-devel@nongnu.org, luis@cs.umu.se, amit.shah@redhat.com, pbonzini@redhat.com, david@gibson.dropbear.id.au "Dr. David Alan Gilbert (git)" wrote: > From: "Dr. David Alan Gilbert" > > The state of the postcopy process is managed via a series of messages; > * Add wrappers and handlers for sending/receiving these messages > * Add state variable that track the current state of postcopy > > Signed-off-by: Dr. David Alan Gilbert > --- > include/migration/migration.h | 16 +++ > include/sysemu/sysemu.h | 20 ++++ > migration/migration.c | 13 +++ > migration/savevm.c | 247 ++++++++++++++++++++++++++++++++++++++++++ > trace-events | 10 ++ > 5 files changed, 306 insertions(+) > > diff --git a/migration/migration.c b/migration/migration.c > index cd89a9b..34cd9a6 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -1128,3 +1128,16 @@ void migrate_fd_connect(MigrationState *s) > qemu_thread_create(&s->thread, "migration", migration_thread, s, > QEMU_THREAD_JOINABLE); > } > + > +PostcopyState postcopy_state_get(MigrationIncomingState *mis) > +{ > + return atomic_fetch_add(&mis->postcopy_state, 0); What is wrong with atomic_read() here? As the set of the state is atomic, even a normal read would do (I think) > +void qemu_savevm_send_postcopy_ram_discard(QEMUFile *f, const char *name, > + uint16_t len, > + uint64_t *start_list, > + uint64_t *end_list) I haven't looked at the following patches where this function is used, but it appears that getting an iovec could be a good idea? > +{ > + uint8_t *buf; > + uint16_t tmplen; > + uint16_t t; > + size_t name_len = strlen(name); > + > + trace_qemu_savevm_send_postcopy_ram_discard(name, len); > + buf = g_malloc0(len*16 + name_len + 3); I would suggest gmalloc0(1 + 1 + name_len + 1 + (8 + 8) * len) just to be clear where things came from. I think that we don't need the \0 at all. If \0 is not there, strlen() return is going to be "funny". So, we can just change the assert to name_len < 255? > + buf[0] = 0; /* Version */ > + assert(name_len < 256); Can we move the assert before the malloc()? My guess is that in a perfect world the assert would be a return -EINVAL, but I know that it is complicated. > + buf[1] = name_len; > + memcpy(buf+2, name, name_len); spaces around '+' (same around) > + tmplen = 2+name_len; > + buf[tmplen++] = '\0'; > + > + for (t = 0; t < len; t++) { > + cpu_to_be64w((uint64_t *)(buf + tmplen), start_list[t]); > + tmplen += 8; > + cpu_to_be64w((uint64_t *)(buf + tmplen), end_list[t]); > + tmplen += 8; trace_qemu_savevm_send_postcopy_range(name, start_list[t], end_list[t]); ?? > + /* We're expecting a > + * Version (0) > + * a RAM ID string (length byte, name, 0 term) > + * then at least 1 16 byte chunk > + */ > + if (len < 20) { 1 + 1+1+1+1+2*8 Humm, thinking about it, .... why are we not needing a length field of number of entries? > + error_report("CMD_POSTCOPY_RAM_DISCARD invalid length (%d)", len); > + return -1; > + } > + > + tmp = qemu_get_byte(mis->file); > + if (tmp != 0) { I think that a constant telling POSTCOPY_VERSION0 or whatever?