From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:59039)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <pbonzini@redhat.com>) id 1UocV5-0008NQ-BM
	for qemu-devel@nongnu.org; Mon, 17 Jun 2013 12:41:53 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <pbonzini@redhat.com>) id 1UocV3-0007uN-Nh
	for qemu-devel@nongnu.org; Mon, 17 Jun 2013 12:41:51 -0400
Received: from mx1.redhat.com ([209.132.183.28]:25417)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <pbonzini@redhat.com>) id 1UocV3-0007tN-G5
	for qemu-devel@nongnu.org; Mon, 17 Jun 2013 12:41:49 -0400
Message-ID: <51BF3C46.4090207@redhat.com>
Date: Mon, 17 Jun 2013 18:41:42 +0200
From: Paolo Bonzini <pbonzini@redhat.com>
MIME-Version: 1.0
References: <1371381681-14252-1-git-send-email-pingfanl@linux.vnet.ibm.com>
	<1371381681-14252-3-git-send-email-pingfanl@linux.vnet.ibm.com>
	<20130617152814.GB31444@stefanha-thinkpad.redhat.com>
In-Reply-To: <20130617152814.GB31444@stefanha-thinkpad.redhat.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH v2 2/2] QEMUBH: make AioContext's bh
	re-entrant
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: Stefan Hajnoczi <stefanha@gmail.com>
Cc: Liu Ping Fan <qemulist@gmail.com>, Anthony Liguori <anthony@codemonkey.ws>, qemu-devel@nongnu.org

Il 17/06/2013 17:28, Stefan Hajnoczi ha scritto:
>> > +    qemu_mutex_lock(&ctx->bh_lock);
>> >      bh->next = ctx->first_bh;
>> > +    /* Make sure the memebers ready before putting bh into list */
> s/memebers/members/
> 
>> > +    smp_wmb();
> Why lock bh_lock before assigning bh->next?  Could you lock the mutex
> here and then drop the smp_wmb() since the pthread function already does
> that?
> 
> http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_11

Not sure I understand, ctx->first_bh is read here and that's what the
lock protects.

    thread 1                           thread 2
    ------------------------------------------------------------------
    bh->next = ctx->first_bh;
                                       bh->next = ctx->first_bh;
                                       lock
                                       ctx->first_bh = bh;
                                       unlock
    lock
    ctx->first_bh = bh;
    unlock

and thread 2's bottom half is gone.  There is also a similar race that
leaves a dangling pointer if aio_bh_new races against aio_bh_poll.

Paolo