lkml.org 
[lkml]   [2020]   [Jul]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH v3 07/19] vhost: Remove redundant use of read_barrier_depends() barrier
    Date
    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>
    Acked-by: Paul E. McKenney <paulmck@kernel.org>
    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 d7b8df3edffc..74d135ee7e26 100644
    --- a/drivers/vhost/vhost.c
    +++ b/drivers/vhost/vhost.c
    @@ -2092,11 +2092,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.27.0.383.g050319c2ae-goog
    \
     
     \ /
      Last update: 2020-07-10 18:54    [W:4.163 / U:0.612 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site