From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57022) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bfpWz-00044D-ON for qemu-devel@nongnu.org; Fri, 02 Sep 2016 10:33:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bfpWv-0000AW-I4 for qemu-devel@nongnu.org; Fri, 02 Sep 2016 10:33:20 -0400 Received: from mail-yw0-x244.google.com ([2607:f8b0:4002:c05::244]:36549) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bfpWv-0000AE-DW for qemu-devel@nongnu.org; Fri, 02 Sep 2016 10:33:17 -0400 Received: by mail-yw0-x244.google.com with SMTP id z124so2621539ywz.3 for ; Fri, 02 Sep 2016 07:33:17 -0700 (PDT) From: Pranith Kumar Date: Fri, 02 Sep 2016 10:33:15 -0400 Message-ID: <87shti74no.fsf@gmail.com> MIME-Version: 1.0 Content-Type: text/plain Subject: [Qemu-devel] aio: reg. smp_read_barrier_depends() in aio_bh_poll() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: pbonzini@redhat.com, qemu-devel@nongnu.org Hi Paolo, This is in reference to the discussion we had yesterday on IRC. I am trying to understand the need for smp_read_barrier_depends() and how it prevents the following race condition. I think a regular barrier() should suffice instead of smp_read_barrier_depends(). Consider: P0 P1 ---------------------------------------- bh = ctx->first_bh; smp_read_barrier_depends(); // barrier() should be sufficient since bh // is local variable next = bh->next; lock(bh_lock); new_bh->next = ctx->first_bh; smp_wmb(); ctx->first_bh = new_bh; unlock(bh_lock); if (bh) { // do something } Why do you think smp_read_barrier_depends() is necessary here? If bh was a shared variable I would understand, but here bh is local and a regular barrier() would make sure that we are not optimizing the initial load into bh. A patch fixing this follows. Thanks, -- Pranith From: Pranith Kumar Date: Fri, 2 Sep 2016 10:30:23 -0400 Subject: [PATCH] aio: Remove spurious smp_read_barrier_depends() smp_read_barrier_depends() should be used only if you are reading dependent pointers which are shared. Here 'bh' is a local variable and dereferencing it will always be ordered after loading 'bh', i.e., bh->next will always be ordered after fetching bh. However the initial load into 'bh' should not be optimized away, hence we use atomic_read() to ensure this. Signed-off-by: Pranith Kumar --- async.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/async.c b/async.c index 3bca9b0..f4f8b17 100644 --- a/async.c +++ b/async.c @@ -76,9 +76,8 @@ int aio_bh_poll(AioContext *ctx) ctx->walking_bh++; ret = 0; - for (bh = ctx->first_bh; bh; bh = next) { - /* Make sure that fetching bh happens before accessing its members */ - smp_read_barrier_depends(); + for (bh = atomic_read(&ctx->first_bh); bh; bh = next) { + /* store bh->next since bh can be freed in aio_bh_call() */ next = bh->next; /* The atomic_xchg is paired with the one in qemu_bh_schedule. The * implicit memory barrier ensures that the callback sees all writes -- 2.9.3