qemu-devel
[Top][All Lists]
Advanced

[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: Juan Quintela
Subject: Re: [Qemu-devel] [PATCH v7 18/42] Add wrappers and handlers for sending/receiving the postcopy-ram migration messages.
Date: Mon, 13 Jul 2015 13:02:09 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)

"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)

> +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?



reply via email to

[Prev in Thread] Current Thread [Next in Thread]