diff mbox series

vhost: fix vring enable with VDPA

Message ID 20230302094808.1519310-1-david.marchand@redhat.com (mailing list archive)
State Accepted, archived
Delegated to: Maxime Coquelin
Headers
Series vhost: fix vring enable with VDPA |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/intel-Functional success Functional PASS
ci/github-robot: build success github build: passed
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-intel-Performance fail Performance Testing issues
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS

Commit Message

David Marchand March 2, 2023, 9:48 a.m. UTC 
  For VDPA devices, vq are not locked once the device has been configured.
But we need to hold the vq lock to check if vhost async is enabled.

Combining VDPA and vhost async does not seem to make much sense, so
prevent this combination, and only assert the lock is taken when VDPA is
not configured on this vq.

Bugzilla ID: 1169
Fixes: 4b02c2673757 ("vhost: annotate async accesses")

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/vhost/vhost.c      | 4 ++--
 lib/vhost/vhost_user.c | 8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

Comments

Maxime Coquelin March 2, 2023, 10:42 a.m. UTC | #1 
On 3/2/23 10:48, David Marchand wrote:
> For VDPA devices, vq are not locked once the device has been configured.
> But we need to hold the vq lock to check if vhost async is enabled.
> 
> Combining VDPA and vhost async does not seem to make much sense, so
> prevent this combination, and only assert the lock is taken when VDPA is
> not configured on this vq.
> 
> Bugzilla ID: 1169
> Fixes: 4b02c2673757 ("vhost: annotate async accesses")
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>   lib/vhost/vhost.c      | 4 ++--
>   lib/vhost/vhost_user.c | 8 ++++----
>   2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> index c36dc06a0a..6b73c03224 100644
> --- a/lib/vhost/vhost.c
> +++ b/lib/vhost/vhost.c
> @@ -1754,7 +1754,7 @@ rte_vhost_async_channel_register(int vid, uint16_t queue_id)
>   
>   	vq = dev->virtqueue[queue_id];
>   
> -	if (unlikely(vq == NULL || !dev->async_copy))
> +	if (unlikely(vq == NULL || !dev->async_copy || dev->vdpa_dev))
>   		return -1;
>   
>   	rte_spinlock_lock(&vq->access_lock);
> @@ -1778,7 +1778,7 @@ rte_vhost_async_channel_register_thread_unsafe(int vid, uint16_t queue_id)
>   
>   	vq = dev->virtqueue[queue_id];
>   
> -	if (unlikely(vq == NULL || !dev->async_copy))
> +	if (unlikely(vq == NULL || !dev->async_copy || dev->vdpa_dev))
>   		return -1;
>   
>   	vq_assert_lock(dev, vq);
> diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
> index d702d082dd..daf3458985 100644
> --- a/lib/vhost/vhost_user.c
> +++ b/lib/vhost/vhost_user.c
> @@ -2177,10 +2177,10 @@ vhost_user_set_vring_enable(struct virtio_net **pdev,
>   		enable, index);
>   
>   	vq = dev->virtqueue[index];
> -	/* vhost_user_lock_all_queue_pairs locked all qps */
> -	vq_assert_lock(dev, vq);
> -	if (enable && vq->async) {
> -		if (vq->async->pkts_inflight_n) {
> +	if (!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED)) {
> +		/* vhost_user_lock_all_queue_pairs locked all qps */
> +		vq_assert_lock(dev, vq);
> +		if (enable && vq->async && vq->async->pkts_inflight_n) {
>   			VHOST_LOG_CONFIG(dev->ifname, ERR,
>   				"failed to enable vring. Inflight packets must be completed first\n");
>   			return RTE_VHOST_MSG_RESULT_ERR;

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime
Chenbo Xia March 3, 2023, 5:45 a.m. UTC | #2 
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Thursday, March 2, 2023 5:48 PM
> To: dev@dpdk.org
> Cc: ssimma@nvidia.com; yajunw@nvidia.com; Maxime Coquelin
> <maxime.coquelin@redhat.com>; Xia, Chenbo <chenbo.xia@intel.com>; Morten
> Brørup <mb@smartsharesystems.com>
> Subject: [PATCH] vhost: fix vring enable with VDPA
> 
> For VDPA devices, vq are not locked once the device has been configured.
> But we need to hold the vq lock to check if vhost async is enabled.
> 
> Combining VDPA and vhost async does not seem to make much sense, so
> prevent this combination, and only assert the lock is taken when VDPA is
> not configured on this vq.
> 
> Bugzilla ID: 1169
> Fixes: 4b02c2673757 ("vhost: annotate async accesses")
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  lib/vhost/vhost.c      | 4 ++--
>  lib/vhost/vhost_user.c | 8 ++++----
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> index c36dc06a0a..6b73c03224 100644
> --- a/lib/vhost/vhost.c
> +++ b/lib/vhost/vhost.c
> @@ -1754,7 +1754,7 @@ rte_vhost_async_channel_register(int vid, uint16_t
> queue_id)
> 
>  	vq = dev->virtqueue[queue_id];
> 
> -	if (unlikely(vq == NULL || !dev->async_copy))
> +	if (unlikely(vq == NULL || !dev->async_copy || dev->vdpa_dev))
>  		return -1;
> 
>  	rte_spinlock_lock(&vq->access_lock);
> @@ -1778,7 +1778,7 @@ rte_vhost_async_channel_register_thread_unsafe(int
> vid, uint16_t queue_id)
> 
>  	vq = dev->virtqueue[queue_id];
> 
> -	if (unlikely(vq == NULL || !dev->async_copy))
> +	if (unlikely(vq == NULL || !dev->async_copy || dev->vdpa_dev))
>  		return -1;
> 
>  	vq_assert_lock(dev, vq);
> diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
> index d702d082dd..daf3458985 100644
> --- a/lib/vhost/vhost_user.c
> +++ b/lib/vhost/vhost_user.c
> @@ -2177,10 +2177,10 @@ vhost_user_set_vring_enable(struct virtio_net
> **pdev,
>  		enable, index);
> 
>  	vq = dev->virtqueue[index];
> -	/* vhost_user_lock_all_queue_pairs locked all qps */
> -	vq_assert_lock(dev, vq);
> -	if (enable && vq->async) {
> -		if (vq->async->pkts_inflight_n) {
> +	if (!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED)) {
> +		/* vhost_user_lock_all_queue_pairs locked all qps */
> +		vq_assert_lock(dev, vq);
> +		if (enable && vq->async && vq->async->pkts_inflight_n) {
>  			VHOST_LOG_CONFIG(dev->ifname, ERR,
>  				"failed to enable vring. Inflight packets must be
> completed first\n");
>  			return RTE_VHOST_MSG_RESULT_ERR;
> --
> 2.39.2

Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
Ali Alnubani March 3, 2023, 12:54 p.m. UTC | #3 
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Thursday, March 2, 2023 11:48 AM
> To: dev@dpdk.org
> Cc: Semion Simma <ssimma@nvidia.com>; Yajun Wu <yajunw@nvidia.com>;
> Maxime Coquelin <maxime.coquelin@redhat.com>; Chenbo Xia
> <chenbo.xia@intel.com>; Morten Brørup <mb@smartsharesystems.com>
> Subject: [PATCH] vhost: fix vring enable with VDPA
> 
> For VDPA devices, vq are not locked once the device has been configured.
> But we need to hold the vq lock to check if vhost async is enabled.
> 
> Combining VDPA and vhost async does not seem to make much sense, so
> prevent this combination, and only assert the lock is taken when VDPA is
> not configured on this vq.
> 
> Bugzilla ID: 1169
> Fixes: 4b02c2673757 ("vhost: annotate async accesses")
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---

Verified it resolves the issue, thanks David.

Tested-by: Ali Alnubani <alialnu@nvidia.com>
Maxime Coquelin March 6, 2023, 2:21 p.m. UTC | #4 
On 3/2/23 10:48, David Marchand wrote:
> For VDPA devices, vq are not locked once the device has been configured.
> But we need to hold the vq lock to check if vhost async is enabled.
> 
> Combining VDPA and vhost async does not seem to make much sense, so
> prevent this combination, and only assert the lock is taken when VDPA is
> not configured on this vq.
> 
> Bugzilla ID: 1169
> Fixes: 4b02c2673757 ("vhost: annotate async accesses")
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>   lib/vhost/vhost.c      | 4 ++--
>   lib/vhost/vhost_user.c | 8 ++++----
>   2 files changed, 6 insertions(+), 6 deletions(-)
> 

Applied to dpdk-next-virtio/main.

Thanks,
Maxime
diff mbox series

Patch

diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
index c36dc06a0a..6b73c03224 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -1754,7 +1754,7 @@  rte_vhost_async_channel_register(int vid, uint16_t queue_id)
 
 	vq = dev->virtqueue[queue_id];
 
-	if (unlikely(vq == NULL || !dev->async_copy))
+	if (unlikely(vq == NULL || !dev->async_copy || dev->vdpa_dev))
 		return -1;
 
 	rte_spinlock_lock(&vq->access_lock);
@@ -1778,7 +1778,7 @@  rte_vhost_async_channel_register_thread_unsafe(int vid, uint16_t queue_id)
 
 	vq = dev->virtqueue[queue_id];
 
-	if (unlikely(vq == NULL || !dev->async_copy))
+	if (unlikely(vq == NULL || !dev->async_copy || dev->vdpa_dev))
 		return -1;
 
 	vq_assert_lock(dev, vq);
diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
index d702d082dd..daf3458985 100644
--- a/lib/vhost/vhost_user.c
+++ b/lib/vhost/vhost_user.c
@@ -2177,10 +2177,10 @@  vhost_user_set_vring_enable(struct virtio_net **pdev,
 		enable, index);
 
 	vq = dev->virtqueue[index];
-	/* vhost_user_lock_all_queue_pairs locked all qps */
-	vq_assert_lock(dev, vq);
-	if (enable && vq->async) {
-		if (vq->async->pkts_inflight_n) {
+	if (!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED)) {
+		/* vhost_user_lock_all_queue_pairs locked all qps */
+		vq_assert_lock(dev, vq);
+		if (enable && vq->async && vq->async->pkts_inflight_n) {
 			VHOST_LOG_CONFIG(dev->ifname, ERR,
 				"failed to enable vring. Inflight packets must be completed first\n");
 			return RTE_VHOST_MSG_RESULT_ERR;