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

[Marcelo Tosatti]

On Tue, Aug 30, 2011 at 10:48:11AM +0200, Paolo Bonzini wrote:
> 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).

Fair enough, its indeed generic except misuse of ram lock.

> >>+    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...

Sounds good.

> >+        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