Re: [Qemu-devel] [RFC PATCH v2 1/3] separate thread for VM migration

[Paolo Bonzini]

On 07/29/2011 10:57 PM, Umesh Deshpande wrote:
> This patch creates a separate thread for the guest migration on the source side.
>
> Signed-off-by: Umesh Deshpande<address@hidden>

Looks pretty good!

One thing that shows, is that the interface separation between 
buffered_file.c is migration.c is pretty weird.  Your patch makes it 
somewhat worse, but it was like this before so it's not your fault.  The 
good thing is that if buffered_file.c uses threads, you can fix a large 
part of this and get even simpler code:

1) there is really just one way to implement migrate_fd_put_notify, and 
with your simplifications it does not belong anymore in migration.c.

2) s->callback is actually not NULL exactly if s->file->frozen_output is 
true, you can remove it as well;

3) buffered_close is messy because it can be called from both the 
iothread (monitor->migrate_fd_cancel->migrate_fd_cleanup->qemu_fclose) 
or the migration thread (after qemu_savevm_state_complete).  But 
buffered_close is actually very similar to your thread function (it does 
flush+wait_for_unfreeze, basically)!  So buffered_close can be simply:

    s->closed = 1;
    ret = qemu_thread_join(s->thread); /* doesn't exist yet :) */
    qemu_free(...);
    return ret;

Another nit is that here:

> +        if (migrate_fd_check_expire()) {
> +            buffered_rate_tick(s->file);
> +        }
> +
> +        if (s->state != MIG_STATE_ACTIVE) {
> +            break;
> +        }
> +
> +        if (s->callback) {
> +            migrate_fd_wait_for_unfreeze(s);
> +            s->callback(s);
> +        }

you can still have a busy wait.

Putting it all together, you can move the thread function back to 
buffered_file.c like:

    while (!s->closed || (!s->has_error && s->buffer_size)) {
        if (s->freeze_output) {
            qemu_mutex_unlock_iothread();
            s->wait_for_unfreeze(s);
            qemu_mutex_lock_iothread();
            /* This comes from qemu_file_put_notify (via
               buffered_put_buffer---can be simplified a lot too?).
            s->freeze_output = 0;
            /* Test again for cancellation.  */
            continue;
        }

        int64_t current_time = qemu_get_clock_ms(rt_clock);
        if (s->expire_time > current_time) {
            struct timeval tv = { .tv_sec = 0, .tv_usec = ... };
            qemu_mutex_unlock_iothread();
            select (0, NULL, NULL, NULL, &tv);
            qemu_mutex_lock_iothread();
            s->expire_time = qemu_get_clock_ms(rt_clock) + 100;
            continue;
        }

        /* This comes from buffered_rate_tick.  */
        s->bytes_xfer = 0;
        buffered_flush(s);
        if (!s->closed) {
            s->put_ready(s->opaque);
        }
    }

    ret = s->close(s->opaque);
    ...

Does it look sane?

Paolo