[2/5] vhost: enforce desc flags and content read ordering
Checks
Commit Message
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(+)
Comments
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.
> *desc_count = 0;
> *len = 0;
>
>
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;
>>
>>
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;
>>>
>
>
@@ -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();
+
*desc_count = 0;
*len = 0;