Re: read_barrier_depends() usage in vhost.c

Re: read_barrier_depends() usage in vhost.c

[Will Deacon]

[Bah: I typoed the LKML address, so I've fixed it for this one]

On Thu, Oct 17, 2019 at 12:33:40AM +0100, Will Deacon wrote:
> Hi all,
> 
> In an attempt to remove the remaining traces of [smp_]read_barrier_depends()
> following my previous patches to strengthen READ_ONCE() for Alpha [1], I
> ended up trying to decipher the read_barrier_depends() usage in the vhost
> driver:
> 
> --->8
> 
> // drivers/vhost/vhost.c
> static int get_indirect(struct vhost_virtqueue *vq,
> 			struct iovec iov[], unsigned int iov_size,
> 			unsigned int *out_num, unsigned int *in_num,
> 			struct vhost_log *log, unsigned int *log_num,
> 			struct vring_desc *indirect)
> {
> 	[...]
> 
> 	/* We will use the result as an address to read from, so most
> 	 * architectures only need a compiler barrier here. */
> 	read_barrier_depends();
> 
> --->8
> 
> Unfortunately, although the barrier is commented (hurrah!), it's not
> particularly enlightening about the accesses making up the dependency
> chain, and I don't understand the supposed need for a compiler barrier
> either (read_barrier_depends() doesn't generally provide this).
> 
> Does anybody know which accesses are being ordered here? Usually you'd need
> a READ_ONCE()/rcu_dereference() beginning the chain, but I haven't managed
> to find one...
> 
> Thanks,
> 
> Will
> 
> [1] c2bc66082e10 ("locking/barriers: Add implicit smp_read_barrier_depends() to READ_ONCE()")

Re: read_barrier_depends() usage in vhost.c

[Herbert Xu]

Will Deacon <will@kernel.org> wrote:
>
>> --->8
>> 
>> // drivers/vhost/vhost.c
>> static int get_indirect(struct vhost_virtqueue *vq,
>>                       struct iovec iov[], unsigned int iov_size,
>>                       unsigned int *out_num, unsigned int *in_num,
>>                       struct vhost_log *log, unsigned int *log_num,
>>                       struct vring_desc *indirect)
>> {
>>       [...]
>> 
>>       /* We will use the result as an address to read from, so most
>>        * architectures only need a compiler barrier here. */
>>       read_barrier_depends();
>> 
>> --->8
>> 
>> Unfortunately, although the barrier is commented (hurrah!), it's not
>> particularly enlightening about the accesses making up the dependency
>> chain, and I don't understand the supposed need for a compiler barrier
>> either (read_barrier_depends() doesn't generally provide this).
>> 
>> Does anybody know which accesses are being ordered here? Usually you'd need
>> a READ_ONCE()/rcu_dereference() beginning the chain, but I haven't managed
>> to find one...

I think what it's trying to separate is using indirect->addr as a
base and then reading from that through copy_from_iter.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: read_barrier_depends() usage in vhost.c

[Jason Wang]

On 2019/12/18 下午5:19, Herbert Xu wrote:
> Will Deacon <will@kernel.org> wrote:
>>> --->8
>>>
>>> // drivers/vhost/vhost.c
>>> static int get_indirect(struct vhost_virtqueue *vq,
>>>                        struct iovec iov[], unsigned int iov_size,
>>>                        unsigned int *out_num, unsigned int *in_num,
>>>                        struct vhost_log *log, unsigned int *log_num,
>>>                        struct vring_desc *indirect)
>>> {
>>>        [...]
>>>
>>>        /* We will use the result as an address to read from, so most
>>>         * architectures only need a compiler barrier here. */
>>>        read_barrier_depends();
>>>
>>> --->8
>>>
>>> Unfortunately, although the barrier is commented (hurrah!), it's not
>>> particularly enlightening about the accesses making up the dependency
>>> chain, and I don't understand the supposed need for a compiler barrier
>>> either (read_barrier_depends() doesn't generally provide this).
>>>
>>> Does anybody know which accesses are being ordered here? Usually you'd need
>>> a READ_ONCE()/rcu_dereference() beginning the chain, but I haven't managed
>>> to find one...
> I think what it's trying to separate is using indirect->addr as a
> base and then reading from that through copy_from_iter.
>
> Cheers,


The question is that there's a smp_rmb() before in vhost_get_vq_desc(), 
isn't it sufficient to do this?

Thanks