Re: [Qemu-devel] [PATCH 5/5] Separate migration thread

[Paolo Bonzini <pbonzini@redhat.com>]

On 08/29/2011 08:49 PM, Marcelo Tosatti wrote:
>> >  -static void buffered_rate_tick(void *opaque)
>> >  +static void *migrate_vm(void *opaque)
>> >    {
>
> buffered_file.c was generic code that has now become migration specific
> (although migration was the only user). So it should either stop
> pretending to be generic code, by rename to migration_thread.c along
> with un-exporting interfaces, or it should remain generic and therefore
> all migration specific knowledge moved somewhere else.

Actually, the thread function is ill-named.  buffered_file.c is still 
generic code (or if it is not, it's a bug), except it should be called 
threaded_file.c.

Moving it to migration.c is also an option of course.  I asked Umesh to 
keep the abstraction for now, because it helped pinpointing places where 
abstractions were leaking in (such as the qemu_mutex_unlock_migrate_ram 
call that you found).

>> +    int64_t current_time, expire_time = qemu_get_clock_ms(rt_clock) + 100;
>> +    struct timeval tv = { .tv_sec = 0, .tv_usec = 100000};
>
> qemu_get_clock_ms should happen under iothread lock.

For rt_clock it is safe.  Should be documented, though.

>> +    qemu_mutex_lock_migrate_ram();
>>      s = migrate_to_fms(current_migration);
>>      if (s && s->file) {
>>          qemu_file_set_rate_limit(s->file, max_throttle);
>>      }
>> +    qemu_mutex_unlock_migrate_ram();
>
> This lock protects the RAMlist, and only the RAMlist, but here its
> being used to protect migration thread data. As noted above, a new lock
> should be introduced.

Even better, freeing the buffered_file should be only done in the 
iothread (if this is not the case) so that the lock can be pushed down 
to buffered_set_rate_limit...

> +        qemu_mutex_lock_migrate_ram();
>          if (qemu_fclose(s->file) != 0) {
>              ret = -1;
>          }
> +        qemu_mutex_unlock_migrate_ram();

... and buffered_close (if a lock turns out to be needed at all).

Paolo