Re: [Qemu-devel] [PATCH 23/42] migration: yay, buffering is gone

Re: [Qemu-devel] [PATCH 23/42] migration: yay, buffering is gone

[Wenchao Xia]

Hi, Paolo
  Do you think static buffer in qemu_file should be also removed?

> Buffering was needed because blocking writes could take a long time
> and starve other threads seeking to grab the big QEMU mutex.
> 
> Now that all writes (except within _complete callbacks) are done
> outside the big QEMU mutex, we do not need buffering at all.
> 
> Reviewed-by: Orit Wasserman <owasserm@redhat.com>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   include/migration/migration.h |  3 --
>   migration.c                   | 79 ++++++++++++-------------------------------
>   savevm.c                      |  1 +
>   3 files changed, 22 insertions(+), 61 deletions(-)
> 
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index ed20bed..cec8643 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -34,9 +34,6 @@ struct MigrationState
>       int64_t bandwidth_limit;
>       size_t bytes_xfer;
>       size_t xfer_limit;
> -    uint8_t *buffer;
> -    size_t buffer_size;
> -    size_t buffer_capacity;
>       QemuThread thread;
>       QEMUBH *cleanup_bh;
> 
> diff --git a/migration.c b/migration.c
> index e64c92d..4c8d576 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -514,73 +514,41 @@ int64_t migrate_xbzrle_cache_size(void)
> 
>   /* migration thread support */
> 
> -
> -static void buffered_flush(MigrationState *s)
> -{
> -    size_t offset = 0;
> -    ssize_t ret = 0;
> -
> -    DPRINTF("flushing %zu byte(s) of data\n", s->buffer_size);
> -
> -    if (qemu_file_get_error(s->file)) {
> -        s->buffer_size = 0;
> -        return;
> -    }
> -    qemu_fflush(s->file);
> -
> -    while (s->bytes_xfer < s->xfer_limit && offset < s->buffer_size) {
> -        size_t to_send = MIN(s->buffer_size - offset, s->xfer_limit - s->bytes_xfer);
> -        ret = migrate_fd_put_buffer(s, s->buffer + offset, to_send);
> -        if (ret <= 0) {
> -            DPRINTF("error flushing data, %zd\n", ret);
> -            break;
> -        } else {
> -            DPRINTF("flushed %zd byte(s)\n", ret);
> -            offset += ret;
> -            s->bytes_xfer += ret;
> -        }
> -    }
> -
> -    DPRINTF("flushed %zu of %zu byte(s)\n", offset, s->buffer_size);
> -    memmove(s->buffer, s->buffer + offset, s->buffer_size - offset);
> -    s->buffer_size -= offset;
> -
> -    if (ret < 0) {
> -        qemu_file_set_error(s->file, ret);
> -    }
> -}
> -
>   static int buffered_put_buffer(void *opaque, const uint8_t *buf,
>                                  int64_t pos, int size)
>   {
>       MigrationState *s = opaque;
> -    ssize_t error;
> +    ssize_t ret;
> +    size_t sent;
> 
>       DPRINTF("putting %d bytes at %" PRId64 "\n", size, pos);
> 
> -    error = qemu_file_get_error(s->file);
> -    if (error) {
> -        DPRINTF("flush when error, bailing: %s\n", strerror(-error));
> -        return error;
> +    ret = qemu_file_get_error(s->file);
> +    if (ret) {
> +        DPRINTF("flush when error, bailing: %s\n", strerror(-ret));
> +        return ret;
>       }
> 
>       if (size <= 0) {
>           return size;
>       }
> 
> -    if (size > (s->buffer_capacity - s->buffer_size)) {
> -        DPRINTF("increasing buffer capacity from %zu by %zu\n",
> -                s->buffer_capacity, size + 1024);
> -
> -        s->buffer_capacity += size + 1024;
> -
> -        s->buffer = g_realloc(s->buffer, s->buffer_capacity);
> +    sent = 0;
> +    while (size) {
> +        ret = migrate_fd_put_buffer(s, buf, size);
> +        if (ret <= 0) {
> +            DPRINTF("error flushing data, %zd\n", ret);
> +            return ret;
> +        } else {
> +            DPRINTF("flushed %zd byte(s)\n", ret);
> +            sent += ret;
> +            buf += ret;
> +            size -= ret;
> +            s->bytes_xfer += ret;
> +        }
>       }
> 
> -    memcpy(s->buffer + s->buffer_size, buf, size);
> -    s->buffer_size += size;
> -
> -    return size;
> +    return sent;
>   }
> 
>   static int buffered_close(void *opaque)
> @@ -712,10 +680,9 @@ static void *buffered_file_thread(void *opaque)
>               g_usleep((initial_time + BUFFER_DELAY - current_time)*1000);
>               sleep_time += qemu_get_clock_ms(rt_clock) - current_time;
>           }
> -        buffered_flush(s);
>           if (qemu_file_get_error(s->file)) {
>               migrate_finish_set_state(s, MIG_STATE_ERROR);
> -        } else if (last_round && s->buffer_size == 0) {
> +        } else if (last_round) {
>               migrate_finish_set_state(s, MIG_STATE_COMPLETED);
>           }
>       }
> @@ -735,7 +702,6 @@ static void *buffered_file_thread(void *opaque)
>       qemu_bh_schedule(s->cleanup_bh);
>       qemu_mutex_unlock_iothread();
> 
> -    g_free(s->buffer);
>       return NULL;
>   }
> 
> @@ -754,9 +720,6 @@ void migrate_fd_connect(MigrationState *s)
>       trace_migrate_set_state(MIG_STATE_ACTIVE);
> 
>       s->bytes_xfer = 0;
> -    s->buffer = NULL;
> -    s->buffer_size = 0;
> -    s->buffer_capacity = 0;
>       /* This is a best 1st approximation. ns to ms */
>       s->expected_downtime = max_downtime/1000000;
> 
> diff --git a/savevm.c b/savevm.c
> index 7c7774e..ce10295 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1724,6 +1724,7 @@ void qemu_savevm_state_complete(QEMUFile *f)
>       }
> 
>       qemu_put_byte(f, QEMU_VM_EOF);
> +    qemu_fflush(f);
>   }
> 
>   uint64_t qemu_savevm_state_pending(QEMUFile *f, uint64_t max_size)
> 


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 23/42] migration: yay, buffering is gone

[Paolo Bonzini]

> Hi, Paolo
>   Do you think static buffer in qemu_file should be also removed?

Orit is working on that.

Paolo

Re: [Qemu-devel] [PATCH 14/42] migration: prepare to access s->state outside critical sections

[Stefan Hajnoczi]

On Fri, Feb 22, 2013 at 5:36 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Accessing s->state outside the big QEMU lock will simplify a bit the
> locking/unlocking of the iothread lock.
>
> We need to keep the lock in migrate_fd_error and migrate_fd_completed,
> however, because they call migrate_fd_cleanup.
>
> Reviewed-by: Orit Wasserman <owasserm@redhat.com>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  migration.c | 33 +++++++++++++++++++++------------
>  1 file changed, 21 insertions(+), 12 deletions(-)
>
> diff --git a/migration.c b/migration.c
> index b091532..b40755f 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -279,19 +279,25 @@ static void migrate_fd_cleanup(MigrationState *s)
>      notifier_list_notify(&migration_state_notifiers, s);
>  }
>
> +static void migrate_finish_set_state(MigrationState *s, int new_state)
> +{
> +    if (__sync_val_compare_and_swap(&s->state, MIG_STATE_ACTIVE,

kraxel_rhel61's mingw build fails:

  LINK  i386-softmmu/qemu-system-i386.exe
../migration.o:migration.c:(.text+0x408): undefined reference to
`__sync_val_compare_and_swap_4'
../migration.o:migration.c:(.text+0x7af): undefined reference to
`__sync_val_compare_and_swap_4'
../migration.o:migration.c:(.text+0x827): undefined reference to
`__sync_val_compare_and_swap_4'

http://buildbot.b1-systems.de/qemu/builders/default_mingw32/builds/566/steps/compile/logs/stdio

The latest RHEL6 mingw gcc is version 4.4 but this buildslave is
RHEL6.1 so perhaps it's an older version that is missing the atomics
builtins?

Stefan

Re: [Qemu-devel] [PATCH 14/42] migration: prepare to access s->state outside critical sections

[Gerd Hoffmann]

  Hi,

> http://buildbot.b1-systems.de/qemu/builders/default_mingw32/builds/566/steps/compile/logs/stdio
> 
> The latest RHEL6 mingw gcc is version 4.4 but this buildslave is
> RHEL6.1 so perhaps it's an older version that is missing the atomics
> builtins?

No, it's RHEL-6.4 actually, even though the name suggests otherwise (was
a bad idea to include the minor rev in the buildslave name ...).

There is a detailed slave info page btw:
http://buildbot.b1-systems.de/qemu/buildslaves/kraxel_rhel61

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 14/42] migration: prepare to access s->state outside critical sections

[Paolo Bonzini]

----- Messaggio originale -----
> Da: "Gerd Hoffmann" <kraxel@redhat.com>
> A: "Stefan Hajnoczi" <stefanha@gmail.com>
> Cc: "Paolo Bonzini" <pbonzini@redhat.com>, "qemu-devel" <qemu-devel@nongnu.org>, "Orit Wasserman"
> <owasserm@redhat.com>, "Juan Quintela" <quintela@redhat.com>
> Inviato: Lunedì, 25 marzo 2013 10:52:36
> Oggetto: Re: [Qemu-devel] [PATCH 14/42] migration: prepare to access s->state outside critical sections
> 
> Hi,
> 
> > http://buildbot.b1-systems.de/qemu/builders/default_mingw32/builds/566/steps/compile/logs/stdio
> > 
> > The latest RHEL6 mingw gcc is version 4.4 but this buildslave is
> > RHEL6.1 so perhaps it's an older version that is missing the
> > atomics builtins?
> 
> No, it's RHEL-6.4 actually, even though the name suggests otherwise
> (was a bad idea to include the minor rev in the buildslave name ...).

Can you check if this fixes it?

diff --git a/configure b/configure
index 46a7594..a324ca5 100755
--- a/configure
+++ b/configure
@@ -931,9 +931,9 @@ case "$cpu" in
            LDFLAGS="-m64 $LDFLAGS"
            ;;
     i386)
-           QEMU_CFLAGS="-m32 $QEMU_CFLAGS"
-           LDFLAGS="-m32 $LDFLAGS"
-           cc_i386='$(CC) -m32'
+           QEMU_CFLAGS="-m32 -mcpu=i486 $QEMU_CFLAGS"
+           LDFLAGS="-m32 -mcpu=i486 $LDFLAGS"
+           cc_i386='$(CC) -m32 -mcpu=i486'
            ;;
     x86_64)
            QEMU_CFLAGS="-m64 $QEMU_CFLAGS"

Paolo

Re: [Qemu-devel] [PATCH 14/42] migration: prepare to access s->state outside critical sections

[Stefan Weil]

Am 25.03.2013 12:02, schrieb Paolo Bonzini:
>
> ----- Messaggio originale -----
>> Da: "Gerd Hoffmann" <kraxel@redhat.com>
>> A: "Stefan Hajnoczi" <stefanha@gmail.com>
>> Cc: "Paolo Bonzini" <pbonzini@redhat.com>, "qemu-devel" <qemu-devel@nongnu.org>, "Orit Wasserman"
>> <owasserm@redhat.com>, "Juan Quintela" <quintela@redhat.com>
>> Inviato: Lunedì, 25 marzo 2013 10:52:36
>> Oggetto: Re: [Qemu-devel] [PATCH 14/42] migration: prepare to access s->state outside critical sections
>>
>> Hi,
>>
>>> http://buildbot.b1-systems.de/qemu/builders/default_mingw32/builds/566/steps/compile/logs/stdio
>>>
>>> The latest RHEL6 mingw gcc is version 4.4 but this buildslave is
>>> RHEL6.1 so perhaps it's an older version that is missing the
>>> atomics builtins?
>> No, it's RHEL-6.4 actually, even though the name suggests otherwise
>> (was a bad idea to include the minor rev in the buildslave name ...).
> Can you check if this fixes it?
>
> diff --git a/configure b/configure
> index 46a7594..a324ca5 100755
> --- a/configure
> +++ b/configure
> @@ -931,9 +931,9 @@ case "$cpu" in
>             LDFLAGS="-m64 $LDFLAGS"
>             ;;
>      i386)
> -           QEMU_CFLAGS="-m32 $QEMU_CFLAGS"
> -           LDFLAGS="-m32 $LDFLAGS"
> -           cc_i386='$(CC) -m32'
> +           QEMU_CFLAGS="-m32 -mcpu=i486 $QEMU_CFLAGS"
> +           LDFLAGS="-m32 -mcpu=i486 $LDFLAGS"
> +           cc_i386='$(CC) -m32 -mcpu=i486'
>             ;;
>      x86_64)
>             QEMU_CFLAGS="-m64 $QEMU_CFLAGS"
>
> Paolo

Hi Stefan, hi Paolo,

cross compilation on Debian Squeeze shows the same error.

It is fixed by setting the correct cpu architecture (-march=i686).
As far as I know, gcc 4.4 defaults to -march=i386 which does
not support the atomic operations.

I have sent a patch today - please try it.

Regards,
Stefan