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

[Paolo Bonzini <pbonzini@redhat.com>]

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<udeshpan@redhat.com>

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