From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([140.186.70.92]:54630)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <paolo.bonzini@gmail.com>) id 1QvPuP-0006Cz-7G
	for qemu-devel@nongnu.org; Mon, 22 Aug 2011 04:31:02 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <paolo.bonzini@gmail.com>) id 1QvPuO-0002jd-6v
	for qemu-devel@nongnu.org; Mon, 22 Aug 2011 04:31:01 -0400
Received: from mail-gy0-f173.google.com ([209.85.160.173]:64119)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <paolo.bonzini@gmail.com>) id 1QvPuO-0002jU-4R
	for qemu-devel@nongnu.org; Mon, 22 Aug 2011 04:31:00 -0400
Received: by gyd12 with SMTP id 12so4070811gyd.4
	for <qemu-devel@nongnu.org>; Mon, 22 Aug 2011 01:30:59 -0700 (PDT)
Sender: Paolo Bonzini <paolo.bonzini@gmail.com>
Message-ID: <4E5213BC.1070905@redhat.com>
Date: Mon, 22 Aug 2011 10:30:52 +0200
From: Paolo Bonzini <pbonzini@redhat.com>
MIME-Version: 1.0
References: <4E4E5C3E.4010603@redhat.com> <4E506250.6050405@redhat.com>
In-Reply-To: <4E506250.6050405@redhat.com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [RFC PATCH v4 3/5] separate migration bitmap
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Umesh Deshpande <udeshpan@redhat.com>
Cc: qemu-devel <qemu-devel@nongnu.org>

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