[Qemu-devel] aio: reg. smp_read_barrier_depends() in aio_bh_poll()

[Pranith Kumar <bobby.prani@gmail.com>]

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 <bobby.prani@gmail.com>
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 <bobby.prani@gmail.com>
---
 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