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: sta...@dpdk.org
>>>
>>> Reported-by: Jason Wang <jasow...@redhat.com>
>>> Signed-off-by: Maxime Coquelin <maxime.coque...@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.maxim...@samsung.com>
>
> Thanks
>
>
>>
>>> *desc_count = 0;
>>> *len = 0;
>>>
>
>
