| Message ID | 20160831222909.26983-1-bobby.prani@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/09/2016 00:29, Pranith Kumar wrote: > smp_read_barrier_depends() should be used only if you are reading > dependent pointers which are shared. bh is shared since it is equal to ctx->first_bh or ctx->first_bh->...->next. While the compiler will always order the load of bh->next after the load of ctx->first_bh and any previous load of bh->next, this may not be the case for the processor. Only the DEC Alpha has this behavior, but it _can_ happen. Paolo 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. > > This patch removes the barrier and adds a comment why storing > 'bh->next' is necessary. > > Signed-off-by: Pranith Kumar <bobby.prani@gmail.com> > --- > async.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/async.c b/async.c > index 3bca9b0..6b691aa 100644 > --- a/async.c > +++ b/async.c > @@ -77,8 +77,7 @@ int aio_bh_poll(AioContext *ctx) > > ret = 0; > for (bh = ctx->first_bh; bh; bh = next) { > - /* Make sure that fetching bh happens before accessing its members */ > - smp_read_barrier_depends(); > + /* 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 >
On Thu, Sep 1, 2016 at 7:01 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 01/09/2016 00:29, Pranith Kumar wrote: >> smp_read_barrier_depends() should be used only if you are reading >> dependent pointers which are shared. > > bh is shared since it is equal to ctx->first_bh or > ctx->first_bh->...->next. While the compiler will always order the load > of bh->next after the load of ctx->first_bh and any previous load of > bh->next, this may not be the case for the processor. Only the DEC > Alpha has this behavior, but it _can_ happen. In that case there are a bunch of places in the same file where we need to insert this barrier. I also guarantee that there are many more files which would need this barrier if we examine closely. I really doubt if we see this kind of re-ordering. Did we ever get any bug reports about running qemu on an SMP Alpha? smp_read_barrier_depends() is evil and I would prefer we get rid of it. Thanks,
On 01/09/2016 16:40, Pranith Kumar wrote: >> > >> > bh is shared since it is equal to ctx->first_bh or >> > ctx->first_bh->...->next. While the compiler will always order the load >> > of bh->next after the load of ctx->first_bh and any previous load of >> > bh->next, this may not be the case for the processor. Only the DEC >> > Alpha has this behavior, but it _can_ happen. > In that case there are a bunch of places in the same file where we > need to insert this barrier. I also guarantee that there are many more > files which would need this barrier if we examine closely. If it's protected by a lock, it's not necessary. If you don't like smp_read_barrier_depends, you can introduce a synonym for atomic_rcu_read with another name. But the functionality is required. Paolo
diff --git a/async.c b/async.c index 3bca9b0..6b691aa 100644 --- a/async.c +++ b/async.c @@ -77,8 +77,7 @@ int aio_bh_poll(AioContext *ctx) ret = 0; for (bh = ctx->first_bh; bh; bh = next) { - /* Make sure that fetching bh happens before accessing its members */ - smp_read_barrier_depends(); + /* 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
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. This patch removes the barrier and adds a comment why storing 'bh->next' is necessary. Signed-off-by: Pranith Kumar <bobby.prani@gmail.com> --- async.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)