diff mbox series

[2/5] vhost: enforce desc flags and content read ordering

Message ID 20181205094957.1938-3-maxime.coquelin@redhat.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series vhost: add missing barriers, remove useless volatiles |

Checks

Context Check Description
ci/Intel-compilation success Compilation OK

Commit Message

Maxime Coquelin Dec. 5, 2018, 9:49 a.m. UTC 
  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

Ilya Maximets Dec. 5, 2018, 1:33 p.m. UTC | #1 
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;
>  
>
Jason Wang Dec. 6, 2018, 4:24 a.m. UTC | #2 
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;
>>   
>>
Ilya Maximets Dec. 6, 2018, 11:34 a.m. UTC | #3 
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;
>>>  
> 
>
diff mbox series

Patch

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();
+
 	*desc_count = 0;
 	*len = 0;