qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Paolo Bonzini
Subject: Re: [Qemu-devel] [PATCH 5/5] Separate migration thread
Date: Tue, 30 Aug 2011 10:48:11 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:6.0) Gecko/20110816 Thunderbird/6.0

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]