From mboxrd@z Thu Jan  1 00:00:00 1970
From: Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [RFC PATCH v2 2/3] fine grained qemu_mutex locking
	for migration
Date: Mon, 01 Aug 2011 11:39:54 +0200
Message-ID: <4E36746A.6040901@redhat.com>
References: <cover.1311971937.git.udeshpan@redhat.com>
	<f4440591ca4843adff3a588864199ace81e5f11c.1311971938.git.udeshpan@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Cc: mtosatti@redhat.com, qemu-devel@nongnu.org, kvm@vger.kernel.org,
	Juan Quintela <quintela@redhat.com>
To: Umesh Deshpande <udeshpan@redhat.com>
Return-path: <qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org>
In-Reply-To: <f4440591ca4843adff3a588864199ace81e5f11c.1311971938.git.udeshpan@redhat.com>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: </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>
Errors-To: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org
Sender: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org
List-Id: kvm.vger.kernel.org

On 07/29/2011 10:57 PM, Umesh Deshpande wrote:
> +    qemu_mutex_unlock_iothread();
>
>       while (s->state == MIG_STATE_ACTIVE) {
>           if (migrate_fd_check_expire()) {
> +            qemu_mutex_lock_iothread();
>               buffered_rate_tick(s->file);
> +            qemu_mutex_unlock_iothread();
>           }
>
>           if (s->state != MIG_STATE_ACTIVE) {
> @@ -392,12 +396,11 @@ void migrate_fd_begin(void *arg)
>
>           if (s->callback) {
>               migrate_fd_wait_for_unfreeze(s);
> +            qemu_mutex_lock_iothread();
>               s->callback(s);
> +            qemu_mutex_unlock_iothread();
>           }
>       }
> -
> -out:
> -    qemu_mutex_unlock_iothread();

I think it's clearer to unlock explicitly around the waiting points (see 
review of 1/3).  In fact, I think you're working around the busy wait by 
accessing s->state outside the lock, right?  I don't think this is 
provably safe; moving the knowledge of the thread entirely within 
buffered_file.c also fixes this, because then the lifetimes of the 
thread and the QEMUFile are much clearer.

Thanks,

Paolo