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
