Re: [Qemu-devel] [PATCH 22/30] migration: unfold rest of migrate_fd_put_ready() into thread

[Paolo Bonzini <pbonzini@redhat.com>]

Il 18/10/2012 09:30, Juan Quintela ha scritto:
> This will allow us finer control in next patches.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration.c | 95 ++++++++++++++++++++++++++-----------------------------------
>  1 file changed, 41 insertions(+), 54 deletions(-)
> 
> diff --git a/migration.c b/migration.c
> index 7206866..e6ff1f1 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -644,54 +644,6 @@ static int64_t buffered_get_rate_limit(void *opaque)
>      return s->xfer_limit;
>  }
> 
> -static bool migrate_fd_put_ready(MigrationState *s, uint64_t max_size)
> -{
> -    int ret;
> -    uint64_t pending_size;
> -    bool last_round = false;
> -
> -    qemu_mutex_lock_iothread();
> -    DPRINTF("iterate\n");
> -    pending_size = qemu_savevm_state_pending(s->file, max_size);
> -    DPRINTF("pending size %lu max %lu\n", pending_size, max_size);
> -    if (pending_size >= max_size) {
> -        ret = qemu_savevm_state_iterate(s->file);
> -        if (ret < 0) {
> -            migrate_fd_error(s);
> -        }
> -    } else {
> -        int old_vm_running = runstate_is_running();
> -        int64_t start_time, end_time;
> -
> -        DPRINTF("done iterating\n");
> -        start_time = qemu_get_clock_ms(rt_clock);
> -        qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
> -        if (old_vm_running) {
> -            vm_stop(RUN_STATE_FINISH_MIGRATE);
> -        } else {
> -            vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
> -        }
> -
> -        if (qemu_savevm_state_complete(s->file) < 0) {
> -            migrate_fd_error(s);
> -        } else {
> -            migrate_fd_completed(s);
> -        }
> -        end_time = qemu_get_clock_ms(rt_clock);
> -        s->total_time = end_time - s->total_time;
> -        s->downtime = end_time - start_time;
> -        if (s->state != MIG_STATE_COMPLETED) {
> -            if (old_vm_running) {
> -                vm_start();
> -            }
> -        }
> -        last_round = true;
> -    }
> -    qemu_mutex_unlock_iothread();
> -
> -    return last_round;
> -}
> -
>  /* 100ms  xfer_limit is the limit that we should write each 100ms */
>  #define BUFFER_DELAY 100
> 
> @@ -716,6 +668,7 @@ static void *buffered_file_thread(void *opaque)
> 
>      while (true) {
>          int64_t current_time = qemu_get_clock_ms(rt_clock);
> +        uint64_t pending_size;
> 
>          qemu_mutex_lock_iothread();
>          if (m->state != MIG_STATE_ACTIVE) {
> @@ -727,6 +680,46 @@ static void *buffered_file_thread(void *opaque)
>              qemu_mutex_unlock_iothread();
>              break;
>          }
> +        if (s->bytes_xfer < s->xfer_limit) {
> +            DPRINTF("iterate\n");
> +            pending_size = qemu_savevm_state_pending(m->file, max_size);
> +            DPRINTF("pending size %lu max %lu\n", pending_size, max_size);
> +            if (pending_size >= max_size) {
> +                ret = qemu_savevm_state_iterate(m->file);

So RAM migration is still being run inside the BQL, isn't it?

> +                if (ret < 0) {
> +                    qemu_mutex_unlock_iothread();
> +                    break;

There's a lot of

                    qemu_mutex_unlock_iothread();
                    break;

in this function.  Perhaps it is better if you make an invariant that
the loop is entered and exited with the BQL taken, and it is only
unlocked in the middle.  It makes sense once you fold everything in
migration.c.

It can be a separate patch though.

> +                }
> +            } else {
> +                int old_vm_running = runstate_is_running();
> +                int64_t start_time, end_time;
> +
> +                DPRINTF("done iterating\n");
> +                start_time = qemu_get_clock_ms(rt_clock);
> +                qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
> +                if (old_vm_running) {
> +                    vm_stop(RUN_STATE_FINISH_MIGRATE);
> +                } else {
> +                    vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
> +                }
> +                ret = qemu_savevm_state_complete(m->file);
> +                if (ret < 0) {
> +                    qemu_mutex_unlock_iothread();
> +                    break;
> +                } else {
> +                    migrate_fd_completed(m);
> +                }
> +                end_time = qemu_get_clock_ms(rt_clock);
> +                m->total_time = end_time - m->total_time;
> +                m->downtime = end_time - start_time;
> +                if (m->state != MIG_STATE_COMPLETED) {
> +                    if (old_vm_running) {
> +                        vm_start();
> +                    }
> +                }
> +                last_round = true;
> +            }
> +        }
>          qemu_mutex_unlock_iothread();
> 
>          if (current_time >= initial_time + BUFFER_DELAY) {
> @@ -747,12 +740,6 @@ static void *buffered_file_thread(void *opaque)
>              usleep((initial_time + BUFFER_DELAY - current_time)*1000);
>          }
>          buffered_flush(s);
> -
> -        DPRINTF("file is ready\n");
> -        if (s->bytes_xfer < s->xfer_limit) {
> -            DPRINTF("notifying client\n");
> -            last_round = migrate_fd_put_ready(m, max_size);
> -        }
>      }
> 
>  out:
>