Re: [Qemu-devel] [PATCH v7 18/42] Add wrappers and handlers for sending/receiving the postcopy-ram migration messages.

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Juan Quintela (quintela@redhat.com) wrote:
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > 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 <dgilbert@redhat.com>
> > ---
> >  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