Re: [dpdk-dev] [2/5] vhost: enforce desc flags and content read ordering

[Jason Wang <jasowang@redhat.com>]

On 2018/12/5 下午9:33, Ilya Maximets wrote:
> On 05.12.2018 12:49, Maxime Coquelin wrote:
>> A read barrier is required to ensure that the ordering between
>> descriptor's flags and content reads is enforced.
>>
>> Fixes: 2f3225a7d69b ("vhost: add vector filling support for packed ring")
>> Cc: stable@dpdk.org
>>
>> Reported-by: Jason Wang <jasowang@redhat.com>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>   lib/librte_vhost/virtio_net.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
>> index f11ebb54f..68b72e7a5 100644
>> --- a/lib/librte_vhost/virtio_net.c
>> +++ b/lib/librte_vhost/virtio_net.c
>> @@ -520,6 +520,12 @@ fill_vec_buf_packed(struct virtio_net *dev, struct vhost_virtqueue *vq,
>>   	if (unlikely(!desc_is_avail(&descs[avail_idx], wrap_counter)))
>>   		return -1;
>>   
>> +	/*
>> +	 * The ordering between desc flags and desc
>> +	 * content reads need to be enforced.
>> +	 */
>> +	rte_smp_rmb();
>> +
> Same here. 'desc_is_avail' reads and uses the flags. i.e.
> no way for reordering,
> Writes must be ordered on the virtio side by the write barrier.
> This means that if flags are updated (desc_is_avail() == true)
> than the whole descriptor already updated and the data is written.
> No need to have any read barriers here.


In fact , the sequence might be:


flag = read desc[avail_idx].flag [1]

if(flag is not avail) {

     read desc[avail_idx].id [2]

}


There's no data dependency but control dependency here, so 2 could be 
done before 1 without a rmb.

Thanks


>
>>   	*desc_count = 0;
>>   	*len = 0;
>>   
>>