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

[Ilya Maximets <i.maximets@samsung.com>]

On 06.12.2018 7:24, Jason Wang wrote:
> 
> 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.

OK. Thanks. I agree. Missed that speculative load.

Acked-by: Ilya Maximets <i.maximets@samsung.com>

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