On 08/21/2011 03:41 AM, Umesh Deshpande wrote:
This should be run under the iothread lock. Pay attention to avoiding
lock inversion: the I/O thread always takes the iothread lock outside
and the ramlist lock within, so the migration thread must do the same.
BTW, I think this code in the migration thread patch also needs the
iothread lock:
if (stage < 0) {
cpu_physical_memory_set_dirty_tracking(0);
return 0;
}
if (cpu_physical_sync_dirty_bitmap(0, TARGET_PHYS_ADDR_MAX) != 0) {
qemu_file_set_error(f);
return 0;
}
Callers of above code snippets (sync_migration_bitmap etc.) are holding
the iothread mutex. It has been made sure that the original qemu dirty
bitmap is only accessed when holding the mutex.
But you cannot do it like in this patch, because here you have a deadlock:
+ if (stage != 3) {
+ qemu_mutex_lock_ramlist();
+ qemu_mutex_unlock_iothread();
+ }
+
while (!qemu_file_rate_limit(f)) {
int bytes_sent;
@@ -308,6 +315,11 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage,
void *opaque)
}
}
+ if (stage != 3) {
+ qemu_mutex_lock_iothread();
Lock order: ramlist, iothread. The I/O thread instead takes the
iothread lock outside and the ramlist lock inside. All this makes me
even more convinced that you're locking is both too coarse and too
complicated (perhaps it's not complicated, it's just under-documented;
but the coarseness problem is there and it's what causes these lock
inversions).
+ qemu_mutex_unlock_ramlist();
+ }
+
Paolo
