From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37555) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZUc08-0006L7-63 for qemu-devel@nongnu.org; Wed, 26 Aug 2015 10:48:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZUc05-00021i-0T for qemu-devel@nongnu.org; Wed, 26 Aug 2015 10:48:32 -0400 Received: from mx1.redhat.com ([209.132.183.28]:40782) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZUc04-00021S-Od for qemu-devel@nongnu.org; Wed, 26 Aug 2015 10:48:28 -0400 Date: Wed, 26 Aug 2015 15:48:21 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20150826144820.GD2336@work-vm> References: <1434450415-11339-1-git-send-email-dgilbert@redhat.com> <1434450415-11339-19-git-send-email-dgilbert@redhat.com> <87k2u4l3ri.fsf@neno.neno> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87k2u4l3ri.fsf@neno.neno> Subject: Re: [Qemu-devel] [PATCH v7 18/42] Add wrappers and handlers for sending/receiving the postcopy-ram migration messages. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Juan Quintela 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 * Juan Quintela (quintela@redhat.com) wrote: > "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) Actually, I made this an atomic_mb_read as per Paolo's comment on my v5 version (31st March). I also added a comment documenting which threads read/write the state. > > +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? Yes, although I wouldn't want to make the wire format dependent on the host size_t or pointer size or anything. > > > +{ > > + 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. Done. > 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? Dave Gibson asked for the \0 in a previous review. > > > + buf[0] = 0; /* Version */ > > + assert(name_len < 256); > > Can we move the assert before the malloc()? Done. > 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) Done. > > > + 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 Done. > Humm, thinking about it, .... why are we not needing a length field of > number of entries? Because we've got the size of the whole message from the command header. > > + 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? Done; (as a const postcopy_ram_discard_version) Dave -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK