Re: [RFC PATCH v3 1/4] separate thread for VM migration

From: Umesh Deshpande <udeshpan@redhat.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: kvm@vger.kernel.org, quintela@redhat.com, mtosatti@redhat.com
Subject: Re: [RFC PATCH v3 1/4] separate thread for VM migration
Date: Thu, 11 Aug 2011 13:36:02 -0400	[thread overview]
Message-ID: <4E441302.5090102@redhat.com> (raw)
In-Reply-To: <4E4400EE.1030905@redhat.com>

On 08/11/2011 12:18 PM, Paolo Bonzini wrote:
>> @@ -175,20 +170,20 @@ static int buffered_close(void *opaque)
>>
>>       while (!s->has_error&&  s->buffer_size) {
>>           buffered_flush(s);
>> -        if (s->freeze_output)
>> +        if (s->freeze_output) {
>>               s->wait_for_unfreeze(s);
>> +        }
>>       }
>
> This is racy; you might end up calling buffered_put_buffer twice from 
> two different threads.
Now, migrate_fd_cleanup, buffured_close is just executed by the 
migration thread.
I am not letting iothread call any migration cancellation related 
functions. In stead it just submits the request and waits for the 
migration thread to terminate itself in the next iteration.
The reason is to avoid the call to qemu_fflush,  
qemu_savevm_state_cancel (to carry out migrate_cancel) from iothread 
while migration thread is transferring data without holding the locks.

>
>> -    ret = s->close(s->opaque);
>> +    s->closed = 1;
>>
>> -    qemu_del_timer(s->timer);
>> -    qemu_free_timer(s->timer);
>> +    ret = s->close(s->opaque);
>>       qemu_free(s->buffer);
>> -    qemu_free(s);
>
> ... similarly, here the migration thread might end up using the 
> buffer.  Just set s->closed here and wait for thread completion; the 
> migration thread can handle the flushes free the buffer etc.  Let the 
> migration thread do as much as possible, it will simplify your life.
>
>>       return ret;
>>   }
>>
>> +
>>   static int buffered_rate_limit(void *opaque)
>>   {
>>       QEMUFileBuffered *s = opaque;
>> @@ -228,34 +223,55 @@ static int64_t buffered_get_rate_limit(void 
>> *opaque)
>>       return s->xfer_limit;
>>   }
>>
>> -static void buffered_rate_tick(void *opaque)
>> +static void *migrate_vm(void *opaque)
>>   {
>>       QEMUFileBuffered *s = opaque;
>> +    int64_t current_time, expire_time = qemu_get_clock_ms(rt_clock) 
>> + 100;
>> +    struct timeval tv = { .tv_sec = 0, .tv_usec = 100000};
>>
>> -    if (s->has_error) {
>> -        buffered_close(s);
>> -        return;
>> -    }
>> +    qemu_mutex_lock_iothread();
>>
>> -    qemu_mod_timer(s->timer, qemu_get_clock_ms(rt_clock) + 100);
>> +    while (!s->closed) {
>
> ... This can be in fact
>
>     while (!s->closed || s->buffered_size)
>
> and that alone will subsume the loop in buffered_close, no?
s->fd is closed in migrate_fd_cleanup (which calls buffered_close). So I 
flush the buffer in buffered_close before closing the descriptor, and 
then migration thread simply exits because s->closed is set.

- Umesh