[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] aio: reg. smp_read_barrier_depends() in aio_bh_poll()
From: |
Pranith Kumar |
Subject: |
[Qemu-devel] aio: reg. smp_read_barrier_depends() in aio_bh_poll() |
Date: |
Fri, 02 Sep 2016 10:33:15 -0400 |
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 <address@hidden>
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 <address@hidden>
---
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
- [Qemu-devel] aio: reg. smp_read_barrier_depends() in aio_bh_poll(),
Pranith Kumar <=