diff mbox series

vhost: prevent disabled rings to be processed with zero-copy

Message ID 20190227085428.13666-1-maxime.coquelin@redhat.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers show
Series vhost: prevent disabled rings to be processed with zero-copy | expand

Checks

Context Check Description
ci/Performance-Testing fail build patch failure
ci/Intel-compilation fail Compilation issues

Commit Message

Maxime Coquelin Feb. 27, 2019, 8:54 a.m. UTC
The vhost-user spec says that once the vring is disabled, the
client has to stop processing it. But it can happen when
dequeue zero-copy is enabled if outstanding descriptors buffers
are still being processed by an extranl NIC or another guest.

The fix consists in draining the zmbufs list to ensure no more
descriptors buffers are in the wild.

Note that this fix is only working in the case REPLY_ACK
protocol feature is enabled, which is not the case by default
for now (it is only enabled when IOMMU feature is enabled in
the vhost library).

Fixes: b0a985d1f340 ("vhost: add dequeue zero copy")
Cc: stable@dpdk.org

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/vhost_user.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Tiwei Bie Feb. 27, 2019, 10:10 a.m. UTC | #1
On Wed, Feb 27, 2019 at 09:54:28AM +0100, Maxime Coquelin wrote:
> The vhost-user spec says that once the vring is disabled, the
> client has to stop processing it. But it can happen when
> dequeue zero-copy is enabled if outstanding descriptors buffers
> are still being processed by an extranl NIC or another guest.

s/extranl/external/

> 
> The fix consists in draining the zmbufs list to ensure no more
> descriptors buffers are in the wild.
> 
> Note that this fix is only working in the case REPLY_ACK
> protocol feature is enabled, which is not the case by default
> for now (it is only enabled when IOMMU feature is enabled in
> the vhost library).
> 
> Fixes: b0a985d1f340 ("vhost: add dequeue zero copy")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  lib/librte_vhost/vhost_user.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index 36c0c676d..ef947b90c 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -1336,6 +1336,10 @@ vhost_user_set_vring_enable(struct virtio_net **pdev,
>  		"set queue enable: %d to qp idx: %d\n",
>  		enable, index);
>  
> +	/* On disable, rings have to be stopped being processed. */
> +	if (!enable && dev->dequeue_zero_copy)
> +		drain_zmbuf_list(dev->virtqueue[index]);

It might be better to drain the zmbufs list after calling
application's vring_state_changed() callback, so there will
be a chance for applications to take some actions first if
they want.

Overall this looks good to me,

Reviewed-by: Tiwei Bie <tiwei.bie@intel.com>

Thanks!
Tiwei
Maxime Coquelin Feb. 27, 2019, 10:22 a.m. UTC | #2
On 2/27/19 11:10 AM, Tiwei Bie wrote:
> On Wed, Feb 27, 2019 at 09:54:28AM +0100, Maxime Coquelin wrote:
>> The vhost-user spec says that once the vring is disabled, the
>> client has to stop processing it. But it can happen when
>> dequeue zero-copy is enabled if outstanding descriptors buffers
>> are still being processed by an extranl NIC or another guest.
> 
> s/extranl/external/
> 
>>
>> The fix consists in draining the zmbufs list to ensure no more
>> descriptors buffers are in the wild.
>>
>> Note that this fix is only working in the case REPLY_ACK
>> protocol feature is enabled, which is not the case by default
>> for now (it is only enabled when IOMMU feature is enabled in
>> the vhost library).
>>
>> Fixes: b0a985d1f340 ("vhost: add dequeue zero copy")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>   lib/librte_vhost/vhost_user.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
>> index 36c0c676d..ef947b90c 100644
>> --- a/lib/librte_vhost/vhost_user.c
>> +++ b/lib/librte_vhost/vhost_user.c
>> @@ -1336,6 +1336,10 @@ vhost_user_set_vring_enable(struct virtio_net **pdev,
>>   		"set queue enable: %d to qp idx: %d\n",
>>   		enable, index);
>>   
>> +	/* On disable, rings have to be stopped being processed. */
>> +	if (!enable && dev->dequeue_zero_copy)
>> +		drain_zmbuf_list(dev->virtqueue[index]);
> 
> It might be better to drain the zmbufs list after calling
> application's vring_state_changed() callback, so there will
> be a chance for applications to take some actions first if
> they want.

Good point, I'll fix it in v2.

> Overall this looks good to me,
> 
> Reviewed-by: Tiwei Bie <tiwei.bie@intel.com>
> 
> Thanks!
> Tiwei
>
diff mbox series

Patch

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 36c0c676d..ef947b90c 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -1336,6 +1336,10 @@  vhost_user_set_vring_enable(struct virtio_net **pdev,
 		"set queue enable: %d to qp idx: %d\n",
 		enable, index);
 
+	/* On disable, rings have to be stopped being processed. */
+	if (!enable && dev->dequeue_zero_copy)
+		drain_zmbuf_list(dev->virtqueue[index]);
+
 	did = dev->vdpa_dev_id;
 	vdpa_dev = rte_vdpa_get_device(did);
 	if (vdpa_dev && vdpa_dev->ops->set_vring_state)