Messages in this thread Patch in this message | | | From | Will Deacon <> | Subject | [PATCH 04/13] vhost: Remove redundant use of read_barrier_depends() barrier | Date | Fri, 8 Nov 2019 17:01:11 +0000 |
| |
Since commit 76ebbe78f739 ("locking/barriers: Add implicit smp_read_barrier_depends() to READ_ONCE()"), there is no need to use 'smp_read_barrier_depends()' outside of the Alpha architecture code.
Unfortunately, there is precisely >one< user in the vhost code, and there isn't an obvious 'READ_ONCE()' access making the barrier redundant. However, on closer inspection (thanks, Jason), it appears that vring synchronisation between the producer and consumer occurs via the 'avail_idx' field, which is followed up by an 'rmb()' in 'vhost_get_vq_desc()', making the 'read_barrier_depends()' redundant on Alpha.
Jason says:
| I'm also confused about the barrier here, basically in driver side | we did: | | 1) allocate pages | 2) store pages in indirect->addr | 3) smp_wmb() | 4) increase the avail idx (somehow a tail pointer of vring) | | in vhost we did: | | 1) read avail idx | 2) smp_rmb() | 3) read indirect->addr | 4) read from indirect->addr | | It looks to me even the data dependency barrier is not necessary | since we have rmb() which is sufficient for us to the correct | indirect->addr and driver are not expected to do any writing to | indirect->addr after avail idx is increased
Remove the redundant barrier invocation.
Suggested-by: Jason Wang <jasowang@redhat.com> Signed-off-by: Will Deacon <will@kernel.org> --- drivers/vhost/vhost.c | 5 ----- 1 file changed, 5 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index 36ca2cf419bf..865bc91b783c 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -2128,11 +2128,6 @@ static int get_indirect(struct vhost_virtqueue *vq, return ret; } iov_iter_init(&from, READ, vq->indirect, ret, len); - - /* We will use the result as an address to read from, so most - * architectures only need a compiler barrier here. */ - read_barrier_depends(); - count = len / sizeof desc; /* Buffers are chained via a 16 bit next field, so * we can have at most 2^16 of these. */ -- 2.24.0.rc1.363.gb1bccd3e3d-goog
| |