Re: [PATCH 5/5] Separate migration thread

Paolo Bonzini
2011-08-30T08:48:17Z

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