[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v7 18/42] Add wrappers and handlers for sending/
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [Qemu-devel] [PATCH v7 18/42] Add wrappers and handlers for sending/receiving the postcopy-ram migration messages. |
Date: |
Wed, 26 Aug 2015 15:48:21 +0100 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
* Juan Quintela (address@hidden) wrote:
> "Dr. David Alan Gilbert (git)" <address@hidden> wrote:
> > From: "Dr. David Alan Gilbert" <address@hidden>
> >
> > 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 <address@hidden>
> > ---
> > 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 / address@hidden / Manchester, UK
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-devel] [PATCH v7 18/42] Add wrappers and handlers for sending/receiving the postcopy-ram migration messages.,
Dr. David Alan Gilbert <=