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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
