[dpdk-dev] [PATCH 0/2] provide thread unsafe async registration functions

[dpdk-dev] [PATCH 0/2] provide thread unsafe async registration functions

[Jiayu Hu]

Lock protection is needed during the vhost notifies the application of
device readiness, so the first patch is to add lock protection. After
performing locking, existed async vhost registration functions will cause
deadlock, as they acquire lock too. So the second patch is to provide
unsafe registration functions to support calling within vhost callback
functions.

Jiayu Hu (2):
  vhost: fix lock on device readiness notification
  vhost: add thread unsafe async registration functions

 doc/guides/prog_guide/vhost_lib.rst |  12 +++
 lib/vhost/rte_vhost_async.h         |  42 ++++++++++
 lib/vhost/version.map               |   4 +
 lib/vhost/vhost.c                   | 161 +++++++++++++++++++++++++++---------
 lib/vhost/vhost_user.c              |   5 +-
 5 files changed, 180 insertions(+), 44 deletions(-)

-- 
2.7.4

[dpdk-dev] [PATCH 1/2] vhost: fix lock on device readiness notification

[Jiayu Hu]

The vhost notifies the application of device readiness via
vhost_user_notify_queue_state(), but calling this function
is not protected by the lock. This patch is to make this
function call lock protected.

Fixes: d0fcc38f5fa4 ("vhost: improve device readiness notifications")
Cc: stable@dpdk.org
Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
---
 lib/vhost/vhost_user.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
index 8f0eba6..dabce26 100644
--- a/lib/vhost/vhost_user.c
+++ b/lib/vhost/vhost_user.c
@@ -2915,9 +2915,6 @@ vhost_user_msg_handler(int vid, int fd)
 		}
 	}
 
-	if (unlock_required)
-		vhost_user_unlock_all_queue_pairs(dev);
-
 	/* If message was not handled at this stage, treat it as an error */
 	if (!handled) {
 		VHOST_LOG_CONFIG(ERR,
@@ -2952,6 +2949,8 @@ vhost_user_msg_handler(int vid, int fd)
 		}
 	}
 
+	if (unlock_required)
+		vhost_user_unlock_all_queue_pairs(dev);
 
 	if (!virtio_is_ready(dev))
 		goto out;
-- 
2.7.4

[dpdk-dev] [PATCH 2/2] vhost: add thread unsafe async registration functions

[Jiayu Hu]

This patch is to add thread unsafe version for async register and
unregister functions.

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
---
 doc/guides/prog_guide/vhost_lib.rst |  12 +++
 lib/vhost/rte_vhost_async.h         |  42 ++++++++++
 lib/vhost/version.map               |   4 +
 lib/vhost/vhost.c                   | 161 +++++++++++++++++++++++++++---------
 4 files changed, 178 insertions(+), 41 deletions(-)

diff --git a/doc/guides/prog_guide/vhost_lib.rst b/doc/guides/prog_guide/vhost_lib.rst
index d18fb98..6b2745f 100644
--- a/doc/guides/prog_guide/vhost_lib.rst
+++ b/doc/guides/prog_guide/vhost_lib.rst
@@ -253,6 +253,12 @@ The following is an overview of some key Vhost API functions:
     vhost invokes this function to get the copy data completed by async
     devices.
 
+* ``rte_vhost_async_channel_register_thread_unsafe(vid, queue_id, features, ops)``
+  Register a vhost queue with async copy device channel without
+  performing any locking.
+
+  This function is only safe to call from within vhost callback functions.
+
 * ``rte_vhost_async_channel_unregister(vid, queue_id)``
 
   Unregister the async copy device channel from a vhost queue.
@@ -265,6 +271,12 @@ The following is an overview of some key Vhost API functions:
   devices for all vhost queues in destroy_device(), when a
   virtio device is paused or shut down.
 
+* ``rte_vhost_async_channel_unregister_thread_unsafe(vid, queue_id)``
+  Unregister the async copy device channel from a vhost queue without
+  performing any locking.
+
+  This function is only safe to call from within vhost callback functions.
+
 * ``rte_vhost_submit_enqueue_burst(vid, queue_id, pkts, count, comp_pkts, comp_count)``
 
   Submit an enqueue request to transmit ``count`` packets from host to guest
diff --git a/lib/vhost/rte_vhost_async.h b/lib/vhost/rte_vhost_async.h
index 6faa31f..d054b5f 100644
--- a/lib/vhost/rte_vhost_async.h
+++ b/lib/vhost/rte_vhost_async.h
@@ -143,6 +143,48 @@ __rte_experimental
 int rte_vhost_async_channel_unregister(int vid, uint16_t queue_id);
 
 /**
+ * register an async channel for vhost without performing any locking
+ *
+ * @note This function does not perform any locking, and is only safe to call
+ *       from within vhost callback functions.
+ *
+ * @param vid
+ *  vhost device id async channel to be attached to
+ * @param queue_id
+ *  vhost queue id async channel to be attached to
+ * @param features
+ *  DMA channel feature bit
+ *    b0       : DMA supports inorder data transfer
+ *    b1  - b15: reserved
+ *    b16 - b27: Packet length threshold for DMA transfer
+ *    b28 - b31: reserved
+ * @param ops
+ *  DMA operation callbacks
+ * @return
+ *  0 on success, -1 on failures
+ */
+__rte_experimental
+int rte_vhost_async_channel_register_thread_unsafe(int vid, uint16_t queue_id,
+	uint32_t features, struct rte_vhost_async_channel_ops *ops);
+
+/**
+ * unregister a dma channel for vhost without performing any lock
+ *
+ * @note This function does not perform any locking, and is only safe to call
+ *       from within vhost callback functions.
+ *
+ * @param vid
+ *  vhost device id DMA channel to be detached
+ * @param queue_id
+ *  vhost queue id DMA channel to be detached
+ * @return
+ *  0 on success, -1 on failures
+ */
+__rte_experimental
+int rte_vhost_async_channel_unregister_thread_unsafe(int vid,
+		uint16_t queue_id);
+
+/**
  * This function submits enqueue data to async engine. Successfully
  * enqueued packets can be transfer completed or being occupied by DMA
  * engines, when this API returns. Transfer completed packets are returned
diff --git a/lib/vhost/version.map b/lib/vhost/version.map
index 9103a23..2363db8 100644
--- a/lib/vhost/version.map
+++ b/lib/vhost/version.map
@@ -79,4 +79,8 @@ EXPERIMENTAL {
 
 	# added in 21.05
 	rte_vhost_get_negotiated_protocol_features;
+
+	# added in 21.08
+	rte_vhost_async_channel_register_thread_unsafe;
+	rte_vhost_async_channel_unregister_thread_unsafe;
 };
diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
index c96f633..025e150 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -1609,46 +1609,20 @@ int rte_vhost_extern_callback_register(int vid,
 	return 0;
 }
 
-int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
-					uint32_t features,
-					struct rte_vhost_async_channel_ops *ops)
+static __rte_always_inline int
+async_channel_register(int vid, uint16_t queue_id,
+		struct rte_vhost_async_features f,
+		struct rte_vhost_async_channel_ops *ops)
 {
-	struct vhost_virtqueue *vq;
 	struct virtio_net *dev = get_device(vid);
-	struct rte_vhost_async_features f;
+	struct vhost_virtqueue *vq = dev->virtqueue[queue_id];
 	int node;
 
-	if (dev == NULL || ops == NULL)
-		return -1;
-
-	f.intval = features;
-
-	if (queue_id >= VHOST_MAX_VRING)
-		return -1;
-
-	vq = dev->virtqueue[queue_id];
-
-	if (unlikely(vq == NULL || !dev->async_copy))
-		return -1;
-
-	if (unlikely(!f.async_inorder)) {
-		VHOST_LOG_CONFIG(ERR,
-			"async copy is not supported on non-inorder mode "
-			"(vid %d, qid: %d)\n", vid, queue_id);
-		return -1;
-	}
-
-	if (unlikely(ops->check_completed_copies == NULL ||
-		ops->transfer_data == NULL))
-		return -1;
-
-	rte_spinlock_lock(&vq->access_lock);
-
 	if (unlikely(vq->async_registered)) {
 		VHOST_LOG_CONFIG(ERR,
 			"async register failed: channel already registered "
 			"(vid %d, qid: %d)\n", vid, queue_id);
-		goto reg_out;
+		return -1;
 	}
 
 #ifdef RTE_LIBRTE_VHOST_NUMA
@@ -1670,7 +1644,7 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 		VHOST_LOG_CONFIG(ERR,
 			"async register failed: cannot allocate memory for async_pkts_info "
 			"(vid %d, qid: %d)\n", vid, queue_id);
-		goto reg_out;
+		return -1;
 	}
 
 	vq->it_pool = rte_malloc_socket(NULL,
@@ -1681,7 +1655,7 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 		VHOST_LOG_CONFIG(ERR,
 			"async register failed: cannot allocate memory for it_pool "
 			"(vid %d, qid: %d)\n", vid, queue_id);
-		goto reg_out;
+		return -1;
 	}
 
 	vq->vec_pool = rte_malloc_socket(NULL,
@@ -1692,7 +1666,7 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 		VHOST_LOG_CONFIG(ERR,
 			"async register failed: cannot allocate memory for vec_pool "
 			"(vid %d, qid: %d)\n", vid, queue_id);
-		goto reg_out;
+		return -1;
 	}
 
 	if (vq_is_packed(dev)) {
@@ -1704,7 +1678,7 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 			VHOST_LOG_CONFIG(ERR,
 				"async register failed: cannot allocate memory for async buffers "
 				"(vid %d, qid: %d)\n", vid, queue_id);
-			goto reg_out;
+			return -1;
 		}
 	} else {
 		vq->async_descs_split = rte_malloc_socket(NULL,
@@ -1715,22 +1689,92 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 			VHOST_LOG_CONFIG(ERR,
 				"async register failed: cannot allocate memory for async descs "
 				"(vid %d, qid: %d)\n", vid, queue_id);
-			goto reg_out;
+			return -1;
 		}
 	}
 
 	vq->async_ops.check_completed_copies = ops->check_completed_copies;
 	vq->async_ops.transfer_data = ops->transfer_data;
-
 	vq->async_inorder = f.async_inorder;
 	vq->async_threshold = f.async_threshold;
-
 	vq->async_registered = true;
 
-reg_out:
+	return 0;
+}
+
+int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
+					uint32_t features,
+					struct rte_vhost_async_channel_ops *ops)
+{
+	struct vhost_virtqueue *vq;
+	struct virtio_net *dev = get_device(vid);
+	struct rte_vhost_async_features f;
+	int ret;
+
+	if (dev == NULL || ops == NULL)
+		return -1;
+
+	f.intval = features;
+
+	if (queue_id >= VHOST_MAX_VRING)
+		return -1;
+
+	vq = dev->virtqueue[queue_id];
+
+	if (unlikely(vq == NULL || !dev->async_copy))
+		return -1;
+
+	if (unlikely(!f.async_inorder)) {
+		VHOST_LOG_CONFIG(ERR,
+			"async copy is not supported on non-inorder mode "
+			"(vid %d, qid: %d)\n", vid, queue_id);
+		return -1;
+	}
+
+	if (unlikely(ops->check_completed_copies == NULL ||
+		ops->transfer_data == NULL))
+		return -1;
+
+	rte_spinlock_lock(&vq->access_lock);
+	ret = async_channel_register(vid, queue_id, f, ops);
 	rte_spinlock_unlock(&vq->access_lock);
 
-	return 0;
+	return ret;
+}
+
+int rte_vhost_async_channel_register_thread_unsafe(int vid, uint16_t queue_id,
+					uint32_t features,
+					struct rte_vhost_async_channel_ops *ops)
+{
+	struct vhost_virtqueue *vq;
+	struct virtio_net *dev = get_device(vid);
+	struct rte_vhost_async_features f;
+
+	if (dev == NULL || ops == NULL)
+		return -1;
+
+	f.intval = features;
+
+	if (queue_id >= VHOST_MAX_VRING)
+		return -1;
+
+	vq = dev->virtqueue[queue_id];
+
+	if (unlikely(vq == NULL || !dev->async_copy))
+		return -1;
+
+	if (unlikely(!f.async_inorder)) {
+		VHOST_LOG_CONFIG(ERR,
+			"async copy is not supported on non-inorder mode "
+			"(vid %d, qid: %d)\n", vid, queue_id);
+		return -1;
+	}
+
+	if (unlikely(ops->check_completed_copies == NULL ||
+		ops->transfer_data == NULL))
+		return -1;
+
+	return async_channel_register(vid, queue_id, f, ops);
 }
 
 int rte_vhost_async_channel_unregister(int vid, uint16_t queue_id)
@@ -1780,5 +1824,40 @@ int rte_vhost_async_channel_unregister(int vid, uint16_t queue_id)
 	return ret;
 }
 
+int rte_vhost_async_channel_unregister_thread_unsafe(int vid,
+		uint16_t queue_id)
+{
+	struct vhost_virtqueue *vq;
+	struct virtio_net *dev = get_device(vid);
+
+	if (dev == NULL)
+		return -1;
+
+	if (queue_id >= VHOST_MAX_VRING)
+		return -1;
+
+	vq = dev->virtqueue[queue_id];
+
+	if (vq == NULL)
+		return -1;
+
+	if (!vq->async_registered)
+		return 0;
+
+	if (vq->async_pkts_inflight_n) {
+		VHOST_LOG_CONFIG(ERR, "Failed to unregister async channel. "
+			"async inflight packets must be completed before unregistration.\n");
+		return -1;
+	}
+
+	vhost_free_async_mem(vq);
+
+	vq->async_ops.transfer_data = NULL;
+	vq->async_ops.check_completed_copies = NULL;
+	vq->async_registered = false;
+
+	return 0;
+}
+
 RTE_LOG_REGISTER_SUFFIX(vhost_config_log_level, config, INFO);
 RTE_LOG_REGISTER_SUFFIX(vhost_data_log_level, data, WARNING);
-- 
2.7.4

Re: [dpdk-dev] [PATCH 0/2] provide thread unsafe async registration functions

[Maxime Coquelin]

On 5/28/21 10:11 AM, Jiayu Hu wrote:
> Lock protection is needed during the vhost notifies the application of
> device readiness, so the first patch is to add lock protection. After
> performing locking, existed async vhost registration functions will cause
> deadlock, as they acquire lock too. So the second patch is to provide
> unsafe registration functions to support calling within vhost callback
> functions.
> 
> Jiayu Hu (2):
>   vhost: fix lock on device readiness notification
>   vhost: add thread unsafe async registration functions
> 
>  doc/guides/prog_guide/vhost_lib.rst |  12 +++
>  lib/vhost/rte_vhost_async.h         |  42 ++++++++++
>  lib/vhost/version.map               |   4 +
>  lib/vhost/vhost.c                   | 161 +++++++++++++++++++++++++++---------
>  lib/vhost/vhost_user.c              |   5 +-
>  5 files changed, 180 insertions(+), 44 deletions(-)
> 

Re: [dpdk-dev] [PATCH 0/2] provide thread unsafe async registration functions

[Maxime Coquelin]

Sorry, for previous blank reply.

On 5/28/21 10:11 AM, Jiayu Hu wrote:
> Lock protection is needed during the vhost notifies the application of
> device readiness, so the first patch is to add lock protection. After
> performing locking, existed async vhost registration functions will cause
> deadlock, as they acquire lock too. So the second patch is to provide
> unsafe registration functions to support calling within vhost callback
> functions.

I agree the callback should be always protected, and in that case having
a new thread-unsafe API makes sense for async registration.

Regarding backport, I'm not sure what we should do.

Backporting new API is a no-go, but with only backporting patch 1 async
feature will be always broken on 20.11 LTS, right?

What do you think?

Thanks,
Maxime

> Jiayu Hu (2):
>   vhost: fix lock on device readiness notification
>   vhost: add thread unsafe async registration functions
> 
>  doc/guides/prog_guide/vhost_lib.rst |  12 +++
>  lib/vhost/rte_vhost_async.h         |  42 ++++++++++
>  lib/vhost/version.map               |   4 +
>  lib/vhost/vhost.c                   | 161 +++++++++++++++++++++++++++---------
>  lib/vhost/vhost_user.c              |   5 +-
>  5 files changed, 180 insertions(+), 44 deletions(-)
> 

Re: [dpdk-dev] [PATCH 0/2] provide thread unsafe async registration functions

[Hu, Jiayu]

Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin <mcoqueli@redhat.com>
> Sent: Friday, June 4, 2021 3:25 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>;
> Wang, Yinan <yinan.wang@intel.com>
> Subject: Re: [PATCH 0/2] provide thread unsafe async registration functions
> 
> Sorry, for previous blank reply.
> 
> On 5/28/21 10:11 AM, Jiayu Hu wrote:
> > Lock protection is needed during the vhost notifies the application of
> > device readiness, so the first patch is to add lock protection. After
> > performing locking, existed async vhost registration functions will cause
> > deadlock, as they acquire lock too. So the second patch is to provide
> > unsafe registration functions to support calling within vhost callback
> > functions.
> 
> I agree the callback should be always protected, and in that case having
> a new thread-unsafe API makes sense for async registration.
> 
> Regarding backport, I'm not sure what we should do.
> 
> Backporting new API is a no-go, but with only backporting patch 1 async
> feature will be always broken on 20.11 LTS, right?

Yes, if only backporting this fix patch to 20.11 LTS, it may break apps who call
async registration functions inside vhost callbacks.

How about making this patch not a fix, but a new feature?

Thanks,
Jiayu
> 
> What do you think?
> 
> Thanks,
> Maxime
> 
> > Jiayu Hu (2):
> >   vhost: fix lock on device readiness notification
> >   vhost: add thread unsafe async registration functions
> >
> >  doc/guides/prog_guide/vhost_lib.rst |  12 +++
> >  lib/vhost/rte_vhost_async.h         |  42 ++++++++++
> >  lib/vhost/version.map               |   4 +
> >  lib/vhost/vhost.c                   | 161 +++++++++++++++++++++++++++---------
> >  lib/vhost/vhost_user.c              |   5 +-
> >  5 files changed, 180 insertions(+), 44 deletions(-)
> >

Re: [dpdk-dev] [PATCH 0/2] provide thread unsafe async registration functions

[Maxime Coquelin]

Hi Jiayu,

On 6/7/21 10:07 AM, Hu, Jiayu wrote:
> Hi Maxime,
> 
>> -----Original Message-----
>> From: Maxime Coquelin <mcoqueli@redhat.com>
>> Sent: Friday, June 4, 2021 3:25 PM
>> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
>> Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>;
>> Wang, Yinan <yinan.wang@intel.com>
>> Subject: Re: [PATCH 0/2] provide thread unsafe async registration functions
>>
>> Sorry, for previous blank reply.
>>
>> On 5/28/21 10:11 AM, Jiayu Hu wrote:
>>> Lock protection is needed during the vhost notifies the application of
>>> device readiness, so the first patch is to add lock protection. After
>>> performing locking, existed async vhost registration functions will cause
>>> deadlock, as they acquire lock too. So the second patch is to provide
>>> unsafe registration functions to support calling within vhost callback
>>> functions.
>>
>> I agree the callback should be always protected, and in that case having
>> a new thread-unsafe API makes sense for async registration.
>>
>> Regarding backport, I'm not sure what we should do.
>>
>> Backporting new API is a no-go, but with only backporting patch 1 async
>> feature will be always broken on 20.11 LTS, right?
> 
> Yes, if only backporting this fix patch to 20.11 LTS, it may break apps who call
> async registration functions inside vhost callbacks.
> 
> How about making this patch not a fix, but a new feature?

Async will be still broken in v20.11 in this case.
Maybe the better thing would be to remove async support in v20.11, as
its support was quite limited in that release anyway. Does that make
sense?

Thanks,
Maxime

> Thanks,
> Jiayu
>>
>> What do you think?
>>
>> Thanks,
>> Maxime
>>
>>> Jiayu Hu (2):
>>>   vhost: fix lock on device readiness notification
>>>   vhost: add thread unsafe async registration functions
>>>
>>>  doc/guides/prog_guide/vhost_lib.rst |  12 +++
>>>  lib/vhost/rte_vhost_async.h         |  42 ++++++++++
>>>  lib/vhost/version.map               |   4 +
>>>  lib/vhost/vhost.c                   | 161 +++++++++++++++++++++++++++---------
>>>  lib/vhost/vhost_user.c              |   5 +-
>>>  5 files changed, 180 insertions(+), 44 deletions(-)
>>>
> 

Re: [dpdk-dev] [PATCH 0/2] provide thread unsafe async registration functions

[Hu, Jiayu]

> -----Original Message-----
> From: Maxime Coquelin <mcoqueli@redhat.com>
> Sent: Monday, June 7, 2021 9:20 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>;
> Wang, Yinan <yinan.wang@intel.com>
> Subject: Re: [PATCH 0/2] provide thread unsafe async registration functions
> 
> Hi Jiayu,
> 
> On 6/7/21 10:07 AM, Hu, Jiayu wrote:
> > Hi Maxime,
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin <mcoqueli@redhat.com>
> >> Sent: Friday, June 4, 2021 3:25 PM
> >> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
> >> Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>;
> >> Wang, Yinan <yinan.wang@intel.com>
> >> Subject: Re: [PATCH 0/2] provide thread unsafe async registration
> functions
> >>
> >> Sorry, for previous blank reply.
> >>
> >> On 5/28/21 10:11 AM, Jiayu Hu wrote:
> >>> Lock protection is needed during the vhost notifies the application of
> >>> device readiness, so the first patch is to add lock protection. After
> >>> performing locking, existed async vhost registration functions will cause
> >>> deadlock, as they acquire lock too. So the second patch is to provide
> >>> unsafe registration functions to support calling within vhost callback
> >>> functions.
> >>
> >> I agree the callback should be always protected, and in that case having
> >> a new thread-unsafe API makes sense for async registration.
> >>
> >> Regarding backport, I'm not sure what we should do.
> >>
> >> Backporting new API is a no-go, but with only backporting patch 1 async
> >> feature will be always broken on 20.11 LTS, right?
> >
> > Yes, if only backporting this fix patch to 20.11 LTS, it may break apps who
> call
> > async registration functions inside vhost callbacks.
> >
> > How about making this patch not a fix, but a new feature?
> 
> Async will be still broken in v20.11 in this case.
> Maybe the better thing would be to remove async support in v20.11, as
> its support was quite limited in that release anyway. Does that make
> sense?

OK, that makes sense to me.

Thanks,
Jiayu
> 
> Thanks,
> Maxime
> 
> > Thanks,
> > Jiayu
> >>
> >> What do you think?
> >>
> >> Thanks,
> >> Maxime
> >>
> >>> Jiayu Hu (2):
> >>>   vhost: fix lock on device readiness notification
> >>>   vhost: add thread unsafe async registration functions
> >>>
> >>>  doc/guides/prog_guide/vhost_lib.rst |  12 +++
> >>>  lib/vhost/rte_vhost_async.h         |  42 ++++++++++
> >>>  lib/vhost/version.map               |   4 +
> >>>  lib/vhost/vhost.c                   | 161 +++++++++++++++++++++++++++--------
> -
> >>>  lib/vhost/vhost_user.c              |   5 +-
> >>>  5 files changed, 180 insertions(+), 44 deletions(-)
> >>>
> >

Re: [dpdk-dev] [PATCH 0/2] provide thread unsafe async registration functions

[Hu, Jiayu]

Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin <mcoqueli@redhat.com>
> Sent: Monday, June 7, 2021 9:20 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>;
> Wang, Yinan <yinan.wang@intel.com>
> Subject: Re: [PATCH 0/2] provide thread unsafe async registration functions
> 
> Hi Jiayu,
> 
> On 6/7/21 10:07 AM, Hu, Jiayu wrote:
> > Hi Maxime,
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin <mcoqueli@redhat.com>
> >> Sent: Friday, June 4, 2021 3:25 PM
> >> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
> >> Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>;
> >> Wang, Yinan <yinan.wang@intel.com>
> >> Subject: Re: [PATCH 0/2] provide thread unsafe async registration
> >> functions
> >>
> >> Sorry, for previous blank reply.
> >>
> >> On 5/28/21 10:11 AM, Jiayu Hu wrote:
> >>> Lock protection is needed during the vhost notifies the application
> >>> of device readiness, so the first patch is to add lock protection.
> >>> After performing locking, existed async vhost registration functions
> >>> will cause deadlock, as they acquire lock too. So the second patch
> >>> is to provide unsafe registration functions to support calling
> >>> within vhost callback functions.
> >>
> >> I agree the callback should be always protected, and in that case
> >> having a new thread-unsafe API makes sense for async registration.
> >>
> >> Regarding backport, I'm not sure what we should do.
> >>
> >> Backporting new API is a no-go, but with only backporting patch 1
> >> async feature will be always broken on 20.11 LTS, right?
> >
> > Yes, if only backporting this fix patch to 20.11 LTS, it may break
> > apps who call async registration functions inside vhost callbacks.
> >
> > How about making this patch not a fix, but a new feature?
> 
> Async will be still broken in v20.11 in this case.
> Maybe the better thing would be to remove async support in v20.11, as its
> support was quite limited in that release anyway. Does that make sense?

The code of supporting async vhost are beyond 1000 lines. I am afraid that
removing such more code in 20.11 LTS may get objected. Can we note async
register/unregister only work in new_/destroy_device, and using them in
other vhost callback functions will cause deadlock in 20.11 LTS instead?
Does it make sense to you?

 Thanks,
Jiayu
> 
> Thanks,
> Maxime
> 
> > Thanks,
> > Jiayu
> >>
> >> What do you think?
> >>
> >> Thanks,
> >> Maxime

Re: [dpdk-dev] [PATCH 0/2] provide thread unsafe async registration functions

[Maxime Coquelin]

Hi Jiayu,

On 6/29/21 7:36 AM, Hu, Jiayu wrote:
> Hi Maxime,
> 
>> -----Original Message-----
>> From: Maxime Coquelin <mcoqueli@redhat.com>
>> Sent: Monday, June 7, 2021 9:20 PM
>> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
>> Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>;
>> Wang, Yinan <yinan.wang@intel.com>
>> Subject: Re: [PATCH 0/2] provide thread unsafe async registration functions
>>
>> Hi Jiayu,
>>
>> On 6/7/21 10:07 AM, Hu, Jiayu wrote:
>>> Hi Maxime,
>>>
>>>> -----Original Message-----
>>>> From: Maxime Coquelin <mcoqueli@redhat.com>
>>>> Sent: Friday, June 4, 2021 3:25 PM
>>>> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
>>>> Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>;
>>>> Wang, Yinan <yinan.wang@intel.com>
>>>> Subject: Re: [PATCH 0/2] provide thread unsafe async registration
>>>> functions
>>>>
>>>> Sorry, for previous blank reply.
>>>>
>>>> On 5/28/21 10:11 AM, Jiayu Hu wrote:
>>>>> Lock protection is needed during the vhost notifies the application
>>>>> of device readiness, so the first patch is to add lock protection.
>>>>> After performing locking, existed async vhost registration functions
>>>>> will cause deadlock, as they acquire lock too. So the second patch
>>>>> is to provide unsafe registration functions to support calling
>>>>> within vhost callback functions.
>>>>
>>>> I agree the callback should be always protected, and in that case
>>>> having a new thread-unsafe API makes sense for async registration.
>>>>
>>>> Regarding backport, I'm not sure what we should do.
>>>>
>>>> Backporting new API is a no-go, but with only backporting patch 1
>>>> async feature will be always broken on 20.11 LTS, right?
>>>
>>> Yes, if only backporting this fix patch to 20.11 LTS, it may break
>>> apps who call async registration functions inside vhost callbacks.
>>>
>>> How about making this patch not a fix, but a new feature?
>>
>> Async will be still broken in v20.11 in this case.
>> Maybe the better thing would be to remove async support in v20.11, as its
>> support was quite limited in that release anyway. Does that make sense?
> 
> The code of supporting async vhost are beyond 1000 lines. I am afraid that
> removing such more code in 20.11 LTS may get objected. Can we note async
> register/unregister only work in new_/destroy_device, and using them in
> other vhost callback functions will cause deadlock in 20.11 LTS instead?
> Does it make sense to you?

You are right, that removing 1L LoC in LTS might not be the best idea,
since it will cause conflicts when doing backports later on.

Maybe the best way is to return -1 at async registration time, with
logging an error?

Thanks,
Maxime

>  Thanks,
> Jiayu
>>
>> Thanks,
>> Maxime
>>
>>> Thanks,
>>> Jiayu
>>>>
>>>> What do you think?
>>>>
>>>> Thanks,
>>>> Maxime
> 

Re: [dpdk-dev] [PATCH 0/2] provide thread unsafe async registration functions

[Hu, Jiayu]

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Thursday, July 1, 2021 11:43 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>; Maxime Coquelin
> <mcoqueli@redhat.com>; dev@dpdk.org
> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Wang, Yinan
> <yinan.wang@intel.com>; David Marchand <david.marchand@redhat.com>
> Subject: Re: [PATCH 0/2] provide thread unsafe async registration functions
> 
> Hi Jiayu,
> 
> On 6/29/21 7:36 AM, Hu, Jiayu wrote:
> > Hi Maxime,
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin <mcoqueli@redhat.com>
> >> Sent: Monday, June 7, 2021 9:20 PM
> >> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
> >> Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>;
> >> Wang, Yinan <yinan.wang@intel.com>
> >> Subject: Re: [PATCH 0/2] provide thread unsafe async registration
> >> functions
> >>
> >> Hi Jiayu,
> >>
> >> On 6/7/21 10:07 AM, Hu, Jiayu wrote:
> >>> Hi Maxime,
> >>>
> >>>> -----Original Message-----
> >>>> From: Maxime Coquelin <mcoqueli@redhat.com>
> >>>> Sent: Friday, June 4, 2021 3:25 PM
> >>>> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
> >>>> Cc: maxime.coquelin@redhat.com; Xia, Chenbo
> <chenbo.xia@intel.com>;
> >>>> Wang, Yinan <yinan.wang@intel.com>
> >>>> Subject: Re: [PATCH 0/2] provide thread unsafe async registration
> >>>> functions
> >>>>
> >>>> Sorry, for previous blank reply.
> >>>>
> >>>> On 5/28/21 10:11 AM, Jiayu Hu wrote:
> >>>>> Lock protection is needed during the vhost notifies the
> >>>>> application of device readiness, so the first patch is to add lock
> protection.
> >>>>> After performing locking, existed async vhost registration
> >>>>> functions will cause deadlock, as they acquire lock too. So the
> >>>>> second patch is to provide unsafe registration functions to
> >>>>> support calling within vhost callback functions.
> >>>>
> >>>> I agree the callback should be always protected, and in that case
> >>>> having a new thread-unsafe API makes sense for async registration.
> >>>>
> >>>> Regarding backport, I'm not sure what we should do.
> >>>>
> >>>> Backporting new API is a no-go, but with only backporting patch 1
> >>>> async feature will be always broken on 20.11 LTS, right?
> >>>
> >>> Yes, if only backporting this fix patch to 20.11 LTS, it may break
> >>> apps who call async registration functions inside vhost callbacks.
> >>>
> >>> How about making this patch not a fix, but a new feature?
> >>
> >> Async will be still broken in v20.11 in this case.
> >> Maybe the better thing would be to remove async support in v20.11, as
> >> its support was quite limited in that release anyway. Does that make
> sense?
> >
> > The code of supporting async vhost are beyond 1000 lines. I am afraid
> > that removing such more code in 20.11 LTS may get objected. Can we
> > note async register/unregister only work in new_/destroy_device, and
> > using them in other vhost callback functions will cause deadlock in 20.11
> LTS instead?
> > Does it make sense to you?
> 
> You are right, that removing 1L LoC in LTS might not be the best idea, since it
> will cause conflicts when doing backports later on.
> 
> Maybe the best way is to return -1 at async registration time, with logging an
> error?

OK.

Thanks,
Jiayu

> 
> Thanks,
> Maxime
> 
> >  Thanks,
> > Jiayu
> >>
> >> Thanks,
> >> Maxime
> >>
> >>> Thanks,
> >>> Jiayu
> >>>>
> >>>> What do you think?
> >>>>
> >>>> Thanks,
> >>>> Maxime
> >

Re: [dpdk-dev] [PATCH 1/2] vhost: fix lock on device readiness notification

[Maxime Coquelin]

On 5/28/21 10:11 AM, Jiayu Hu wrote:
> The vhost notifies the application of device readiness via
> vhost_user_notify_queue_state(), but calling this function
> is not protected by the lock. This patch is to make this
> function call lock protected.
> 
> Fixes: d0fcc38f5fa4 ("vhost: improve device readiness notifications")
> Cc: stable@dpdk.org
> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> ---
>  lib/vhost/vhost_user.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
> index 8f0eba6..dabce26 100644
> --- a/lib/vhost/vhost_user.c
> +++ b/lib/vhost/vhost_user.c
> @@ -2915,9 +2915,6 @@ vhost_user_msg_handler(int vid, int fd)
>  		}
>  	}
>  
> -	if (unlock_required)
> -		vhost_user_unlock_all_queue_pairs(dev);
> -
>  	/* If message was not handled at this stage, treat it as an error */
>  	if (!handled) {
>  		VHOST_LOG_CONFIG(ERR,
> @@ -2952,6 +2949,8 @@ vhost_user_msg_handler(int vid, int fd)
>  		}
>  	}
>  
> +	if (unlock_required)
> +		vhost_user_unlock_all_queue_pairs(dev);
>  
>  	if (!virtio_is_ready(dev))
>  		goto out;
> 

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

Thanks,
Maxime

Re: [dpdk-dev] [PATCH 2/2] vhost: add thread unsafe async registration functions

[Maxime Coquelin]

On 5/28/21 10:11 AM, Jiayu Hu wrote:
> This patch is to add thread unsafe version for async register and

s/is to add/adds/

> unregister functions.
> 
> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> ---
>  doc/guides/prog_guide/vhost_lib.rst |  12 +++
>  lib/vhost/rte_vhost_async.h         |  42 ++++++++++
>  lib/vhost/version.map               |   4 +
>  lib/vhost/vhost.c                   | 161 +++++++++++++++++++++++++++---------
>  4 files changed, 178 insertions(+), 41 deletions(-)
> 
> diff --git a/doc/guides/prog_guide/vhost_lib.rst b/doc/guides/prog_guide/vhost_lib.rst
> index d18fb98..6b2745f 100644
> --- a/doc/guides/prog_guide/vhost_lib.rst
> +++ b/doc/guides/prog_guide/vhost_lib.rst
> @@ -253,6 +253,12 @@ The following is an overview of some key Vhost API functions:
>      vhost invokes this function to get the copy data completed by async
>      devices.
>  
> +* ``rte_vhost_async_channel_register_thread_unsafe(vid, queue_id, features, ops)``
> +  Register a vhost queue with async copy device channel without
> +  performing any locking.
> +
> +  This function is only safe to call from within vhost callback functions.

I'm not sure with this, AFAICS .new_device() is called without the lock
held, so is that safe? Since .new_device() is the first time the app
gets required device info to use the Vhost API, that might not be an
issue, though.

Re: [dpdk-dev] [PATCH 2/2] vhost: add thread unsafe async registration functions

[Hu, Jiayu]

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Friday, July 2, 2021 3:41 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Wang, Yinan
> <yinan.wang@intel.com>
> Subject: Re: [PATCH 2/2] vhost: add thread unsafe async registration
> functions
> 
> 
> 
> On 5/28/21 10:11 AM, Jiayu Hu wrote:
> > This patch is to add thread unsafe version for async register and
> 
> s/is to add/adds/
> 
> > unregister functions.
> >
> > Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> > ---
> >  doc/guides/prog_guide/vhost_lib.rst |  12 +++
> >  lib/vhost/rte_vhost_async.h         |  42 ++++++++++
> >  lib/vhost/version.map               |   4 +
> >  lib/vhost/vhost.c                   | 161 +++++++++++++++++++++++++++---------
> >  4 files changed, 178 insertions(+), 41 deletions(-)
> >
> > diff --git a/doc/guides/prog_guide/vhost_lib.rst
> > b/doc/guides/prog_guide/vhost_lib.rst
> > index d18fb98..6b2745f 100644
> > --- a/doc/guides/prog_guide/vhost_lib.rst
> > +++ b/doc/guides/prog_guide/vhost_lib.rst
> > @@ -253,6 +253,12 @@ The following is an overview of some key Vhost
> API functions:
> >      vhost invokes this function to get the copy data completed by async
> >      devices.
> >
> > +* ``rte_vhost_async_channel_register_thread_unsafe(vid, queue_id,
> > +features, ops)``
> > +  Register a vhost queue with async copy device channel without
> > +  performing any locking.
> > +
> > +  This function is only safe to call from within vhost callback functions.
> 
> I'm not sure with this, AFAICS .new_device() is called without the lock held,
> so is that safe? Since .new_device() is the first time the app gets required
> device info to use the Vhost API, that might not be an issue, though.

I think so. Although new_device() is called without lock, no data plane threads
can operate vrings if device is not created.

Thanks,
Jiayu

Re: [dpdk-dev] [PATCH 2/2] vhost: add thread unsafe async registration functions

[Maxime Coquelin]

On 5/28/21 10:11 AM, Jiayu Hu wrote:
> This patch is to add thread unsafe version for async register and
> unregister functions.
> 
> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> ---
>  doc/guides/prog_guide/vhost_lib.rst |  12 +++
>  lib/vhost/rte_vhost_async.h         |  42 ++++++++++
>  lib/vhost/version.map               |   4 +
>  lib/vhost/vhost.c                   | 161 +++++++++++++++++++++++++++---------
>  4 files changed, 178 insertions(+), 41 deletions(-)
> 
> diff --git a/doc/guides/prog_guide/vhost_lib.rst b/doc/guides/prog_guide/vhost_lib.rst
> index d18fb98..6b2745f 100644
> --- a/doc/guides/prog_guide/vhost_lib.rst
> +++ b/doc/guides/prog_guide/vhost_lib.rst
> @@ -253,6 +253,12 @@ The following is an overview of some key Vhost API functions:
>      vhost invokes this function to get the copy data completed by async
>      devices.
>  
> +* ``rte_vhost_async_channel_register_thread_unsafe(vid, queue_id, features, ops)``
> +  Register a vhost queue with async copy device channel without
> +  performing any locking.
> +
> +  This function is only safe to call from within vhost callback functions.
> +
>  * ``rte_vhost_async_channel_unregister(vid, queue_id)``
>  
>    Unregister the async copy device channel from a vhost queue.
> @@ -265,6 +271,12 @@ The following is an overview of some key Vhost API functions:
>    devices for all vhost queues in destroy_device(), when a
>    virtio device is paused or shut down.
>  
> +* ``rte_vhost_async_channel_unregister_thread_unsafe(vid, queue_id)``
> +  Unregister the async copy device channel from a vhost queue without
> +  performing any locking.
> +
> +  This function is only safe to call from within vhost callback functions.
> +
>  * ``rte_vhost_submit_enqueue_burst(vid, queue_id, pkts, count, comp_pkts, comp_count)``
>  
>    Submit an enqueue request to transmit ``count`` packets from host to guest
> diff --git a/lib/vhost/rte_vhost_async.h b/lib/vhost/rte_vhost_async.h
> index 6faa31f..d054b5f 100644
> --- a/lib/vhost/rte_vhost_async.h
> +++ b/lib/vhost/rte_vhost_async.h
> @@ -143,6 +143,48 @@ __rte_experimental
>  int rte_vhost_async_channel_unregister(int vid, uint16_t queue_id);
>  
>  /**
> + * register an async channel for vhost without performing any locking
> + *
> + * @note This function does not perform any locking, and is only safe to call
> + *       from within vhost callback functions.
> + *
> + * @param vid
> + *  vhost device id async channel to be attached to
> + * @param queue_id
> + *  vhost queue id async channel to be attached to
> + * @param features
> + *  DMA channel feature bit
> + *    b0       : DMA supports inorder data transfer
> + *    b1  - b15: reserved
> + *    b16 - b27: Packet length threshold for DMA transfer
> + *    b28 - b31: reserved
> + * @param ops
> + *  DMA operation callbacks
> + * @return
> + *  0 on success, -1 on failures
> + */
> +__rte_experimental
> +int rte_vhost_async_channel_register_thread_unsafe(int vid, uint16_t queue_id,
> +	uint32_t features, struct rte_vhost_async_channel_ops *ops);
> +
> +/**
> + * unregister a dma channel for vhost without performing any lock
> + *
> + * @note This function does not perform any locking, and is only safe to call
> + *       from within vhost callback functions.
> + *
> + * @param vid
> + *  vhost device id DMA channel to be detached
> + * @param queue_id
> + *  vhost queue id DMA channel to be detached
> + * @return
> + *  0 on success, -1 on failures
> + */
> +__rte_experimental
> +int rte_vhost_async_channel_unregister_thread_unsafe(int vid,
> +		uint16_t queue_id);
> +
> +/**
>   * This function submits enqueue data to async engine. Successfully
>   * enqueued packets can be transfer completed or being occupied by DMA
>   * engines, when this API returns. Transfer completed packets are returned
> diff --git a/lib/vhost/version.map b/lib/vhost/version.map
> index 9103a23..2363db8 100644
> --- a/lib/vhost/version.map
> +++ b/lib/vhost/version.map
> @@ -79,4 +79,8 @@ EXPERIMENTAL {
>  
>  	# added in 21.05
>  	rte_vhost_get_negotiated_protocol_features;
> +
> +	# added in 21.08
> +	rte_vhost_async_channel_register_thread_unsafe;
> +	rte_vhost_async_channel_unregister_thread_unsafe;
>  };
> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> index c96f633..025e150 100644
> --- a/lib/vhost/vhost.c
> +++ b/lib/vhost/vhost.c
> @@ -1609,46 +1609,20 @@ int rte_vhost_extern_callback_register(int vid,
>  	return 0;
>  }
>  
> -int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
> -					uint32_t features,
> -					struct rte_vhost_async_channel_ops *ops)
> +static __rte_always_inline int
> +async_channel_register(int vid, uint16_t queue_id,
> +		struct rte_vhost_async_features f,
> +		struct rte_vhost_async_channel_ops *ops)
>  {
> -	struct vhost_virtqueue *vq;
>  	struct virtio_net *dev = get_device(vid);
> -	struct rte_vhost_async_features f;
> +	struct vhost_virtqueue *vq = dev->virtqueue[queue_id];
>  	int node;
>  
> -	if (dev == NULL || ops == NULL)
> -		return -1;
> -
> -	f.intval = features;
> -
> -	if (queue_id >= VHOST_MAX_VRING)
> -		return -1;
> -
> -	vq = dev->virtqueue[queue_id];
> -
> -	if (unlikely(vq == NULL || !dev->async_copy))
> -		return -1;
> -
> -	if (unlikely(!f.async_inorder)) {
> -		VHOST_LOG_CONFIG(ERR,
> -			"async copy is not supported on non-inorder mode "
> -			"(vid %d, qid: %d)\n", vid, queue_id);
> -		return -1;
> -	}
> -
> -	if (unlikely(ops->check_completed_copies == NULL ||
> -		ops->transfer_data == NULL))
> -		return -1;
> -
> -	rte_spinlock_lock(&vq->access_lock);
> -
>  	if (unlikely(vq->async_registered)) {
>  		VHOST_LOG_CONFIG(ERR,
>  			"async register failed: channel already registered "
>  			"(vid %d, qid: %d)\n", vid, queue_id);
> -		goto reg_out;
> +		return -1;
>  	}
>  
>  #ifdef RTE_LIBRTE_VHOST_NUMA
> @@ -1670,7 +1644,7 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
>  		VHOST_LOG_CONFIG(ERR,
>  			"async register failed: cannot allocate memory for async_pkts_info "
>  			"(vid %d, qid: %d)\n", vid, queue_id);
> -		goto reg_out;
> +		return -1;
>  	}
>  
>  	vq->it_pool = rte_malloc_socket(NULL,
> @@ -1681,7 +1655,7 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
>  		VHOST_LOG_CONFIG(ERR,
>  			"async register failed: cannot allocate memory for it_pool "
>  			"(vid %d, qid: %d)\n", vid, queue_id);
> -		goto reg_out;
> +		return -1;
>  	}
>  
>  	vq->vec_pool = rte_malloc_socket(NULL,
> @@ -1692,7 +1666,7 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
>  		VHOST_LOG_CONFIG(ERR,
>  			"async register failed: cannot allocate memory for vec_pool "
>  			"(vid %d, qid: %d)\n", vid, queue_id);
> -		goto reg_out;
> +		return -1;
>  	}
>  
>  	if (vq_is_packed(dev)) {
> @@ -1704,7 +1678,7 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
>  			VHOST_LOG_CONFIG(ERR,
>  				"async register failed: cannot allocate memory for async buffers "
>  				"(vid %d, qid: %d)\n", vid, queue_id);
> -			goto reg_out;
> +			return -1;
>  		}
>  	} else {
>  		vq->async_descs_split = rte_malloc_socket(NULL,
> @@ -1715,22 +1689,92 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
>  			VHOST_LOG_CONFIG(ERR,
>  				"async register failed: cannot allocate memory for async descs "
>  				"(vid %d, qid: %d)\n", vid, queue_id);
> -			goto reg_out;
> +			return -1;
>  		}
>  	}
>  
>  	vq->async_ops.check_completed_copies = ops->check_completed_copies;
>  	vq->async_ops.transfer_data = ops->transfer_data;
> -
>  	vq->async_inorder = f.async_inorder;
>  	vq->async_threshold = f.async_threshold;
> -
>  	vq->async_registered = true;
>  
> -reg_out:
> +	return 0;
> +}
> +
> +int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
> +					uint32_t features,
> +					struct rte_vhost_async_channel_ops *ops)
> +{
> +	struct vhost_virtqueue *vq;
> +	struct virtio_net *dev = get_device(vid);
> +	struct rte_vhost_async_features f;
> +	int ret;
> +
> +	if (dev == NULL || ops == NULL)
> +		return -1;
> +
> +	f.intval = features;

Not directly related to this patch set, but could you please rework
struct rte_vhost_async_features? There is no point to pack the flags and
threshold values.

Also, the prototype should just pass the struct directly, or add
different fields for the threshold and the features.


> +
> +	if (queue_id >= VHOST_MAX_VRING)
> +		return -1;
> +
> +	vq = dev->virtqueue[queue_id];
> +
> +	if (unlikely(vq == NULL || !dev->async_copy))
> +		return -1;
> +
> +	if (unlikely(!f.async_inorder)) {
> +		VHOST_LOG_CONFIG(ERR,
> +			"async copy is not supported on non-inorder mode "
> +			"(vid %d, qid: %d)\n", vid, queue_id);
> +		return -1;
> +	}
> +
> +	if (unlikely(ops->check_completed_copies == NULL ||
> +		ops->transfer_data == NULL))
> +		return -1;
> +
> +	rte_spinlock_lock(&vq->access_lock);
> +	ret = async_channel_register(vid, queue_id, f, ops);
>  	rte_spinlock_unlock(&vq->access_lock);
>  
> -	return 0;
> +	return ret;
> +}
> +
> +int rte_vhost_async_channel_register_thread_unsafe(int vid, uint16_t queue_id,
> +					uint32_t features,
> +					struct rte_vhost_async_channel_ops *ops)
> +{
> +	struct vhost_virtqueue *vq;
> +	struct virtio_net *dev = get_device(vid);
> +	struct rte_vhost_async_features f;
> +
> +	if (dev == NULL || ops == NULL)
> +		return -1;
> +
> +	f.intval = features;
> +
> +	if (queue_id >= VHOST_MAX_VRING)
> +		return -1;
> +
> +	vq = dev->virtqueue[queue_id];
> +
> +	if (unlikely(vq == NULL || !dev->async_copy))
> +		return -1;
> +
> +	if (unlikely(!f.async_inorder)) {
> +		VHOST_LOG_CONFIG(ERR,
> +			"async copy is not supported on non-inorder mode "
> +			"(vid %d, qid: %d)\n", vid, queue_id);
> +		return -1;
> +	}
> +
> +	if (unlikely(ops->check_completed_copies == NULL ||
> +		ops->transfer_data == NULL))
> +		return -1;
> +
> +	return async_channel_register(vid, queue_id, f, ops);
>  }
>  
>  int rte_vhost_async_channel_unregister(int vid, uint16_t queue_id)
> @@ -1780,5 +1824,40 @@ int rte_vhost_async_channel_unregister(int vid, uint16_t queue_id)
>  	return ret;
>  }
>  
> +int rte_vhost_async_channel_unregister_thread_unsafe(int vid,
> +		uint16_t queue_id)
> +{
> +	struct vhost_virtqueue *vq;
> +	struct virtio_net *dev = get_device(vid);
> +
> +	if (dev == NULL)
> +		return -1;
> +
> +	if (queue_id >= VHOST_MAX_VRING)
> +		return -1;
> +
> +	vq = dev->virtqueue[queue_id];
> +
> +	if (vq == NULL)
> +		return -1;
> +
> +	if (!vq->async_registered)
> +		return 0;
> +
> +	if (vq->async_pkts_inflight_n) {
> +		VHOST_LOG_CONFIG(ERR, "Failed to unregister async channel. "
> +			"async inflight packets must be completed before unregistration.\n");
> +		return -1;
> +	}
> +
> +	vhost_free_async_mem(vq);
> +
> +	vq->async_ops.transfer_data = NULL;
> +	vq->async_ops.check_completed_copies = NULL;
> +	vq->async_registered = false;
> +
> +	return 0;
> +}
> +
>  RTE_LOG_REGISTER_SUFFIX(vhost_config_log_level, config, INFO);
>  RTE_LOG_REGISTER_SUFFIX(vhost_data_log_level, data, WARNING);
> 

Re: [dpdk-dev] [PATCH 2/2] vhost: add thread unsafe async registration functions

[Hu, Jiayu]

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Monday, July 5, 2021 4:59 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Wang, Yinan
> <yinan.wang@intel.com>
> Subject: Re: [PATCH 2/2] vhost: add thread unsafe async registration
> functions
> On 5/28/21 10:11 AM, Jiayu Hu wrote:
> > This patch is to add thread unsafe version for async register and
> > unregister functions.
> >
> > Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> > ---
> >  doc/guides/prog_guide/vhost_lib.rst |  12 +++
> >  lib/vhost/rte_vhost_async.h         |  42 ++++++++++
> >  lib/vhost/version.map               |   4 +
> >  lib/vhost/vhost.c                   | 161 +++++++++++++++++++++++++++---------
> >  4 files changed, 178 insertions(+), 41 deletions(-)
> >
> > diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c index
> > c96f633..025e150 100644
> > --- a/lib/vhost/vhost.c
> > +++ b/lib/vhost/vhost.c
> > @@ -1609,46 +1609,20 @@ int rte_vhost_extern_callback_register(int vid,
> >  	return 0;
> >  }
> >
> > -int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
> > -					uint32_t features,
> > -					struct rte_vhost_async_channel_ops
> *ops)
> > +static __rte_always_inline int
> > +async_channel_register(int vid, uint16_t queue_id,
> > +		struct rte_vhost_async_features f,
> > +		struct rte_vhost_async_channel_ops *ops)
> >  {
> > -	struct vhost_virtqueue *vq;
> >  	struct virtio_net *dev = get_device(vid);
> > -	struct rte_vhost_async_features f;
> > +	struct vhost_virtqueue *vq = dev->virtqueue[queue_id];
> >  	int node;
> >
> > -	if (dev == NULL || ops == NULL)
> > -		return -1;
> > -
> > -	f.intval = features;
> > -
> > -	if (queue_id >= VHOST_MAX_VRING)
> > -		return -1;
> > -
> > -	vq = dev->virtqueue[queue_id];
> > -
> > -	if (unlikely(vq == NULL || !dev->async_copy))
> > -		return -1;
> > -
> > -	if (unlikely(!f.async_inorder)) {
> > -		VHOST_LOG_CONFIG(ERR,
> > -			"async copy is not supported on non-inorder mode "
> > -			"(vid %d, qid: %d)\n", vid, queue_id);
> > -		return -1;
> > -	}
> > -
> > -	if (unlikely(ops->check_completed_copies == NULL ||
> > -		ops->transfer_data == NULL))
> > -		return -1;
> > -
> > -	rte_spinlock_lock(&vq->access_lock);
> > -
> >  	if (unlikely(vq->async_registered)) {
> >  		VHOST_LOG_CONFIG(ERR,
> >  			"async register failed: channel already registered "
> >  			"(vid %d, qid: %d)\n", vid, queue_id);
> > -		goto reg_out;
> > +		return -1;
> >  	}
> >
> >  #ifdef RTE_LIBRTE_VHOST_NUMA
> > @@ -1670,7 +1644,7 @@ int rte_vhost_async_channel_register(int vid,
> uint16_t queue_id,
> >  		VHOST_LOG_CONFIG(ERR,
> >  			"async register failed: cannot allocate memory for
> async_pkts_info "
> >  			"(vid %d, qid: %d)\n", vid, queue_id);
> > -		goto reg_out;
> > +		return -1;
> >  	}
> >
> >  	vq->it_pool = rte_malloc_socket(NULL, @@ -1681,7 +1655,7 @@ int
> > rte_vhost_async_channel_register(int vid, uint16_t queue_id,
> >  		VHOST_LOG_CONFIG(ERR,
> >  			"async register failed: cannot allocate memory for
> it_pool "
> >  			"(vid %d, qid: %d)\n", vid, queue_id);
> > -		goto reg_out;
> > +		return -1;
> >  	}
> >
> >  	vq->vec_pool = rte_malloc_socket(NULL, @@ -1692,7 +1666,7 @@
> int
> > rte_vhost_async_channel_register(int vid, uint16_t queue_id,
> >  		VHOST_LOG_CONFIG(ERR,
> >  			"async register failed: cannot allocate memory for
> vec_pool "
> >  			"(vid %d, qid: %d)\n", vid, queue_id);
> > -		goto reg_out;
> > +		return -1;
> >  	}
> >
> >  	if (vq_is_packed(dev)) {
> > @@ -1704,7 +1678,7 @@ int rte_vhost_async_channel_register(int vid,
> uint16_t queue_id,
> >  			VHOST_LOG_CONFIG(ERR,
> >  				"async register failed: cannot allocate
> memory for async buffers "
> >  				"(vid %d, qid: %d)\n", vid, queue_id);
> > -			goto reg_out;
> > +			return -1;
> >  		}
> >  	} else {
> >  		vq->async_descs_split = rte_malloc_socket(NULL, @@ -
> 1715,22
> > +1689,92 @@ int rte_vhost_async_channel_register(int vid, uint16_t
> queue_id,
> >  			VHOST_LOG_CONFIG(ERR,
> >  				"async register failed: cannot allocate
> memory for async descs "
> >  				"(vid %d, qid: %d)\n", vid, queue_id);
> > -			goto reg_out;
> > +			return -1;
> >  		}
> >  	}
> >
> >  	vq->async_ops.check_completed_copies = ops-
> >check_completed_copies;
> >  	vq->async_ops.transfer_data = ops->transfer_data;
> > -
> >  	vq->async_inorder = f.async_inorder;
> >  	vq->async_threshold = f.async_threshold;
> > -
> >  	vq->async_registered = true;
> >
> > -reg_out:
> > +	return 0;
> > +}
> > +
> > +int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
> > +					uint32_t features,
> > +					struct rte_vhost_async_channel_ops
> *ops) {
> > +	struct vhost_virtqueue *vq;
> > +	struct virtio_net *dev = get_device(vid);
> > +	struct rte_vhost_async_features f;
> > +	int ret;
> > +
> > +	if (dev == NULL || ops == NULL)
> > +		return -1;
> > +
> > +	f.intval = features;
> 
> Not directly related to this patch set, but could you please rework struct
> rte_vhost_async_features? There is no point to pack the flags and threshold
> values.
> 
> Also, the prototype should just pass the struct directly, or add different fields
> for the threshold and the features.
> 

Will rework the structure in v2.

Thanks,
Jiayu

Re: [dpdk-dev] [PATCH v2 2/3] vhost: rework async feature struct

[Maxime Coquelin]

Hi Jiayu,

On 7/7/21 1:18 PM, Jiayu Hu wrote:
> No need to use bitfields in the structure rte_vhost_async_features.
> This patch reworks the structure to improve code readability. In
> addition, add preserved padding fields on the structure for future
> usage.
> 
> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> ---
>  examples/vhost/main.c       |  4 ++--
>  lib/vhost/rte_vhost_async.h | 24 ++++++++----------------
>  lib/vhost/vhost.c           | 13 +++++--------
>  3 files changed, 15 insertions(+), 26 deletions(-)
> 
> diff --git a/examples/vhost/main.c b/examples/vhost/main.c
> index d2179ea..ec1a5b4 100644
> --- a/examples/vhost/main.c
> +++ b/examples/vhost/main.c
> @@ -1468,7 +1468,7 @@ new_device(int vid)
>  		vid, vdev->coreid);
>  
>  	if (async_vhost_driver) {
> -		struct rte_vhost_async_features f;
> +		struct rte_vhost_async_features f = {0};
>  		struct rte_vhost_async_channel_ops channel_ops;
>  
>  		if (dma_type != NULL && strncmp(dma_type, "ioat", 4) == 0) {
> @@ -1480,7 +1480,7 @@ new_device(int vid)
>  			f.async_threshold = 256;
>  
>  			return rte_vhost_async_channel_register(vid, VIRTIO_RXQ,
> -				f.intval, &channel_ops);
> +				f, &channel_ops);
>  		}
>  	}
>  
> diff --git a/lib/vhost/rte_vhost_async.h b/lib/vhost/rte_vhost_async.h
> index 6faa31f..12c6962 100644
> --- a/lib/vhost/rte_vhost_async.h
> +++ b/lib/vhost/rte_vhost_async.h
> @@ -93,18 +93,13 @@ struct async_inflight_info {
>  };
>  
>  /**
> - *  dma channel feature bit definition
> + *  dma channel features
>   */
>  struct rte_vhost_async_features {

Give the struct also contains config/tuning stuff, I don't think
rte_vhost_async_features is a good name, maybe rte_vhost_async_config?

> -	union {
> -		uint32_t intval;
> -		struct {
> -			uint32_t async_inorder:1;
> -			uint32_t resvd_0:15;
> -			uint32_t async_threshold:12;
> -			uint32_t resvd_1:4;
> -		};
> -	};
> +	uint32_t async_threshold;



> +	uint32_t resvd_0[13];

That's a lot of padding! I hope the application won't have to use that
much to configure the async feature one day :)

> +	uint8_t async_inorder;

I would prefer a uint32_t features bitfield for consistency with Vhost
library, with enums for the features definition.

> +	uint8_t resvd_1[7];
>  };
>  
>  /**
> @@ -115,11 +110,7 @@ struct rte_vhost_async_features {
>   * @param queue_id
>   *  vhost queue id async channel to be attached to
>   * @param features
> - *  DMA channel feature bit
> - *    b0       : DMA supports inorder data transfer
> - *    b1  - b15: reserved
> - *    b16 - b27: Packet length threshold for DMA transfer
> - *    b28 - b31: reserved
> + *  DMA channel feature structure
>   * @param ops
>   *  DMA operation callbacks
>   * @return
> @@ -127,7 +118,8 @@ struct rte_vhost_async_features {
>   */
>  __rte_experimental
>  int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
> -	uint32_t features, struct rte_vhost_async_channel_ops *ops);
> +	struct rte_vhost_async_features features,
> +	struct rte_vhost_async_channel_ops *ops);
>  
>  /**
>   * unregister a dma channel for vhost
> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> index 53a470f..835aad8 100644
> --- a/lib/vhost/vhost.c
> +++ b/lib/vhost/vhost.c
> @@ -1620,18 +1620,15 @@ int rte_vhost_extern_callback_register(int vid,
>  }
>  
>  int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
> -					uint32_t features,
> -					struct rte_vhost_async_channel_ops *ops)
> +		struct rte_vhost_async_features features,
> +		struct rte_vhost_async_channel_ops *ops)
>  {
>  	struct vhost_virtqueue *vq;
>  	struct virtio_net *dev = get_device(vid);
> -	struct rte_vhost_async_features f;
>  
>  	if (dev == NULL || ops == NULL)
>  		return -1;
>  
> -	f.intval = features;
> -
>  	if (queue_id >= VHOST_MAX_VRING)
>  		return -1;
>  
> @@ -1640,7 +1637,7 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
>  	if (unlikely(vq == NULL || !dev->async_copy))
>  		return -1;
>  
> -	if (unlikely(!f.async_inorder)) {
> +	if (unlikely(!features.async_inorder)) {
>  		VHOST_LOG_CONFIG(ERR,
>  			"async copy is not supported on non-inorder mode "
>  			"(vid %d, qid: %d)\n", vid, queue_id);
> @@ -1720,8 +1717,8 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
>  	vq->async_ops.check_completed_copies = ops->check_completed_copies;
>  	vq->async_ops.transfer_data = ops->transfer_data;
>  
> -	vq->async_inorder = f.async_inorder;
> -	vq->async_threshold = f.async_threshold;
> +	vq->async_inorder = features.async_inorder;
> +	vq->async_threshold = features.async_threshold;
>  
>  	vq->async_registered = true;
>  
> 

[dpdk-dev] [PATCH v2 0/3] provide thread unsafe async registration functions

[Jiayu Hu]

Lock protection is needed during the vhost notifies the application of
device readiness, so the first patch adds lock protection. In addition,
the second patch reworks async feature structure to improve readability.
After performing locking, existed async vhost registration functions will
cause deadlock, as they acquire lock too. The last patch provides thread
unsafe registration functions to support calling within vhost callback
functions.

v2:
* rework async feature structure
* fix typo in commit log

Jiayu Hu (3):
  vhost: fix lock on device readiness notification
  vhost: rework async feature struct
  vhost: add thread unsafe async registeration functions

 doc/guides/prog_guide/vhost_lib.rst |  12 +++
 examples/vhost/main.c               |   4 +-
 lib/vhost/rte_vhost_async.h         |  63 +++++++++++----
 lib/vhost/version.map               |   4 +
 lib/vhost/vhost.c                   | 157 ++++++++++++++++++++++++++----------
 lib/vhost/vhost_user.c              |   5 +-
 6 files changed, 183 insertions(+), 62 deletions(-)

-- 
2.7.4

[dpdk-dev] [PATCH v2 1/3] vhost: fix lock on device readiness notification

[Jiayu Hu]

The vhost notifies the application of device readiness via
vhost_user_notify_queue_state(), but calling this function
is not protected by the lock. This patch is to make this
function call lock protected.

Fixes: d0fcc38f5fa4 ("vhost: improve device readiness notifications")
Cc: stable@dpdk.org
Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/vhost/vhost_user.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
index 031c578..31300e1 100644
--- a/lib/vhost/vhost_user.c
+++ b/lib/vhost/vhost_user.c
@@ -2995,9 +2995,6 @@ vhost_user_msg_handler(int vid, int fd)
 		}
 	}
 
-	if (unlock_required)
-		vhost_user_unlock_all_queue_pairs(dev);
-
 	/* If message was not handled at this stage, treat it as an error */
 	if (!handled) {
 		VHOST_LOG_CONFIG(ERR,
@@ -3032,6 +3029,8 @@ vhost_user_msg_handler(int vid, int fd)
 		}
 	}
 
+	if (unlock_required)
+		vhost_user_unlock_all_queue_pairs(dev);
 
 	if (!virtio_is_ready(dev))
 		goto out;
-- 
2.7.4

[dpdk-dev] [PATCH v2 2/3] vhost: rework async feature struct

[Jiayu Hu]

No need to use bitfields in the structure rte_vhost_async_features.
This patch reworks the structure to improve code readability. In
addition, add preserved padding fields on the structure for future
usage.

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
---
 examples/vhost/main.c       |  4 ++--
 lib/vhost/rte_vhost_async.h | 24 ++++++++----------------
 lib/vhost/vhost.c           | 13 +++++--------
 3 files changed, 15 insertions(+), 26 deletions(-)

diff --git a/examples/vhost/main.c b/examples/vhost/main.c
index d2179ea..ec1a5b4 100644
--- a/examples/vhost/main.c
+++ b/examples/vhost/main.c
@@ -1468,7 +1468,7 @@ new_device(int vid)
 		vid, vdev->coreid);
 
 	if (async_vhost_driver) {
-		struct rte_vhost_async_features f;
+		struct rte_vhost_async_features f = {0};
 		struct rte_vhost_async_channel_ops channel_ops;
 
 		if (dma_type != NULL && strncmp(dma_type, "ioat", 4) == 0) {
@@ -1480,7 +1480,7 @@ new_device(int vid)
 			f.async_threshold = 256;
 
 			return rte_vhost_async_channel_register(vid, VIRTIO_RXQ,
-				f.intval, &channel_ops);
+				f, &channel_ops);
 		}
 	}
 
diff --git a/lib/vhost/rte_vhost_async.h b/lib/vhost/rte_vhost_async.h
index 6faa31f..12c6962 100644
--- a/lib/vhost/rte_vhost_async.h
+++ b/lib/vhost/rte_vhost_async.h
@@ -93,18 +93,13 @@ struct async_inflight_info {
 };
 
 /**
- *  dma channel feature bit definition
+ *  dma channel features
  */
 struct rte_vhost_async_features {
-	union {
-		uint32_t intval;
-		struct {
-			uint32_t async_inorder:1;
-			uint32_t resvd_0:15;
-			uint32_t async_threshold:12;
-			uint32_t resvd_1:4;
-		};
-	};
+	uint32_t async_threshold;
+	uint32_t resvd_0[13];
+	uint8_t async_inorder;
+	uint8_t resvd_1[7];
 };
 
 /**
@@ -115,11 +110,7 @@ struct rte_vhost_async_features {
  * @param queue_id
  *  vhost queue id async channel to be attached to
  * @param features
- *  DMA channel feature bit
- *    b0       : DMA supports inorder data transfer
- *    b1  - b15: reserved
- *    b16 - b27: Packet length threshold for DMA transfer
- *    b28 - b31: reserved
+ *  DMA channel feature structure
  * @param ops
  *  DMA operation callbacks
  * @return
@@ -127,7 +118,8 @@ struct rte_vhost_async_features {
  */
 __rte_experimental
 int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
-	uint32_t features, struct rte_vhost_async_channel_ops *ops);
+	struct rte_vhost_async_features features,
+	struct rte_vhost_async_channel_ops *ops);
 
 /**
  * unregister a dma channel for vhost
diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
index 53a470f..835aad8 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -1620,18 +1620,15 @@ int rte_vhost_extern_callback_register(int vid,
 }
 
 int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
-					uint32_t features,
-					struct rte_vhost_async_channel_ops *ops)
+		struct rte_vhost_async_features features,
+		struct rte_vhost_async_channel_ops *ops)
 {
 	struct vhost_virtqueue *vq;
 	struct virtio_net *dev = get_device(vid);
-	struct rte_vhost_async_features f;
 
 	if (dev == NULL || ops == NULL)
 		return -1;
 
-	f.intval = features;
-
 	if (queue_id >= VHOST_MAX_VRING)
 		return -1;
 
@@ -1640,7 +1637,7 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 	if (unlikely(vq == NULL || !dev->async_copy))
 		return -1;
 
-	if (unlikely(!f.async_inorder)) {
+	if (unlikely(!features.async_inorder)) {
 		VHOST_LOG_CONFIG(ERR,
 			"async copy is not supported on non-inorder mode "
 			"(vid %d, qid: %d)\n", vid, queue_id);
@@ -1720,8 +1717,8 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 	vq->async_ops.check_completed_copies = ops->check_completed_copies;
 	vq->async_ops.transfer_data = ops->transfer_data;
 
-	vq->async_inorder = f.async_inorder;
-	vq->async_threshold = f.async_threshold;
+	vq->async_inorder = features.async_inorder;
+	vq->async_threshold = features.async_threshold;
 
 	vq->async_registered = true;
 
-- 
2.7.4

[dpdk-dev] [PATCH v2 3/3] vhost: add thread unsafe async registeration functions

[Jiayu Hu]

This patch adds thread unsafe version for async register and
unregister functions.

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
---
 doc/guides/prog_guide/vhost_lib.rst |  12 +++
 lib/vhost/rte_vhost_async.h         |  39 +++++++++
 lib/vhost/version.map               |   4 +
 lib/vhost/vhost.c                   | 154 +++++++++++++++++++++++++++---------
 4 files changed, 171 insertions(+), 38 deletions(-)

diff --git a/doc/guides/prog_guide/vhost_lib.rst b/doc/guides/prog_guide/vhost_lib.rst
index d18fb98..6b2745f 100644
--- a/doc/guides/prog_guide/vhost_lib.rst
+++ b/doc/guides/prog_guide/vhost_lib.rst
@@ -253,6 +253,12 @@ The following is an overview of some key Vhost API functions:
     vhost invokes this function to get the copy data completed by async
     devices.
 
+* ``rte_vhost_async_channel_register_thread_unsafe(vid, queue_id, features, ops)``
+  Register a vhost queue with async copy device channel without
+  performing any locking.
+
+  This function is only safe to call from within vhost callback functions.
+
 * ``rte_vhost_async_channel_unregister(vid, queue_id)``
 
   Unregister the async copy device channel from a vhost queue.
@@ -265,6 +271,12 @@ The following is an overview of some key Vhost API functions:
   devices for all vhost queues in destroy_device(), when a
   virtio device is paused or shut down.
 
+* ``rte_vhost_async_channel_unregister_thread_unsafe(vid, queue_id)``
+  Unregister the async copy device channel from a vhost queue without
+  performing any locking.
+
+  This function is only safe to call from within vhost callback functions.
+
 * ``rte_vhost_submit_enqueue_burst(vid, queue_id, pkts, count, comp_pkts, comp_count)``
 
   Submit an enqueue request to transmit ``count`` packets from host to guest
diff --git a/lib/vhost/rte_vhost_async.h b/lib/vhost/rte_vhost_async.h
index 12c6962..6a93fcd 100644
--- a/lib/vhost/rte_vhost_async.h
+++ b/lib/vhost/rte_vhost_async.h
@@ -135,6 +135,45 @@ __rte_experimental
 int rte_vhost_async_channel_unregister(int vid, uint16_t queue_id);
 
 /**
+ * register an async channel for vhost without performing any locking
+ *
+ * @note This function does not perform any locking, and is only safe to call
+ *       from within vhost callback functions.
+ *
+ * @param vid
+ *  vhost device id async channel to be attached to
+ * @param queue_id
+ *  vhost queue id async channel to be attached to
+ * @param features
+ *  DMA channel feature structure
+ * @param ops
+ *  DMA operation callbacks
+ * @return
+ *  0 on success, -1 on failures
+ */
+__rte_experimental
+int rte_vhost_async_channel_register_thread_unsafe(int vid, uint16_t queue_id,
+	struct rte_vhost_async_features features,
+	struct rte_vhost_async_channel_ops *ops);
+
+/**
+ * unregister a dma channel for vhost without performing any lock
+ *
+ * @note This function does not perform any locking, and is only safe to call
+ *       from within vhost callback functions.
+ *
+ * @param vid
+ *  vhost device id DMA channel to be detached
+ * @param queue_id
+ *  vhost queue id DMA channel to be detached
+ * @return
+ *  0 on success, -1 on failures
+ */
+__rte_experimental
+int rte_vhost_async_channel_unregister_thread_unsafe(int vid,
+		uint16_t queue_id);
+
+/**
  * This function submits enqueue data to async engine. Successfully
  * enqueued packets can be transfer completed or being occupied by DMA
  * engines, when this API returns. Transfer completed packets are returned
diff --git a/lib/vhost/version.map b/lib/vhost/version.map
index 9103a23..2363db8 100644
--- a/lib/vhost/version.map
+++ b/lib/vhost/version.map
@@ -79,4 +79,8 @@ EXPERIMENTAL {
 
 	# added in 21.05
 	rte_vhost_get_negotiated_protocol_features;
+
+	# added in 21.08
+	rte_vhost_async_channel_register_thread_unsafe;
+	rte_vhost_async_channel_unregister_thread_unsafe;
 };
diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
index 835aad8..68d006a 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -1619,42 +1619,19 @@ int rte_vhost_extern_callback_register(int vid,
 	return 0;
 }
 
-int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
-		struct rte_vhost_async_features features,
+static __rte_always_inline int
+async_channel_register(int vid, uint16_t queue_id,
+		struct rte_vhost_async_features f,
 		struct rte_vhost_async_channel_ops *ops)
 {
-	struct vhost_virtqueue *vq;
 	struct virtio_net *dev = get_device(vid);
-
-	if (dev == NULL || ops == NULL)
-		return -1;
-
-	if (queue_id >= VHOST_MAX_VRING)
-		return -1;
-
-	vq = dev->virtqueue[queue_id];
-
-	if (unlikely(vq == NULL || !dev->async_copy))
-		return -1;
-
-	if (unlikely(!features.async_inorder)) {
-		VHOST_LOG_CONFIG(ERR,
-			"async copy is not supported on non-inorder mode "
-			"(vid %d, qid: %d)\n", vid, queue_id);
-		return -1;
-	}
-
-	if (unlikely(ops->check_completed_copies == NULL ||
-		ops->transfer_data == NULL))
-		return -1;
-
-	rte_spinlock_lock(&vq->access_lock);
+	struct vhost_virtqueue *vq = dev->virtqueue[queue_id];
 
 	if (unlikely(vq->async_registered)) {
 		VHOST_LOG_CONFIG(ERR,
 			"async register failed: channel already registered "
 			"(vid %d, qid: %d)\n", vid, queue_id);
-		goto reg_out;
+		return -1;
 	}
 
 	vq->async_pkts_info = rte_malloc_socket(NULL,
@@ -1665,7 +1642,7 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 		VHOST_LOG_CONFIG(ERR,
 			"async register failed: cannot allocate memory for async_pkts_info "
 			"(vid %d, qid: %d)\n", vid, queue_id);
-		goto reg_out;
+		return -1;
 	}
 
 	vq->it_pool = rte_malloc_socket(NULL,
@@ -1676,7 +1653,7 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 		VHOST_LOG_CONFIG(ERR,
 			"async register failed: cannot allocate memory for it_pool "
 			"(vid %d, qid: %d)\n", vid, queue_id);
-		goto reg_out;
+		return -1;
 	}
 
 	vq->vec_pool = rte_malloc_socket(NULL,
@@ -1687,7 +1664,7 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 		VHOST_LOG_CONFIG(ERR,
 			"async register failed: cannot allocate memory for vec_pool "
 			"(vid %d, qid: %d)\n", vid, queue_id);
-		goto reg_out;
+		return -1;
 	}
 
 	if (vq_is_packed(dev)) {
@@ -1699,7 +1676,7 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 			VHOST_LOG_CONFIG(ERR,
 				"async register failed: cannot allocate memory for async buffers "
 				"(vid %d, qid: %d)\n", vid, queue_id);
-			goto reg_out;
+			return -1;
 		}
 	} else {
 		vq->async_descs_split = rte_malloc_socket(NULL,
@@ -1710,22 +1687,88 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 			VHOST_LOG_CONFIG(ERR,
 				"async register failed: cannot allocate memory for async descs "
 				"(vid %d, qid: %d)\n", vid, queue_id);
-			goto reg_out;
+			return -1;
 		}
 	}
 
 	vq->async_ops.check_completed_copies = ops->check_completed_copies;
 	vq->async_ops.transfer_data = ops->transfer_data;
+	vq->async_inorder = f.async_inorder;
+	vq->async_threshold = f.async_threshold;
+	vq->async_registered = true;
 
-	vq->async_inorder = features.async_inorder;
-	vq->async_threshold = features.async_threshold;
+	return 0;
+}
 
-	vq->async_registered = true;
+int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
+		struct rte_vhost_async_features features,
+		struct rte_vhost_async_channel_ops *ops)
+{
+	struct vhost_virtqueue *vq;
+	struct virtio_net *dev = get_device(vid);
+	int ret;
+
+	if (dev == NULL || ops == NULL)
+		return -1;
+
+	if (queue_id >= VHOST_MAX_VRING)
+		return -1;
+
+	vq = dev->virtqueue[queue_id];
+
+	if (unlikely(vq == NULL || !dev->async_copy))
+		return -1;
+
+	if (unlikely(!features.async_inorder)) {
+		VHOST_LOG_CONFIG(ERR,
+			"async copy is not supported on non-inorder mode "
+			"(vid %d, qid: %d)\n", vid, queue_id);
+		return -1;
+	}
+
+	if (unlikely(ops->check_completed_copies == NULL ||
+		ops->transfer_data == NULL)) {
+		return -1;
+	}
 
-reg_out:
+	rte_spinlock_lock(&vq->access_lock);
+	ret = async_channel_register(vid, queue_id, features, ops);
 	rte_spinlock_unlock(&vq->access_lock);
 
-	return 0;
+	return ret;
+}
+
+int rte_vhost_async_channel_register_thread_unsafe(int vid, uint16_t queue_id,
+		struct rte_vhost_async_features features,
+		struct rte_vhost_async_channel_ops *ops)
+{
+	struct vhost_virtqueue *vq;
+	struct virtio_net *dev = get_device(vid);
+
+	if (dev == NULL || ops == NULL)
+		return -1;
+
+	if (queue_id >= VHOST_MAX_VRING)
+		return -1;
+
+	vq = dev->virtqueue[queue_id];
+
+	if (unlikely(vq == NULL || !dev->async_copy))
+		return -1;
+
+	if (unlikely(!features.async_inorder)) {
+		VHOST_LOG_CONFIG(ERR,
+			"async copy is not supported on non-inorder mode "
+			"(vid %d, qid: %d)\n", vid, queue_id);
+		return -1;
+	}
+
+	if (unlikely(ops->check_completed_copies == NULL ||
+		ops->transfer_data == NULL)) {
+		return -1;
+	}
+
+	return async_channel_register(vid, queue_id, features, ops);
 }
 
 int rte_vhost_async_channel_unregister(int vid, uint16_t queue_id)
@@ -1775,5 +1818,40 @@ int rte_vhost_async_channel_unregister(int vid, uint16_t queue_id)
 	return ret;
 }
 
+int rte_vhost_async_channel_unregister_thread_unsafe(int vid,
+		uint16_t queue_id)
+{
+	struct vhost_virtqueue *vq;
+	struct virtio_net *dev = get_device(vid);
+
+	if (dev == NULL)
+		return -1;
+
+	if (queue_id >= VHOST_MAX_VRING)
+		return -1;
+
+	vq = dev->virtqueue[queue_id];
+
+	if (vq == NULL)
+		return -1;
+
+	if (!vq->async_registered)
+		return 0;
+
+	if (vq->async_pkts_inflight_n) {
+		VHOST_LOG_CONFIG(ERR, "Failed to unregister async channel. "
+			"async inflight packets must be completed before unregistration.\n");
+		return -1;
+	}
+
+	vhost_free_async_mem(vq);
+
+	vq->async_ops.transfer_data = NULL;
+	vq->async_ops.check_completed_copies = NULL;
+	vq->async_registered = false;
+
+	return 0;
+}
+
 RTE_LOG_REGISTER_SUFFIX(vhost_config_log_level, config, INFO);
 RTE_LOG_REGISTER_SUFFIX(vhost_data_log_level, data, WARNING);
-- 
2.7.4

[dpdk-dev] [PATCH v3 0/3] provide thread unsafe async registration functions

[Jiayu Hu]

Lock protection is needed during the vhost notifies the application of
device readiness, so the first patch adds lock protection. In addition,
the second patch reworks async feature structure to improve readability.
After performing locking, existed async vhost registration functions will
cause deadlock, as they acquire lock too. The last patch provides thread
unsafe registration functions to support calling within vhost callback
functions.

v3:
* rename and use enum to define async device features
* change padding fields to 8 bytes
v2:
* rework async feature structure
* fix typo in commit log

Jiayu Hu (3):
  vhost: fix lock on device readiness notification
  vhost: rework async configuration struct
  vhost: add thread unsafe async registeration functions

 doc/guides/prog_guide/vhost_lib.rst |  31 +++++--
 examples/vhost/main.c               |   8 +-
 lib/vhost/rte_vhost_async.h         |  74 ++++++++++++----
 lib/vhost/version.map               |   4 +
 lib/vhost/vhost.c                   | 165 +++++++++++++++++++++++++++---------
 lib/vhost/vhost_user.c              |   5 +-
 6 files changed, 213 insertions(+), 74 deletions(-)

-- 
2.7.4

[dpdk-dev] [PATCH v3 1/3] vhost: fix lock on device readiness notification

[Jiayu Hu]

The vhost notifies the application of device readiness via
vhost_user_notify_queue_state(), but calling this function
is not protected by the lock. This patch is to make this
function call lock protected.

Fixes: d0fcc38f5fa4 ("vhost: improve device readiness notifications")
Cc: stable@dpdk.org
Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/vhost/vhost_user.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
index 031c578..31300e1 100644
--- a/lib/vhost/vhost_user.c
+++ b/lib/vhost/vhost_user.c
@@ -2995,9 +2995,6 @@ vhost_user_msg_handler(int vid, int fd)
 		}
 	}
 
-	if (unlock_required)
-		vhost_user_unlock_all_queue_pairs(dev);
-
 	/* If message was not handled at this stage, treat it as an error */
 	if (!handled) {
 		VHOST_LOG_CONFIG(ERR,
@@ -3032,6 +3029,8 @@ vhost_user_msg_handler(int vid, int fd)
 		}
 	}
 
+	if (unlock_required)
+		vhost_user_unlock_all_queue_pairs(dev);
 
 	if (!virtio_is_ready(dev))
 		goto out;
-- 
2.7.4

[dpdk-dev] [PATCH v3 2/3] vhost: rework async configuration struct

[Jiayu Hu]

This patch reworks the async configuration structure to improve code
readability. In addition, add preserved padding fields on the structure
for future usage.

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
---
 doc/guides/prog_guide/vhost_lib.rst | 19 +++++++++++--------
 examples/vhost/main.c               |  8 ++++----
 lib/vhost/rte_vhost_async.h         | 35 +++++++++++++++++------------------
 lib/vhost/vhost.c                   | 13 +++++--------
 4 files changed, 37 insertions(+), 38 deletions(-)

diff --git a/doc/guides/prog_guide/vhost_lib.rst b/doc/guides/prog_guide/vhost_lib.rst
index d18fb98..affdc57 100644
--- a/doc/guides/prog_guide/vhost_lib.rst
+++ b/doc/guides/prog_guide/vhost_lib.rst
@@ -218,26 +218,29 @@ The following is an overview of some key Vhost API functions:
 
   Enable or disable zero copy feature of the vhost crypto backend.
 
-* ``rte_vhost_async_channel_register(vid, queue_id, features, ops)``
+* ``rte_vhost_async_channel_register(vid, queue_id, config, ops)``
 
   Register a vhost queue with async copy device channel after vring
-  is enabled. Following device ``features`` must be specified together
+  is enabled. Following device ``config`` must be specified together
   with the registration:
 
-  * ``async_inorder``
+  * ``features``
 
-    Async copy device can guarantee the ordering of copy completion
-    sequence. Copies are completed in the same order with that at
-    the submission time.
+    This field is used to specify async copy device features.
 
-    Currently, only ``async_inorder`` capable device is supported by vhost.
+    ``RTE_VHOST_ASYNC_INORDER`` represents the async copy device can
+    guarantee the order of copy completion is the same as the order
+    of copy submission.
+
+    Currently, only ``RTE_VHOST_ASYNC_INORDER`` capable device is
+    supported by vhost.
 
   * ``async_threshold``
 
     The copy length (in bytes) below which CPU copy will be used even if
     applications call async vhost APIs to enqueue/dequeue data.
 
-    Typical value is 512~1024 depending on the async device capability.
+    Typical value is 256~1024 depending on the async device capability.
 
   Applications must provide following ``ops`` callbacks for vhost lib to
   work with the async copy devices:
diff --git a/examples/vhost/main.c b/examples/vhost/main.c
index d2179ea..9cd855a 100644
--- a/examples/vhost/main.c
+++ b/examples/vhost/main.c
@@ -1468,7 +1468,7 @@ new_device(int vid)
 		vid, vdev->coreid);
 
 	if (async_vhost_driver) {
-		struct rte_vhost_async_features f;
+		struct rte_vhost_async_config config = {0};
 		struct rte_vhost_async_channel_ops channel_ops;
 
 		if (dma_type != NULL && strncmp(dma_type, "ioat", 4) == 0) {
@@ -1476,11 +1476,11 @@ new_device(int vid)
 			channel_ops.check_completed_copies =
 				ioat_check_completed_copies_cb;
 
-			f.async_inorder = 1;
-			f.async_threshold = 256;
+			config.features = RTE_VHOST_ASYNC_INORDER;
+			config.async_threshold = 256;
 
 			return rte_vhost_async_channel_register(vid, VIRTIO_RXQ,
-				f.intval, &channel_ops);
+				config, &channel_ops);
 		}
 	}
 
diff --git a/lib/vhost/rte_vhost_async.h b/lib/vhost/rte_vhost_async.h
index 6faa31f..c93490d 100644
--- a/lib/vhost/rte_vhost_async.h
+++ b/lib/vhost/rte_vhost_async.h
@@ -93,18 +93,20 @@ struct async_inflight_info {
 };
 
 /**
- *  dma channel feature bit definition
+ *  dma channel features
  */
-struct rte_vhost_async_features {
-	union {
-		uint32_t intval;
-		struct {
-			uint32_t async_inorder:1;
-			uint32_t resvd_0:15;
-			uint32_t async_threshold:12;
-			uint32_t resvd_1:4;
-		};
-	};
+enum {
+	RTE_VHOST_ASYNC_FEATURE_UNKNOWN = 0U,
+	RTE_VHOST_ASYNC_INORDER = 1U << 0,
+};
+
+/**
+ *  dma channel configuration
+ */
+struct rte_vhost_async_config {
+	uint32_t async_threshold;
+	uint32_t features;
+	uint32_t resvd[2];
 };
 
 /**
@@ -114,12 +116,8 @@ struct rte_vhost_async_features {
  *  vhost device id async channel to be attached to
  * @param queue_id
  *  vhost queue id async channel to be attached to
- * @param features
- *  DMA channel feature bit
- *    b0       : DMA supports inorder data transfer
- *    b1  - b15: reserved
- *    b16 - b27: Packet length threshold for DMA transfer
- *    b28 - b31: reserved
+ * @param config
+ *  DMA channel configuration structure
  * @param ops
  *  DMA operation callbacks
  * @return
@@ -127,7 +125,8 @@ struct rte_vhost_async_features {
  */
 __rte_experimental
 int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
-	uint32_t features, struct rte_vhost_async_channel_ops *ops);
+	struct rte_vhost_async_config config,
+	struct rte_vhost_async_channel_ops *ops);
 
 /**
  * unregister a dma channel for vhost
diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
index 53a470f..a20f05a 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -1620,18 +1620,15 @@ int rte_vhost_extern_callback_register(int vid,
 }
 
 int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
-					uint32_t features,
-					struct rte_vhost_async_channel_ops *ops)
+		struct rte_vhost_async_config config,
+		struct rte_vhost_async_channel_ops *ops)
 {
 	struct vhost_virtqueue *vq;
 	struct virtio_net *dev = get_device(vid);
-	struct rte_vhost_async_features f;
 
 	if (dev == NULL || ops == NULL)
 		return -1;
 
-	f.intval = features;
-
 	if (queue_id >= VHOST_MAX_VRING)
 		return -1;
 
@@ -1640,7 +1637,7 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 	if (unlikely(vq == NULL || !dev->async_copy))
 		return -1;
 
-	if (unlikely(!f.async_inorder)) {
+	if (unlikely(!(config.features & RTE_VHOST_ASYNC_INORDER))) {
 		VHOST_LOG_CONFIG(ERR,
 			"async copy is not supported on non-inorder mode "
 			"(vid %d, qid: %d)\n", vid, queue_id);
@@ -1720,8 +1717,8 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 	vq->async_ops.check_completed_copies = ops->check_completed_copies;
 	vq->async_ops.transfer_data = ops->transfer_data;
 
-	vq->async_inorder = f.async_inorder;
-	vq->async_threshold = f.async_threshold;
+	vq->async_inorder = true;
+	vq->async_threshold = config.async_threshold;
 
 	vq->async_registered = true;
 
-- 
2.7.4

[dpdk-dev] [PATCH v3 3/3] vhost: add thread unsafe async registeration functions

[Jiayu Hu]

This patch adds thread unsafe version for async register and
unregister functions.

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
---
 doc/guides/prog_guide/vhost_lib.rst |  12 +++
 lib/vhost/rte_vhost_async.h         |  39 +++++++++
 lib/vhost/version.map               |   4 +
 lib/vhost/vhost.c                   | 154 ++++++++++++++++++++++++++++--------
 4 files changed, 175 insertions(+), 34 deletions(-)

diff --git a/doc/guides/prog_guide/vhost_lib.rst b/doc/guides/prog_guide/vhost_lib.rst
index affdc57..ffe09a3 100644
--- a/doc/guides/prog_guide/vhost_lib.rst
+++ b/doc/guides/prog_guide/vhost_lib.rst
@@ -256,6 +256,12 @@ The following is an overview of some key Vhost API functions:
     vhost invokes this function to get the copy data completed by async
     devices.
 
+* ``rte_vhost_async_channel_register_thread_unsafe(vid, queue_id, config, ops)``
+  Register a vhost queue with async copy device channel without
+  performing any locking.
+
+  This function is only safe to call from within vhost callback functions.
+
 * ``rte_vhost_async_channel_unregister(vid, queue_id)``
 
   Unregister the async copy device channel from a vhost queue.
@@ -268,6 +274,12 @@ The following is an overview of some key Vhost API functions:
   devices for all vhost queues in destroy_device(), when a
   virtio device is paused or shut down.
 
+* ``rte_vhost_async_channel_unregister_thread_unsafe(vid, queue_id)``
+  Unregister the async copy device channel from a vhost queue without
+  performing any locking.
+
+  This function is only safe to call from within vhost callback functions.
+
 * ``rte_vhost_submit_enqueue_burst(vid, queue_id, pkts, count, comp_pkts, comp_count)``
 
   Submit an enqueue request to transmit ``count`` packets from host to guest
diff --git a/lib/vhost/rte_vhost_async.h b/lib/vhost/rte_vhost_async.h
index c93490d..e4d20a3 100644
--- a/lib/vhost/rte_vhost_async.h
+++ b/lib/vhost/rte_vhost_async.h
@@ -142,6 +142,45 @@ __rte_experimental
 int rte_vhost_async_channel_unregister(int vid, uint16_t queue_id);
 
 /**
+ * register an async channel for vhost without performing any locking
+ *
+ * @note This function does not perform any locking, and is only safe to call
+ *       from within vhost callback functions.
+ *
+ * @param vid
+ *  vhost device id async channel to be attached to
+ * @param queue_id
+ *  vhost queue id async channel to be attached to
+ * @param config
+ *  DMA channel configuration
+ * @param ops
+ *  DMA operation callbacks
+ * @return
+ *  0 on success, -1 on failures
+ */
+__rte_experimental
+int rte_vhost_async_channel_register_thread_unsafe(int vid, uint16_t queue_id,
+	struct rte_vhost_async_config config,
+	struct rte_vhost_async_channel_ops *ops);
+
+/**
+ * unregister a dma channel for vhost without performing any lock
+ *
+ * @note This function does not perform any locking, and is only safe to call
+ *       from within vhost callback functions.
+ *
+ * @param vid
+ *  vhost device id DMA channel to be detached
+ * @param queue_id
+ *  vhost queue id DMA channel to be detached
+ * @return
+ *  0 on success, -1 on failures
+ */
+__rte_experimental
+int rte_vhost_async_channel_unregister_thread_unsafe(int vid,
+		uint16_t queue_id);
+
+/**
  * This function submits enqueue data to async engine. Successfully
  * enqueued packets can be transfer completed or being occupied by DMA
  * engines, when this API returns. Transfer completed packets are returned
diff --git a/lib/vhost/version.map b/lib/vhost/version.map
index 9103a23..2363db8 100644
--- a/lib/vhost/version.map
+++ b/lib/vhost/version.map
@@ -79,4 +79,8 @@ EXPERIMENTAL {
 
 	# added in 21.05
 	rte_vhost_get_negotiated_protocol_features;
+
+	# added in 21.08
+	rte_vhost_async_channel_register_thread_unsafe;
+	rte_vhost_async_channel_unregister_thread_unsafe;
 };
diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
index a20f05a..785de54 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -1619,42 +1619,19 @@ int rte_vhost_extern_callback_register(int vid,
 	return 0;
 }
 
-int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
+static __rte_always_inline int
+async_channel_register(int vid, uint16_t queue_id,
 		struct rte_vhost_async_config config,
 		struct rte_vhost_async_channel_ops *ops)
 {
-	struct vhost_virtqueue *vq;
 	struct virtio_net *dev = get_device(vid);
-
-	if (dev == NULL || ops == NULL)
-		return -1;
-
-	if (queue_id >= VHOST_MAX_VRING)
-		return -1;
-
-	vq = dev->virtqueue[queue_id];
-
-	if (unlikely(vq == NULL || !dev->async_copy))
-		return -1;
-
-	if (unlikely(!(config.features & RTE_VHOST_ASYNC_INORDER))) {
-		VHOST_LOG_CONFIG(ERR,
-			"async copy is not supported on non-inorder mode "
-			"(vid %d, qid: %d)\n", vid, queue_id);
-		return -1;
-	}
-
-	if (unlikely(ops->check_completed_copies == NULL ||
-		ops->transfer_data == NULL))
-		return -1;
-
-	rte_spinlock_lock(&vq->access_lock);
+	struct vhost_virtqueue *vq = dev->virtqueue[queue_id];
 
 	if (unlikely(vq->async_registered)) {
 		VHOST_LOG_CONFIG(ERR,
 			"async register failed: channel already registered "
 			"(vid %d, qid: %d)\n", vid, queue_id);
-		goto reg_out;
+		return -1;
 	}
 
 	vq->async_pkts_info = rte_malloc_socket(NULL,
@@ -1665,7 +1642,7 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 		VHOST_LOG_CONFIG(ERR,
 			"async register failed: cannot allocate memory for async_pkts_info "
 			"(vid %d, qid: %d)\n", vid, queue_id);
-		goto reg_out;
+		return -1;
 	}
 
 	vq->it_pool = rte_malloc_socket(NULL,
@@ -1676,7 +1653,7 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 		VHOST_LOG_CONFIG(ERR,
 			"async register failed: cannot allocate memory for it_pool "
 			"(vid %d, qid: %d)\n", vid, queue_id);
-		goto reg_out;
+		return -1;
 	}
 
 	vq->vec_pool = rte_malloc_socket(NULL,
@@ -1687,7 +1664,7 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 		VHOST_LOG_CONFIG(ERR,
 			"async register failed: cannot allocate memory for vec_pool "
 			"(vid %d, qid: %d)\n", vid, queue_id);
-		goto reg_out;
+		return -1;
 	}
 
 	if (vq_is_packed(dev)) {
@@ -1699,7 +1676,7 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 			VHOST_LOG_CONFIG(ERR,
 				"async register failed: cannot allocate memory for async buffers "
 				"(vid %d, qid: %d)\n", vid, queue_id);
-			goto reg_out;
+			return -1;
 		}
 	} else {
 		vq->async_descs_split = rte_malloc_socket(NULL,
@@ -1710,7 +1687,7 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 			VHOST_LOG_CONFIG(ERR,
 				"async register failed: cannot allocate memory for async descs "
 				"(vid %d, qid: %d)\n", vid, queue_id);
-			goto reg_out;
+			return -1;
 		}
 	}
 
@@ -1722,10 +1699,84 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 
 	vq->async_registered = true;
 
-reg_out:
+	return 0;
+}
+
+int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
+		struct rte_vhost_async_config config,
+		struct rte_vhost_async_channel_ops *ops)
+{
+	struct vhost_virtqueue *vq;
+	struct virtio_net *dev = get_device(vid);
+	int ret;
+
+	if (dev == NULL || ops == NULL) {
+		return -1;
+	}
+
+	if (queue_id >= VHOST_MAX_VRING) {
+		return -1;
+	}
+
+	vq = dev->virtqueue[queue_id];
+
+	if (unlikely(vq == NULL || !dev->async_copy)) {
+		return -1;
+	}
+
+	if (unlikely(!(config.features & RTE_VHOST_ASYNC_INORDER))) {
+		VHOST_LOG_CONFIG(ERR,
+			"async copy is not supported on non-inorder mode "
+			"(vid %d, qid: %d)\n", vid, queue_id);
+		return -1;
+	}
+
+	if (unlikely(ops->check_completed_copies == NULL ||
+		ops->transfer_data == NULL)) {
+		return -1;
+	}
+
+	rte_spinlock_lock(&vq->access_lock);
+	ret = async_channel_register(vid, queue_id, config, ops);
 	rte_spinlock_unlock(&vq->access_lock);
 
-	return 0;
+	return ret;
+}
+
+int rte_vhost_async_channel_register_thread_unsafe(int vid, uint16_t queue_id,
+		struct rte_vhost_async_config config,
+		struct rte_vhost_async_channel_ops *ops)
+{
+	struct vhost_virtqueue *vq;
+	struct virtio_net *dev = get_device(vid);
+
+	if (dev == NULL || ops == NULL) {
+		return -1;
+	}
+
+	if (queue_id >= VHOST_MAX_VRING) {
+		return -1;
+	}
+
+	vq = dev->virtqueue[queue_id];
+
+	if (unlikely(vq == NULL || !dev->async_copy)) {
+		return -1;
+	}
+
+	if (unlikely(!(config.features & RTE_VHOST_ASYNC_INORDER))) {
+		VHOST_LOG_CONFIG(ERR,
+			"async copy is not supported on non-inorder mode "
+			"(vid %d, qid: %d)\n", vid, queue_id);
+		return -1;
+	}
+
+	if (unlikely(ops->check_completed_copies == NULL ||
+		ops->transfer_data == NULL)) {
+		return -1;
+	}
+
+	return async_channel_register(vid, queue_id, config, ops);
 }
 
 int rte_vhost_async_channel_unregister(int vid, uint16_t queue_id)
@@ -1775,5 +1826,40 @@ int rte_vhost_async_channel_unregister(int vid, uint16_t queue_id)
 	return ret;
 }
 
+int rte_vhost_async_channel_unregister_thread_unsafe(int vid,
+		uint16_t queue_id)
+{
+	struct vhost_virtqueue *vq;
+	struct virtio_net *dev = get_device(vid);
+
+	if (dev == NULL)
+		return -1;
+
+	if (queue_id >= VHOST_MAX_VRING)
+		return -1;
+
+	vq = dev->virtqueue[queue_id];
+
+	if (vq == NULL)
+		return -1;
+
+	if (!vq->async_registered)
+		return 0;
+
+	if (vq->async_pkts_inflight_n) {
+		VHOST_LOG_CONFIG(ERR, "Failed to unregister async channel. "
+			"async inflight packets must be completed before unregistration.\n");
+		return -1;
+	}
+
+	vhost_free_async_mem(vq);
+
+	vq->async_ops.transfer_data = NULL;
+	vq->async_ops.check_completed_copies = NULL;
+	vq->async_registered = false;
+
+	return 0;
+}
+
 RTE_LOG_REGISTER_SUFFIX(vhost_config_log_level, config, INFO);
 RTE_LOG_REGISTER_SUFFIX(vhost_data_log_level, data, WARNING);
-- 
2.7.4

[dpdk-dev] [PATCH v4 0/3] provide thread unsafe async registration functions

[Jiayu Hu]

Lock protection is needed during the vhost notifies the application of
device readiness, so the first patch adds lock protection. In addition,
the second patch reworks async feature structure to improve readability.
After performing locking, existed async vhost registration functions will
cause deadlock, as they acquire lock too. The last patch provides thread
unsafe registration functions to support calling within vhost callback
functions.

v4:
* remove brace {} in single statement block
v3:
* rename and use enum to define async device features
* change padding fields to 8 bytes
v2:
* rework async feature structure
* fix typo in commit log

Jiayu Hu (3):
  vhost: fix lock on device readiness notification
  vhost: rework async configuration struct
  vhost: add thread unsafe async registeration functions

 doc/guides/prog_guide/vhost_lib.rst |  31 +++++--
 examples/vhost/main.c               |   8 +-
 lib/vhost/rte_vhost_async.h         |  74 ++++++++++++-----
 lib/vhost/version.map               |   4 +
 lib/vhost/vhost.c                   | 157 ++++++++++++++++++++++++++----------
 lib/vhost/vhost_user.c              |   5 +-
 6 files changed, 205 insertions(+), 74 deletions(-)

-- 
2.7.4

[dpdk-dev] [PATCH v4 1/3] vhost: fix lock on device readiness notification

[Jiayu Hu]

The vhost notifies the application of device readiness via
vhost_user_notify_queue_state(), but calling this function
is not protected by the lock. This patch is to make this
function call lock protected.

Fixes: d0fcc38f5fa4 ("vhost: improve device readiness notifications")
Cc: stable@dpdk.org

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/vhost/vhost_user.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
index 031c578..31300e1 100644
--- a/lib/vhost/vhost_user.c
+++ b/lib/vhost/vhost_user.c
@@ -2995,9 +2995,6 @@ vhost_user_msg_handler(int vid, int fd)
 		}
 	}
 
-	if (unlock_required)
-		vhost_user_unlock_all_queue_pairs(dev);
-
 	/* If message was not handled at this stage, treat it as an error */
 	if (!handled) {
 		VHOST_LOG_CONFIG(ERR,
@@ -3032,6 +3029,8 @@ vhost_user_msg_handler(int vid, int fd)
 		}
 	}
 
+	if (unlock_required)
+		vhost_user_unlock_all_queue_pairs(dev);
 
 	if (!virtio_is_ready(dev))
 		goto out;
-- 
2.7.4

[dpdk-dev] [PATCH v4 2/3] vhost: rework async configuration struct

[Jiayu Hu]

This patch reworks the async configuration structure to improve code
readability. In addition, add preserved padding fields on the structure
for future usage.

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
---
 doc/guides/prog_guide/vhost_lib.rst | 19 +++++++++++--------
 examples/vhost/main.c               |  8 ++++----
 lib/vhost/rte_vhost_async.h         | 35 +++++++++++++++++------------------
 lib/vhost/vhost.c                   | 13 +++++--------
 4 files changed, 37 insertions(+), 38 deletions(-)

diff --git a/doc/guides/prog_guide/vhost_lib.rst b/doc/guides/prog_guide/vhost_lib.rst
index d18fb98..affdc57 100644
--- a/doc/guides/prog_guide/vhost_lib.rst
+++ b/doc/guides/prog_guide/vhost_lib.rst
@@ -218,26 +218,29 @@ The following is an overview of some key Vhost API functions:
 
   Enable or disable zero copy feature of the vhost crypto backend.
 
-* ``rte_vhost_async_channel_register(vid, queue_id, features, ops)``
+* ``rte_vhost_async_channel_register(vid, queue_id, config, ops)``
 
   Register a vhost queue with async copy device channel after vring
-  is enabled. Following device ``features`` must be specified together
+  is enabled. Following device ``config`` must be specified together
   with the registration:
 
-  * ``async_inorder``
+  * ``features``
 
-    Async copy device can guarantee the ordering of copy completion
-    sequence. Copies are completed in the same order with that at
-    the submission time.
+    This field is used to specify async copy device features.
 
-    Currently, only ``async_inorder`` capable device is supported by vhost.
+    ``RTE_VHOST_ASYNC_INORDER`` represents the async copy device can
+    guarantee the order of copy completion is the same as the order
+    of copy submission.
+
+    Currently, only ``RTE_VHOST_ASYNC_INORDER`` capable device is
+    supported by vhost.
 
   * ``async_threshold``
 
     The copy length (in bytes) below which CPU copy will be used even if
     applications call async vhost APIs to enqueue/dequeue data.
 
-    Typical value is 512~1024 depending on the async device capability.
+    Typical value is 256~1024 depending on the async device capability.
 
   Applications must provide following ``ops`` callbacks for vhost lib to
   work with the async copy devices:
diff --git a/examples/vhost/main.c b/examples/vhost/main.c
index d2179ea..9cd855a 100644
--- a/examples/vhost/main.c
+++ b/examples/vhost/main.c
@@ -1468,7 +1468,7 @@ new_device(int vid)
 		vid, vdev->coreid);
 
 	if (async_vhost_driver) {
-		struct rte_vhost_async_features f;
+		struct rte_vhost_async_config config = {0};
 		struct rte_vhost_async_channel_ops channel_ops;
 
 		if (dma_type != NULL && strncmp(dma_type, "ioat", 4) == 0) {
@@ -1476,11 +1476,11 @@ new_device(int vid)
 			channel_ops.check_completed_copies =
 				ioat_check_completed_copies_cb;
 
-			f.async_inorder = 1;
-			f.async_threshold = 256;
+			config.features = RTE_VHOST_ASYNC_INORDER;
+			config.async_threshold = 256;
 
 			return rte_vhost_async_channel_register(vid, VIRTIO_RXQ,
-				f.intval, &channel_ops);
+				config, &channel_ops);
 		}
 	}
 
diff --git a/lib/vhost/rte_vhost_async.h b/lib/vhost/rte_vhost_async.h
index 6faa31f..c93490d 100644
--- a/lib/vhost/rte_vhost_async.h
+++ b/lib/vhost/rte_vhost_async.h
@@ -93,18 +93,20 @@ struct async_inflight_info {
 };
 
 /**
- *  dma channel feature bit definition
+ *  dma channel features
  */
-struct rte_vhost_async_features {
-	union {
-		uint32_t intval;
-		struct {
-			uint32_t async_inorder:1;
-			uint32_t resvd_0:15;
-			uint32_t async_threshold:12;
-			uint32_t resvd_1:4;
-		};
-	};
+enum {
+	RTE_VHOST_ASYNC_FEATURE_UNKNOWN = 0U,
+	RTE_VHOST_ASYNC_INORDER = 1U << 0,
+};
+
+/**
+ *  dma channel configuration
+ */
+struct rte_vhost_async_config {
+	uint32_t async_threshold;
+	uint32_t features;
+	uint32_t resvd[2];
 };
 
 /**
@@ -114,12 +116,8 @@ struct rte_vhost_async_features {
  *  vhost device id async channel to be attached to
  * @param queue_id
  *  vhost queue id async channel to be attached to
- * @param features
- *  DMA channel feature bit
- *    b0       : DMA supports inorder data transfer
- *    b1  - b15: reserved
- *    b16 - b27: Packet length threshold for DMA transfer
- *    b28 - b31: reserved
+ * @param config
+ *  DMA channel configuration structure
  * @param ops
  *  DMA operation callbacks
  * @return
@@ -127,7 +125,8 @@ struct rte_vhost_async_features {
  */
 __rte_experimental
 int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
-	uint32_t features, struct rte_vhost_async_channel_ops *ops);
+	struct rte_vhost_async_config config,
+	struct rte_vhost_async_channel_ops *ops);
 
 /**
  * unregister a dma channel for vhost
diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
index 53a470f..a20f05a 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -1620,18 +1620,15 @@ int rte_vhost_extern_callback_register(int vid,
 }
 
 int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
-					uint32_t features,
-					struct rte_vhost_async_channel_ops *ops)
+		struct rte_vhost_async_config config,
+		struct rte_vhost_async_channel_ops *ops)
 {
 	struct vhost_virtqueue *vq;
 	struct virtio_net *dev = get_device(vid);
-	struct rte_vhost_async_features f;
 
 	if (dev == NULL || ops == NULL)
 		return -1;
 
-	f.intval = features;
-
 	if (queue_id >= VHOST_MAX_VRING)
 		return -1;
 
@@ -1640,7 +1637,7 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 	if (unlikely(vq == NULL || !dev->async_copy))
 		return -1;
 
-	if (unlikely(!f.async_inorder)) {
+	if (unlikely(!(config.features & RTE_VHOST_ASYNC_INORDER))) {
 		VHOST_LOG_CONFIG(ERR,
 			"async copy is not supported on non-inorder mode "
 			"(vid %d, qid: %d)\n", vid, queue_id);
@@ -1720,8 +1717,8 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 	vq->async_ops.check_completed_copies = ops->check_completed_copies;
 	vq->async_ops.transfer_data = ops->transfer_data;
 
-	vq->async_inorder = f.async_inorder;
-	vq->async_threshold = f.async_threshold;
+	vq->async_inorder = true;
+	vq->async_threshold = config.async_threshold;
 
 	vq->async_registered = true;
 
-- 
2.7.4

[dpdk-dev] [PATCH v4 3/3] vhost: add thread unsafe async registeration functions

[Jiayu Hu]

This patch adds thread unsafe version for async register and
unregister functions.

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
---
 doc/guides/prog_guide/vhost_lib.rst |  12 +++
 lib/vhost/rte_vhost_async.h         |  39 ++++++++++
 lib/vhost/version.map               |   4 +
 lib/vhost/vhost.c                   | 146 +++++++++++++++++++++++++++---------
 4 files changed, 167 insertions(+), 34 deletions(-)

diff --git a/doc/guides/prog_guide/vhost_lib.rst b/doc/guides/prog_guide/vhost_lib.rst
index affdc57..ffe09a3 100644
--- a/doc/guides/prog_guide/vhost_lib.rst
+++ b/doc/guides/prog_guide/vhost_lib.rst
@@ -256,6 +256,12 @@ The following is an overview of some key Vhost API functions:
     vhost invokes this function to get the copy data completed by async
     devices.
 
+* ``rte_vhost_async_channel_register_thread_unsafe(vid, queue_id, config, ops)``
+  Register a vhost queue with async copy device channel without
+  performing any locking.
+
+  This function is only safe to call from within vhost callback functions.
+
 * ``rte_vhost_async_channel_unregister(vid, queue_id)``
 
   Unregister the async copy device channel from a vhost queue.
@@ -268,6 +274,12 @@ The following is an overview of some key Vhost API functions:
   devices for all vhost queues in destroy_device(), when a
   virtio device is paused or shut down.
 
+* ``rte_vhost_async_channel_unregister_thread_unsafe(vid, queue_id)``
+  Unregister the async copy device channel from a vhost queue without
+  performing any locking.
+
+  This function is only safe to call from within vhost callback functions.
+
 * ``rte_vhost_submit_enqueue_burst(vid, queue_id, pkts, count, comp_pkts, comp_count)``
 
   Submit an enqueue request to transmit ``count`` packets from host to guest
diff --git a/lib/vhost/rte_vhost_async.h b/lib/vhost/rte_vhost_async.h
index c93490d..e4d20a3 100644
--- a/lib/vhost/rte_vhost_async.h
+++ b/lib/vhost/rte_vhost_async.h
@@ -142,6 +142,45 @@ __rte_experimental
 int rte_vhost_async_channel_unregister(int vid, uint16_t queue_id);
 
 /**
+ * register an async channel for vhost without performing any locking
+ *
+ * @note This function does not perform any locking, and is only safe to call
+ *       from within vhost callback functions.
+ *
+ * @param vid
+ *  vhost device id async channel to be attached to
+ * @param queue_id
+ *  vhost queue id async channel to be attached to
+ * @param config
+ *  DMA channel configuration
+ * @param ops
+ *  DMA operation callbacks
+ * @return
+ *  0 on success, -1 on failures
+ */
+__rte_experimental
+int rte_vhost_async_channel_register_thread_unsafe(int vid, uint16_t queue_id,
+	struct rte_vhost_async_config config,
+	struct rte_vhost_async_channel_ops *ops);
+
+/**
+ * unregister a dma channel for vhost without performing any lock
+ *
+ * @note This function does not perform any locking, and is only safe to call
+ *       from within vhost callback functions.
+ *
+ * @param vid
+ *  vhost device id DMA channel to be detached
+ * @param queue_id
+ *  vhost queue id DMA channel to be detached
+ * @return
+ *  0 on success, -1 on failures
+ */
+__rte_experimental
+int rte_vhost_async_channel_unregister_thread_unsafe(int vid,
+		uint16_t queue_id);
+
+/**
  * This function submits enqueue data to async engine. Successfully
  * enqueued packets can be transfer completed or being occupied by DMA
  * engines, when this API returns. Transfer completed packets are returned
diff --git a/lib/vhost/version.map b/lib/vhost/version.map
index 9103a23..2363db8 100644
--- a/lib/vhost/version.map
+++ b/lib/vhost/version.map
@@ -79,4 +79,8 @@ EXPERIMENTAL {
 
 	# added in 21.05
 	rte_vhost_get_negotiated_protocol_features;
+
+	# added in 21.08
+	rte_vhost_async_channel_register_thread_unsafe;
+	rte_vhost_async_channel_unregister_thread_unsafe;
 };
diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
index a20f05a..2d12ce5 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -1619,42 +1619,19 @@ int rte_vhost_extern_callback_register(int vid,
 	return 0;
 }
 
-int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
+static __rte_always_inline int
+async_channel_register(int vid, uint16_t queue_id,
 		struct rte_vhost_async_config config,
 		struct rte_vhost_async_channel_ops *ops)
 {
-	struct vhost_virtqueue *vq;
 	struct virtio_net *dev = get_device(vid);
-
-	if (dev == NULL || ops == NULL)
-		return -1;
-
-	if (queue_id >= VHOST_MAX_VRING)
-		return -1;
-
-	vq = dev->virtqueue[queue_id];
-
-	if (unlikely(vq == NULL || !dev->async_copy))
-		return -1;
-
-	if (unlikely(!(config.features & RTE_VHOST_ASYNC_INORDER))) {
-		VHOST_LOG_CONFIG(ERR,
-			"async copy is not supported on non-inorder mode "
-			"(vid %d, qid: %d)\n", vid, queue_id);
-		return -1;
-	}
-
-	if (unlikely(ops->check_completed_copies == NULL ||
-		ops->transfer_data == NULL))
-		return -1;
-
-	rte_spinlock_lock(&vq->access_lock);
+	struct vhost_virtqueue *vq = dev->virtqueue[queue_id];
 
 	if (unlikely(vq->async_registered)) {
 		VHOST_LOG_CONFIG(ERR,
 			"async register failed: channel already registered "
 			"(vid %d, qid: %d)\n", vid, queue_id);
-		goto reg_out;
+		return -1;
 	}
 
 	vq->async_pkts_info = rte_malloc_socket(NULL,
@@ -1665,7 +1642,7 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 		VHOST_LOG_CONFIG(ERR,
 			"async register failed: cannot allocate memory for async_pkts_info "
 			"(vid %d, qid: %d)\n", vid, queue_id);
-		goto reg_out;
+		return -1;
 	}
 
 	vq->it_pool = rte_malloc_socket(NULL,
@@ -1676,7 +1653,7 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 		VHOST_LOG_CONFIG(ERR,
 			"async register failed: cannot allocate memory for it_pool "
 			"(vid %d, qid: %d)\n", vid, queue_id);
-		goto reg_out;
+		return -1;
 	}
 
 	vq->vec_pool = rte_malloc_socket(NULL,
@@ -1687,7 +1664,7 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 		VHOST_LOG_CONFIG(ERR,
 			"async register failed: cannot allocate memory for vec_pool "
 			"(vid %d, qid: %d)\n", vid, queue_id);
-		goto reg_out;
+		return -1;
 	}
 
 	if (vq_is_packed(dev)) {
@@ -1699,7 +1676,7 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 			VHOST_LOG_CONFIG(ERR,
 				"async register failed: cannot allocate memory for async buffers "
 				"(vid %d, qid: %d)\n", vid, queue_id);
-			goto reg_out;
+			return -1;
 		}
 	} else {
 		vq->async_descs_split = rte_malloc_socket(NULL,
@@ -1710,7 +1687,7 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 			VHOST_LOG_CONFIG(ERR,
 				"async register failed: cannot allocate memory for async descs "
 				"(vid %d, qid: %d)\n", vid, queue_id);
-			goto reg_out;
+			return -1;
 		}
 	}
 
@@ -1722,10 +1699,76 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 
 	vq->async_registered = true;
 
-reg_out:
+	return 0;
+}
+
+int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
+		struct rte_vhost_async_config config,
+		struct rte_vhost_async_channel_ops *ops)
+{
+	struct vhost_virtqueue *vq;
+	struct virtio_net *dev = get_device(vid);
+	int ret;
+
+	if (dev == NULL || ops == NULL)
+		return -1;
+
+	if (queue_id >= VHOST_MAX_VRING)
+		return -1;
+
+	vq = dev->virtqueue[queue_id];
+
+	if (unlikely(vq == NULL || !dev->async_copy))
+		return -1;
+
+	if (unlikely(!(config.features & RTE_VHOST_ASYNC_INORDER))) {
+		VHOST_LOG_CONFIG(ERR,
+			"async copy is not supported on non-inorder mode "
+			"(vid %d, qid: %d)\n", vid, queue_id);
+		return -1;
+	}
+
+	if (unlikely(ops->check_completed_copies == NULL ||
+		ops->transfer_data == NULL))
+		return -1;
+
+	rte_spinlock_lock(&vq->access_lock);
+	ret = async_channel_register(vid, queue_id, config, ops);
 	rte_spinlock_unlock(&vq->access_lock);
 
-	return 0;
+	return ret;
+}
+
+int rte_vhost_async_channel_register_thread_unsafe(int vid, uint16_t queue_id,
+		struct rte_vhost_async_config config,
+		struct rte_vhost_async_channel_ops *ops)
+{
+	struct vhost_virtqueue *vq;
+	struct virtio_net *dev = get_device(vid);
+
+	if (dev == NULL || ops == NULL)
+		return -1;
+
+	if (queue_id >= VHOST_MAX_VRING)
+		return -1;
+
+	vq = dev->virtqueue[queue_id];
+
+	if (unlikely(vq == NULL || !dev->async_copy))
+		return -1;
+
+	if (unlikely(!(config.features & RTE_VHOST_ASYNC_INORDER))) {
+		VHOST_LOG_CONFIG(ERR,
+			"async copy is not supported on non-inorder mode "
+			"(vid %d, qid: %d)\n", vid, queue_id);
+		return -1;
+	}
+
+	if (unlikely(ops->check_completed_copies == NULL ||
+		ops->transfer_data == NULL))
+		return -1;
+
+	return async_channel_register(vid, queue_id, config, ops);
 }
 
 int rte_vhost_async_channel_unregister(int vid, uint16_t queue_id)
@@ -1775,5 +1818,40 @@ int rte_vhost_async_channel_unregister(int vid, uint16_t queue_id)
 	return ret;
 }
 
+int rte_vhost_async_channel_unregister_thread_unsafe(int vid,
+		uint16_t queue_id)
+{
+	struct vhost_virtqueue *vq;
+	struct virtio_net *dev = get_device(vid);
+
+	if (dev == NULL)
+		return -1;
+
+	if (queue_id >= VHOST_MAX_VRING)
+		return -1;
+
+	vq = dev->virtqueue[queue_id];
+
+	if (vq == NULL)
+		return -1;
+
+	if (!vq->async_registered)
+		return 0;
+
+	if (vq->async_pkts_inflight_n) {
+		VHOST_LOG_CONFIG(ERR, "Failed to unregister async channel. "
+			"async inflight packets must be completed before unregistration.\n");
+		return -1;
+	}
+
+	vhost_free_async_mem(vq);
+
+	vq->async_ops.transfer_data = NULL;
+	vq->async_ops.check_completed_copies = NULL;
+	vq->async_registered = false;
+
+	return 0;
+}
+
 RTE_LOG_REGISTER_SUFFIX(vhost_config_log_level, config, INFO);
 RTE_LOG_REGISTER_SUFFIX(vhost_data_log_level, data, WARNING);
-- 
2.7.4

Re: [dpdk-dev] [PATCH v4 2/3] vhost: rework async configuration struct

[Xia, Chenbo]

Hi Jiayu,

> -----Original Message-----
> From: Hu, Jiayu <jiayu.hu@intel.com>
> Sent: Tuesday, July 13, 2021 3:46 PM
> To: dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>; Hu, Jiayu
> <jiayu.hu@intel.com>
> Subject: [PATCH v4 2/3] vhost: rework async configuration struct

Struct -> structure 

> 
> This patch reworks the async configuration structure to improve code
> readability. In addition, add preserved padding fields on the structure
> for future usage.
> 
> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> ---
>  doc/guides/prog_guide/vhost_lib.rst | 19 +++++++++++--------
>  examples/vhost/main.c               |  8 ++++----
>  lib/vhost/rte_vhost_async.h         | 35 +++++++++++++++++------------------
>  lib/vhost/vhost.c                   | 13 +++++--------
>  4 files changed, 37 insertions(+), 38 deletions(-)
> 
> diff --git a/doc/guides/prog_guide/vhost_lib.rst
> b/doc/guides/prog_guide/vhost_lib.rst
> index d18fb98..affdc57 100644
> --- a/doc/guides/prog_guide/vhost_lib.rst
> +++ b/doc/guides/prog_guide/vhost_lib.rst
> @@ -218,26 +218,29 @@ The following is an overview of some key Vhost API
> functions:
> 
>    Enable or disable zero copy feature of the vhost crypto backend.
> 
> -* ``rte_vhost_async_channel_register(vid, queue_id, features, ops)``
> +* ``rte_vhost_async_channel_register(vid, queue_id, config, ops)``
> 
>    Register a vhost queue with async copy device channel after vring

Not related to this patch. But maybe 'Register an async copy device channel
for a vhost queue' is better?

> -  is enabled. Following device ``features`` must be specified together
> +  is enabled. Following device ``config`` must be specified together
>    with the registration:
> 
> -  * ``async_inorder``
> +  * ``features``
> 
> -    Async copy device can guarantee the ordering of copy completion
> -    sequence. Copies are completed in the same order with that at
> -    the submission time.
> +    This field is used to specify async copy device features.
> 
> -    Currently, only ``async_inorder`` capable device is supported by vhost.
> +    ``RTE_VHOST_ASYNC_INORDER`` represents the async copy device can
> +    guarantee the order of copy completion is the same as the order
> +    of copy submission.
> +
> +    Currently, only ``RTE_VHOST_ASYNC_INORDER`` capable device is
> +    supported by vhost.
> 
>    * ``async_threshold``
> 
>      The copy length (in bytes) below which CPU copy will be used even if
>      applications call async vhost APIs to enqueue/dequeue data.
> 
> -    Typical value is 512~1024 depending on the async device capability.
> +    Typical value is 256~1024 depending on the async device capability.
> 
>    Applications must provide following ``ops`` callbacks for vhost lib to
>    work with the async copy devices:
> diff --git a/examples/vhost/main.c b/examples/vhost/main.c
> index d2179ea..9cd855a 100644
> --- a/examples/vhost/main.c
> +++ b/examples/vhost/main.c
> @@ -1468,7 +1468,7 @@ new_device(int vid)
>  		vid, vdev->coreid);
> 
>  	if (async_vhost_driver) {
> -		struct rte_vhost_async_features f;
> +		struct rte_vhost_async_config config = {0};
>  		struct rte_vhost_async_channel_ops channel_ops;
> 
>  		if (dma_type != NULL && strncmp(dma_type, "ioat", 4) == 0) {
> @@ -1476,11 +1476,11 @@ new_device(int vid)
>  			channel_ops.check_completed_copies =
>  				ioat_check_completed_copies_cb;
> 
> -			f.async_inorder = 1;
> -			f.async_threshold = 256;
> +			config.features = RTE_VHOST_ASYNC_INORDER;
> +			config.async_threshold = 256;

This is ok as for now we are using ioat API. But I guess there should be
a more user-friendly way to know the value. I mean, no users want to do
complicated tests for his platform to know the value. Maybe there could be
some auto-test in specific driver to show the value to user?

It's off-topic for this patch. But thinking about it should be good.

> 
>  			return rte_vhost_async_channel_register(vid, VIRTIO_RXQ,
> -				f.intval, &channel_ops);
> +				config, &channel_ops);
>  		}
>  	}
> 
> diff --git a/lib/vhost/rte_vhost_async.h b/lib/vhost/rte_vhost_async.h
> index 6faa31f..c93490d 100644
> --- a/lib/vhost/rte_vhost_async.h
> +++ b/lib/vhost/rte_vhost_async.h
> @@ -93,18 +93,20 @@ struct async_inflight_info {
>  };
> 
>  /**
> - *  dma channel feature bit definition
> + *  dma channel features

Let's use 'async channel' which is also the name for API.
It's always good to use one term for one thing.

>   */
> -struct rte_vhost_async_features {
> -	union {
> -		uint32_t intval;
> -		struct {
> -			uint32_t async_inorder:1;
> -			uint32_t resvd_0:15;
> -			uint32_t async_threshold:12;
> -			uint32_t resvd_1:4;
> -		};
> -	};
> +enum {
> +	RTE_VHOST_ASYNC_FEATURE_UNKNOWN = 0U,
> +	RTE_VHOST_ASYNC_INORDER = 1U << 0,
> +};
> +
> +/**
> + *  dma channel configuration

Ditto

> + */
> +struct rte_vhost_async_config {
> +	uint32_t async_threshold;
> +	uint32_t features;
> +	uint32_t resvd[2];

Generally we use 'rsvd' for the word 'reserved'.

>  };
> 
>  /**
> @@ -114,12 +116,8 @@ struct rte_vhost_async_features {
>   *  vhost device id async channel to be attached to
>   * @param queue_id
>   *  vhost queue id async channel to be attached to
> - * @param features
> - *  DMA channel feature bit
> - *    b0       : DMA supports inorder data transfer
> - *    b1  - b15: reserved
> - *    b16 - b27: Packet length threshold for DMA transfer
> - *    b28 - b31: reserved
> + * @param config
> + *  DMA channel configuration structure

DMA -> async

>   * @param ops
>   *  DMA operation callbacks
>   * @return
> @@ -127,7 +125,8 @@ struct rte_vhost_async_features {
>   */
>  __rte_experimental
>  int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
> -	uint32_t features, struct rte_vhost_async_channel_ops *ops);
> +	struct rte_vhost_async_config config,
> +	struct rte_vhost_async_channel_ops *ops);
> 
>  /**
>   * unregister a dma channel for vhost
> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> index 53a470f..a20f05a 100644
> --- a/lib/vhost/vhost.c
> +++ b/lib/vhost/vhost.c
> @@ -1620,18 +1620,15 @@ int rte_vhost_extern_callback_register(int vid,
>  }
> 
>  int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
> -					uint32_t features,
> -					struct rte_vhost_async_channel_ops *ops)
> +		struct rte_vhost_async_config config,
> +		struct rte_vhost_async_channel_ops *ops)
>  {
>  	struct vhost_virtqueue *vq;
>  	struct virtio_net *dev = get_device(vid);
> -	struct rte_vhost_async_features f;
> 
>  	if (dev == NULL || ops == NULL)
>  		return -1;
> 
> -	f.intval = features;
> -
>  	if (queue_id >= VHOST_MAX_VRING)
>  		return -1;
> 
> @@ -1640,7 +1637,7 @@ int rte_vhost_async_channel_register(int vid, uint16_t
> queue_id,
>  	if (unlikely(vq == NULL || !dev->async_copy))
>  		return -1;
> 
> -	if (unlikely(!f.async_inorder)) {
> +	if (unlikely(!(config.features & RTE_VHOST_ASYNC_INORDER))) {
>  		VHOST_LOG_CONFIG(ERR,
>  			"async copy is not supported on non-inorder mode "
>  			"(vid %d, qid: %d)\n", vid, queue_id);
> @@ -1720,8 +1717,8 @@ int rte_vhost_async_channel_register(int vid, uint16_t
> queue_id,
>  	vq->async_ops.check_completed_copies = ops->check_completed_copies;
>  	vq->async_ops.transfer_data = ops->transfer_data;
> 
> -	vq->async_inorder = f.async_inorder;
> -	vq->async_threshold = f.async_threshold;
> +	vq->async_inorder = true;

Do we still need this? It's never used.

> +	vq->async_threshold = config.async_threshold;

vq->async_threshold is uint16_t and config.async_threshold is uint32_t.
They should be the same.

Thanks,
Chenbo

> 
>  	vq->async_registered = true;
> 
> --
> 2.7.4

Re: [dpdk-dev] [PATCH v4 2/3] vhost: rework async configuration struct

[Hu, Jiayu]

Thanks Chenbo for your comments.

Replies are inline.

> -----Original Message-----
> From: Xia, Chenbo <chenbo.xia@intel.com>
> Sent: Friday, July 16, 2021 2:03 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
> Cc: maxime.coquelin@redhat.com
> Subject: RE: [PATCH v4 2/3] vhost: rework async configuration struct
> 
> Hi Jiayu,
> 
> > -----Original Message-----
> > From: Hu, Jiayu <jiayu.hu@intel.com>
> > Sent: Tuesday, July 13, 2021 3:46 PM
> > To: dev@dpdk.org
> > Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>;
> > Hu, Jiayu <jiayu.hu@intel.com>
> > Subject: [PATCH v4 2/3] vhost: rework async configuration struct
> 
> Struct -> structure
> 
> >
> > This patch reworks the async configuration structure to improve code
> > readability. In addition, add preserved padding fields on the
> > structure for future usage.
> >
> > Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> > ---
> >  doc/guides/prog_guide/vhost_lib.rst | 19 +++++++++++--------
> >  examples/vhost/main.c               |  8 ++++----
> >  lib/vhost/rte_vhost_async.h         | 35 +++++++++++++++++------------------
> >  lib/vhost/vhost.c                   | 13 +++++--------
> >  4 files changed, 37 insertions(+), 38 deletions(-)
> >
> > diff --git a/doc/guides/prog_guide/vhost_lib.rst
> > b/doc/guides/prog_guide/vhost_lib.rst
> > index d18fb98..affdc57 100644
> > --- a/doc/guides/prog_guide/vhost_lib.rst
> > +++ b/doc/guides/prog_guide/vhost_lib.rst
> > @@ -218,26 +218,29 @@ The following is an overview of some key Vhost
> > API
> > functions:
> >
> >    Enable or disable zero copy feature of the vhost crypto backend.
> >
> > -* ``rte_vhost_async_channel_register(vid, queue_id, features, ops)``
> > +* ``rte_vhost_async_channel_register(vid, queue_id, config, ops)``
> >
> >    Register a vhost queue with async copy device channel after vring
> 
> Not related to this patch. But maybe 'Register an async copy device channel
> for a vhost queue' is better?
> 
> > -  is enabled. Following device ``features`` must be specified
> > together
> > +  is enabled. Following device ``config`` must be specified together
> >    with the registration:
> >
> > -  * ``async_inorder``
> > +  * ``features``
> >
> > -    Async copy device can guarantee the ordering of copy completion
> > -    sequence. Copies are completed in the same order with that at
> > -    the submission time.
> > +    This field is used to specify async copy device features.
> >
> > -    Currently, only ``async_inorder`` capable device is supported by vhost.
> > +    ``RTE_VHOST_ASYNC_INORDER`` represents the async copy device can
> > +    guarantee the order of copy completion is the same as the order
> > +    of copy submission.
> > +
> > +    Currently, only ``RTE_VHOST_ASYNC_INORDER`` capable device is
> > +    supported by vhost.
> >
> >    * ``async_threshold``
> >
> >      The copy length (in bytes) below which CPU copy will be used even if
> >      applications call async vhost APIs to enqueue/dequeue data.
> >
> > -    Typical value is 512~1024 depending on the async device capability.
> > +    Typical value is 256~1024 depending on the async device capability.
> >
> >    Applications must provide following ``ops`` callbacks for vhost lib to
> >    work with the async copy devices:
> > diff --git a/examples/vhost/main.c b/examples/vhost/main.c index
> > d2179ea..9cd855a 100644
> > --- a/examples/vhost/main.c
> > +++ b/examples/vhost/main.c
> > @@ -1468,7 +1468,7 @@ new_device(int vid)
> >  		vid, vdev->coreid);
> >
> >  	if (async_vhost_driver) {
> > -		struct rte_vhost_async_features f;
> > +		struct rte_vhost_async_config config = {0};
> >  		struct rte_vhost_async_channel_ops channel_ops;
> >
> >  		if (dma_type != NULL && strncmp(dma_type, "ioat", 4) == 0)
> { @@
> > -1476,11 +1476,11 @@ new_device(int vid)
> >  			channel_ops.check_completed_copies =
> >  				ioat_check_completed_copies_cb;
> >
> > -			f.async_inorder = 1;
> > -			f.async_threshold = 256;
> > +			config.features = RTE_VHOST_ASYNC_INORDER;
> > +			config.async_threshold = 256;
> 
> This is ok as for now we are using ioat API. But I guess there should be a
> more user-friendly way to know the value. I mean, no users want to do
> complicated tests for his platform to know the value. Maybe there could be
> some auto-test in specific driver to show the value to user?

It's hard in a way, as it's not only related to platform, but also application logics.
So I think better to keep it as an input from users, before we have a good idea 😊

> 
> It's off-topic for this patch. But thinking about it should be good.
> 
> >
> >  			return rte_vhost_async_channel_register(vid,
> VIRTIO_RXQ,
> > -				f.intval, &channel_ops);
> > +				config, &channel_ops);
> >  		}
> >  	}
> >
> > diff --git a/lib/vhost/rte_vhost_async.h b/lib/vhost/rte_vhost_async.h
> > index 6faa31f..c93490d 100644
> > --- a/lib/vhost/rte_vhost_async.h
> > +++ b/lib/vhost/rte_vhost_async.h
> > @@ -93,18 +93,20 @@ struct async_inflight_info {  };
> >
> >  /**
> > - *  dma channel feature bit definition
> > + *  dma channel features
> 
> Let's use 'async channel' which is also the name for API.
> It's always good to use one term for one thing.

Sure, I will fix that.

> 
> >   */
> > -struct rte_vhost_async_features {
> > -	union {
> > -		uint32_t intval;
> > -		struct {
> > -			uint32_t async_inorder:1;
> > -			uint32_t resvd_0:15;
> > -			uint32_t async_threshold:12;
> > -			uint32_t resvd_1:4;
> > -		};
> > -	};
> > +enum {
> > +	RTE_VHOST_ASYNC_FEATURE_UNKNOWN = 0U,
> > +	RTE_VHOST_ASYNC_INORDER = 1U << 0,
> > +};
> > +
> > +/**
> > + *  dma channel configuration
> 
> Ditto
> 
> > + */
> > +struct rte_vhost_async_config {
> > +	uint32_t async_threshold;
> > +	uint32_t features;
> > +	uint32_t resvd[2];
> 
> Generally we use 'rsvd' for the word 'reserved'.
> 
> >  };
> >
> >  /**
> > @@ -114,12 +116,8 @@ struct rte_vhost_async_features {
> >   *  vhost device id async channel to be attached to
> >   * @param queue_id
> >   *  vhost queue id async channel to be attached to
> > - * @param features
> > - *  DMA channel feature bit
> > - *    b0       : DMA supports inorder data transfer
> > - *    b1  - b15: reserved
> > - *    b16 - b27: Packet length threshold for DMA transfer
> > - *    b28 - b31: reserved
> > + * @param config
> > + *  DMA channel configuration structure
> 
> DMA -> async
> 
> >   * @param ops
> >   *  DMA operation callbacks
> >   * @return
> > @@ -127,7 +125,8 @@ struct rte_vhost_async_features {
> >   */
> >  __rte_experimental
> >  int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
> > -	uint32_t features, struct rte_vhost_async_channel_ops *ops);
> > +	struct rte_vhost_async_config config,
> > +	struct rte_vhost_async_channel_ops *ops);
> >
> >  /**
> >   * unregister a dma channel for vhost diff --git a/lib/vhost/vhost.c
> > b/lib/vhost/vhost.c index 53a470f..a20f05a 100644
> > --- a/lib/vhost/vhost.c
> > +++ b/lib/vhost/vhost.c
> > @@ -1620,18 +1620,15 @@ int rte_vhost_extern_callback_register(int
> > vid,  }
> >
> >  int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
> > -					uint32_t features,
> > -					struct rte_vhost_async_channel_ops
> *ops)
> > +		struct rte_vhost_async_config config,
> > +		struct rte_vhost_async_channel_ops *ops)
> >  {
> >  	struct vhost_virtqueue *vq;
> >  	struct virtio_net *dev = get_device(vid);
> > -	struct rte_vhost_async_features f;
> >
> >  	if (dev == NULL || ops == NULL)
> >  		return -1;
> >
> > -	f.intval = features;
> > -
> >  	if (queue_id >= VHOST_MAX_VRING)
> >  		return -1;
> >
> > @@ -1640,7 +1637,7 @@ int rte_vhost_async_channel_register(int vid,
> > uint16_t queue_id,
> >  	if (unlikely(vq == NULL || !dev->async_copy))
> >  		return -1;
> >
> > -	if (unlikely(!f.async_inorder)) {
> > +	if (unlikely(!(config.features & RTE_VHOST_ASYNC_INORDER))) {
> >  		VHOST_LOG_CONFIG(ERR,
> >  			"async copy is not supported on non-inorder mode "
> >  			"(vid %d, qid: %d)\n", vid, queue_id); @@ -1720,8
> +1717,8 @@ int
> > rte_vhost_async_channel_register(int vid, uint16_t queue_id,
> >  	vq->async_ops.check_completed_copies = ops-
> >check_completed_copies;
> >  	vq->async_ops.transfer_data = ops->transfer_data;
> >
> > -	vq->async_inorder = f.async_inorder;
> > -	vq->async_threshold = f.async_threshold;
> > +	vq->async_inorder = true;
> 
> Do we still need this? It's never used.

I think we need to keep it, as we may support out-of-order channel in future.

> 
> > +	vq->async_threshold = config.async_threshold;
> 
> vq->async_threshold is uint16_t and config.async_threshold is uint32_t.
> They should be the same.

I will change vq->async_threshold to uint32_t.

Thanks,
Jiayu
> 
> Thanks,
> Chenbo
> 
> >
> >  	vq->async_registered = true;
> >
> > --
> > 2.7.4
> 

Re: [dpdk-dev] [PATCH v4 2/3] vhost: rework async configuration struct

[Xia, Chenbo]

Hi Jiayu,

> -----Original Message-----
> From: Hu, Jiayu <jiayu.hu@intel.com>
> Sent: Friday, July 16, 2021 2:18 PM
> To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org
> Cc: maxime.coquelin@redhat.com
> Subject: RE: [PATCH v4 2/3] vhost: rework async configuration struct
> 
> Thanks Chenbo for your comments.
> 
> Replies are inline.
> 
> > -----Original Message-----
> > From: Xia, Chenbo <chenbo.xia@intel.com>
> > Sent: Friday, July 16, 2021 2:03 PM
> > To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
> > Cc: maxime.coquelin@redhat.com
> > Subject: RE: [PATCH v4 2/3] vhost: rework async configuration struct
> >
> > Hi Jiayu,
> >
> > > -----Original Message-----
> > > From: Hu, Jiayu <jiayu.hu@intel.com>
> > > Sent: Tuesday, July 13, 2021 3:46 PM
> > > To: dev@dpdk.org
> > > Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>;
> > > Hu, Jiayu <jiayu.hu@intel.com>
> > > Subject: [PATCH v4 2/3] vhost: rework async configuration struct
> >
> > Struct -> structure
> >
> > >
> > > This patch reworks the async configuration structure to improve code
> > > readability. In addition, add preserved padding fields on the
> > > structure for future usage.
> > >
> > > Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> > > ---
> > >  doc/guides/prog_guide/vhost_lib.rst | 19 +++++++++++--------
> > >  examples/vhost/main.c               |  8 ++++----
> > >  lib/vhost/rte_vhost_async.h         | 35 +++++++++++++++++---------------
> ---
> > >  lib/vhost/vhost.c                   | 13 +++++--------
> > >  4 files changed, 37 insertions(+), 38 deletions(-)
> > >

[snip]

> > > -vq->async_inorder = f.async_inorder;
> > > -vq->async_threshold = f.async_threshold;
> > > +vq->async_inorder = true;
> >
> > Do we still need this? It's never used.
> 
> I think we need to keep it, as we may support out-of-order channel in future.

We don't like to define things when it's not needed currently. If in future,
the definition should also be made in future :P. Just leave the definition
to future patch.

Thanks,
Chenbo

> 
> >
> > > +vq->async_threshold = config.async_threshold;
> >
> > vq->async_threshold is uint16_t and config.async_threshold is uint32_t.
> > They should be the same.
> 
> I will change vq->async_threshold to uint32_t.
> 
> Thanks,
> Jiayu
> >
> > Thanks,
> > Chenbo
> >
> > >
> > >  vq->async_registered = true;
> > >
> > > --
> > > 2.7.4
> >
> 

Re: [dpdk-dev] [PATCH v4 2/3] vhost: rework async configuration struct

[Hu, Jiayu]

> -----Original Message-----
> From: Xia, Chenbo <chenbo.xia@intel.com>
> Sent: Friday, July 16, 2021 2:28 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
> Cc: maxime.coquelin@redhat.com
> Subject: RE: [PATCH v4 2/3] vhost: rework async configuration struct
> 
> 
> > > > -vq->async_inorder = f.async_inorder; async_threshold =
> > > > -vq->f.async_threshold;
> > > > +vq->async_inorder = true;
> > >
> > > Do we still need this? It's never used.
> >
> > I think we need to keep it, as we may support out-of-order channel in
> future.
> 
> We don't like to define things when it's not needed currently. If in future, the
> definition should also be made in future :P. Just leave the definition to future
> patch.

Fair. I will delete vq->async_inorder field.

Thanks,
Jiayu
> 
> Thanks,
> Chenbo
> 
> >
> > >
> > > > +vq->async_threshold = config.async_threshold;
> > >
> > > vq->async_threshold is uint16_t and config.async_threshold is uint32_t.
> > > They should be the same.
> >
> > I will change vq->async_threshold to uint32_t.
> >
> > Thanks,
> > Jiayu
> > >
> > > Thanks,
> > > Chenbo
> > >
> > > >
> > > >  vq->async_registered = true;
> > > >
> > > > --
> > > > 2.7.4
> > >
> >
> 

Re: [dpdk-dev] [PATCH v4 3/3] vhost: add thread unsafe async registeration functions

[Xia, Chenbo]

Hi Jiayu,

> -----Original Message-----
> From: Hu, Jiayu <jiayu.hu@intel.com>
> Sent: Tuesday, July 13, 2021 3:46 PM
> To: dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>; Hu, Jiayu
> <jiayu.hu@intel.com>
> Subject: [PATCH v4 3/3] vhost: add thread unsafe async registeration functions
> 
> This patch adds thread unsafe version for async register and
> unregister functions.
> 
> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> ---
>  doc/guides/prog_guide/vhost_lib.rst |  12 +++
>  lib/vhost/rte_vhost_async.h         |  39 ++++++++++
>  lib/vhost/version.map               |   4 +
>  lib/vhost/vhost.c                   | 146 +++++++++++++++++++++++++++--------
> -
>  4 files changed, 167 insertions(+), 34 deletions(-)
> 
> diff --git a/doc/guides/prog_guide/vhost_lib.rst
> b/doc/guides/prog_guide/vhost_lib.rst
> index affdc57..ffe09a3 100644
> --- a/doc/guides/prog_guide/vhost_lib.rst
> +++ b/doc/guides/prog_guide/vhost_lib.rst
> @@ -256,6 +256,12 @@ The following is an overview of some key Vhost API
> functions:
>      vhost invokes this function to get the copy data completed by async
>      devices.
> 
> +* ``rte_vhost_async_channel_register_thread_unsafe(vid, queue_id, config,
> ops)``
> +  Register a vhost queue with async copy device channel without

'Register an async copy device channel for a vhost queue' sounds better 

> +  performing any locking.
> +
> +  This function is only safe to call from within vhost callback functions.

call in vhost callback functions?

> +
>  * ``rte_vhost_async_channel_unregister(vid, queue_id)``
> 
>    Unregister the async copy device channel from a vhost queue.
> @@ -268,6 +274,12 @@ The following is an overview of some key Vhost API
> functions:
>    devices for all vhost queues in destroy_device(), when a
>    virtio device is paused or shut down.
> 
> +* ``rte_vhost_async_channel_unregister_thread_unsafe(vid, queue_id)``
> +  Unregister the async copy device channel from a vhost queue without

for a vhost queue?

> +  performing any locking.
> +
> +  This function is only safe to call from within vhost callback functions.

Call in vhost callback functions?

And let's make it clearer. Add this: (i.e., struct vhost_device_ops)

> +
>  * ``rte_vhost_submit_enqueue_burst(vid, queue_id, pkts, count, comp_pkts,
> comp_count)``
> 
>    Submit an enqueue request to transmit ``count`` packets from host to guest
> diff --git a/lib/vhost/rte_vhost_async.h b/lib/vhost/rte_vhost_async.h
> index c93490d..e4d20a3 100644
> --- a/lib/vhost/rte_vhost_async.h
> +++ b/lib/vhost/rte_vhost_async.h
> @@ -142,6 +142,45 @@ __rte_experimental
>  int rte_vhost_async_channel_unregister(int vid, uint16_t queue_id);
> 
>  /**
> + * register an async channel for vhost without performing any locking

register -> Register

for vhost -> for a vhost queue

> + *
> + * @note This function does not perform any locking, and is only safe to call
> + *       from within vhost callback functions.

From within -> in ?

> + *
> + * @param vid
> + *  vhost device id async channel to be attached to
> + * @param queue_id
> + *  vhost queue id async channel to be attached to
> + * @param config
> + *  DMA channel configuration

DMA -> async?

> + * @param ops
> + *  DMA operation callbacks

Async channel operation callbacks?

> + * @return
> + *  0 on success, -1 on failures
> + */
> +__rte_experimental
> +int rte_vhost_async_channel_register_thread_unsafe(int vid, uint16_t queue_id,

According to DPDK coding style. Return value should not be in the same line
of the function name. Just make a new line for it. Please check other functions like
unregister.

> +	struct rte_vhost_async_config config,
> +	struct rte_vhost_async_channel_ops *ops);
> +
> +/**
> + * unregister a dma channel for vhost without performing any lock

Unregister an async channel for a vhost queue

> + *
> + * @note This function does not perform any locking, and is only safe to call
> + *       from within vhost callback functions.

in vhost callback functions

> + *
> + * @param vid
> + *  vhost device id DMA channel to be detached

Vhost device id Async channel to be detached from

> + * @param queue_id
> + *  vhost queue id DMA channel to be detached

Ditto

> + * @return
> + *  0 on success, -1 on failures
> + */
> --
> 2.7.4

[dpdk-dev] [PATCH v5 0/3] provide thread unsafe async registration functions

[Jiayu Hu]

Lock protection is needed during the vhost notifies the application of
device readiness, so the first patch adds lock protection. In addition,
the second patch reworks async feature structure to improve readability.
After performing locking, existed async vhost registration functions will
cause deadlock, as they acquire lock too. The last patch provides thread
unsafe registration functions to support calling within vhost callback
functions.

v5:
* improve coding style
* update commit log, doc and comments
* remove useless field async_inorder
* change async_threshold from uint16_t to uint32_t
v4:
* remove brace {} in single statement block
v3:
* rename and use enum to define async device features
* change padding fields to 8 bytes
v2:
* rework async feature structure
* fix typo in commit log

Jiayu Hu (3):
  vhost: fix lock on device readiness notification
  vhost: rework async configuration structure
  vhost: add thread unsafe async registeration functions

 doc/guides/prog_guide/vhost_lib.rst |  35 ++++++--
 examples/vhost/main.c               |   8 +-
 lib/vhost/rte_vhost_async.h         |  86 ++++++++++++++-----
 lib/vhost/version.map               |   4 +
 lib/vhost/vhost.c                   | 162 ++++++++++++++++++++++++++----------
 lib/vhost/vhost.h                   |   3 +-
 lib/vhost/vhost_user.c              |   5 +-
 7 files changed, 219 insertions(+), 84 deletions(-)

-- 
2.7.4

[dpdk-dev] [PATCH v5 1/3] vhost: fix lock on device readiness notification

[Jiayu Hu]

The vhost notifies the application of device readiness via
vhost_user_notify_queue_state(), but calling this function
is not protected by the lock. This patch is to make this
function call lock protected.

Fixes: d0fcc38f5fa4 ("vhost: improve device readiness notifications")
Cc: stable@dpdk.org

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/vhost/vhost_user.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
index 031c578..31300e1 100644
--- a/lib/vhost/vhost_user.c
+++ b/lib/vhost/vhost_user.c
@@ -2995,9 +2995,6 @@ vhost_user_msg_handler(int vid, int fd)
 		}
 	}
 
-	if (unlock_required)
-		vhost_user_unlock_all_queue_pairs(dev);
-
 	/* If message was not handled at this stage, treat it as an error */
 	if (!handled) {
 		VHOST_LOG_CONFIG(ERR,
@@ -3032,6 +3029,8 @@ vhost_user_msg_handler(int vid, int fd)
 		}
 	}
 
+	if (unlock_required)
+		vhost_user_unlock_all_queue_pairs(dev);
 
 	if (!virtio_is_ready(dev))
 		goto out;
-- 
2.7.4

[dpdk-dev] [PATCH v5 2/3] vhost: rework async configuration structure

[Jiayu Hu]

This patch reworks the async configuration structure to improve code
readability. In addition, add preserved padding fields on the structure
for future usage.

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
---
 doc/guides/prog_guide/vhost_lib.rst | 21 +++++++++--------
 examples/vhost/main.c               |  8 +++----
 lib/vhost/rte_vhost_async.h         | 45 ++++++++++++++++++-------------------
 lib/vhost/vhost.c                   | 19 +++++++---------
 lib/vhost/vhost.h                   |  3 +--
 5 files changed, 47 insertions(+), 49 deletions(-)

diff --git a/doc/guides/prog_guide/vhost_lib.rst b/doc/guides/prog_guide/vhost_lib.rst
index d18fb98..2a61b85 100644
--- a/doc/guides/prog_guide/vhost_lib.rst
+++ b/doc/guides/prog_guide/vhost_lib.rst
@@ -218,26 +218,29 @@ The following is an overview of some key Vhost API functions:
 
   Enable or disable zero copy feature of the vhost crypto backend.
 
-* ``rte_vhost_async_channel_register(vid, queue_id, features, ops)``
+* ``rte_vhost_async_channel_register(vid, queue_id, config, ops)``
 
-  Register a vhost queue with async copy device channel after vring
-  is enabled. Following device ``features`` must be specified together
+  Register an async copy device channel for a vhost queue after vring
+  is enabled. Following device ``config`` must be specified together
   with the registration:
 
-  * ``async_inorder``
+  * ``features``
 
-    Async copy device can guarantee the ordering of copy completion
-    sequence. Copies are completed in the same order with that at
-    the submission time.
+    This field is used to specify async copy device features.
 
-    Currently, only ``async_inorder`` capable device is supported by vhost.
+    ``RTE_VHOST_ASYNC_INORDER`` represents the async copy device can
+    guarantee the order of copy completion is the same as the order
+    of copy submission.
+
+    Currently, only ``RTE_VHOST_ASYNC_INORDER`` capable device is
+    supported by vhost.
 
   * ``async_threshold``
 
     The copy length (in bytes) below which CPU copy will be used even if
     applications call async vhost APIs to enqueue/dequeue data.
 
-    Typical value is 512~1024 depending on the async device capability.
+    Typical value is 256~1024 depending on the async device capability.
 
   Applications must provide following ``ops`` callbacks for vhost lib to
   work with the async copy devices:
diff --git a/examples/vhost/main.c b/examples/vhost/main.c
index d2179ea..9cd855a 100644
--- a/examples/vhost/main.c
+++ b/examples/vhost/main.c
@@ -1468,7 +1468,7 @@ new_device(int vid)
 		vid, vdev->coreid);
 
 	if (async_vhost_driver) {
-		struct rte_vhost_async_features f;
+		struct rte_vhost_async_config config = {0};
 		struct rte_vhost_async_channel_ops channel_ops;
 
 		if (dma_type != NULL && strncmp(dma_type, "ioat", 4) == 0) {
@@ -1476,11 +1476,11 @@ new_device(int vid)
 			channel_ops.check_completed_copies =
 				ioat_check_completed_copies_cb;
 
-			f.async_inorder = 1;
-			f.async_threshold = 256;
+			config.features = RTE_VHOST_ASYNC_INORDER;
+			config.async_threshold = 256;
 
 			return rte_vhost_async_channel_register(vid, VIRTIO_RXQ,
-				f.intval, &channel_ops);
+				config, &channel_ops);
 		}
 	}
 
diff --git a/lib/vhost/rte_vhost_async.h b/lib/vhost/rte_vhost_async.h
index 6faa31f..52b1727 100644
--- a/lib/vhost/rte_vhost_async.h
+++ b/lib/vhost/rte_vhost_async.h
@@ -93,49 +93,48 @@ struct async_inflight_info {
 };
 
 /**
- *  dma channel feature bit definition
+ *  async channel features
  */
-struct rte_vhost_async_features {
-	union {
-		uint32_t intval;
-		struct {
-			uint32_t async_inorder:1;
-			uint32_t resvd_0:15;
-			uint32_t async_threshold:12;
-			uint32_t resvd_1:4;
-		};
-	};
+enum {
+	RTE_VHOST_ASYNC_FEATURE_UNKNOWN = 0U,
+	RTE_VHOST_ASYNC_INORDER = 1U << 0,
 };
 
 /**
- * register an async channel for vhost
+ *  async channel configuration
+ */
+struct rte_vhost_async_config {
+	uint32_t async_threshold;
+	uint32_t features;
+	uint32_t rsvd[2];
+};
+
+/**
+ * Register an async channel for a vhost queue
  *
  * @param vid
  *  vhost device id async channel to be attached to
  * @param queue_id
  *  vhost queue id async channel to be attached to
- * @param features
- *  DMA channel feature bit
- *    b0       : DMA supports inorder data transfer
- *    b1  - b15: reserved
- *    b16 - b27: Packet length threshold for DMA transfer
- *    b28 - b31: reserved
+ * @param config
+ *  Async channel configuration structure
  * @param ops
- *  DMA operation callbacks
+ *  Async channel operation callbacks
  * @return
  *  0 on success, -1 on failures
  */
 __rte_experimental
 int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
-	uint32_t features, struct rte_vhost_async_channel_ops *ops);
+	struct rte_vhost_async_config config,
+	struct rte_vhost_async_channel_ops *ops);
 
 /**
- * unregister a dma channel for vhost
+ * Unregister an async channel for a vhost queue
  *
  * @param vid
- *  vhost device id DMA channel to be detached
+ *  vhost device id async channel to be detached from
  * @param queue_id
- *  vhost queue id DMA channel to be detached
+ *  vhost queue id async channel to be detached from
  * @return
  *  0 on success, -1 on failures
  */
diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
index 53a470f..908758e 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -1619,19 +1619,17 @@ int rte_vhost_extern_callback_register(int vid,
 	return 0;
 }
 
-int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
-					uint32_t features,
-					struct rte_vhost_async_channel_ops *ops)
+int
+rte_vhost_async_channel_register(int vid, uint16_t queue_id,
+		struct rte_vhost_async_config config,
+		struct rte_vhost_async_channel_ops *ops)
 {
 	struct vhost_virtqueue *vq;
 	struct virtio_net *dev = get_device(vid);
-	struct rte_vhost_async_features f;
 
 	if (dev == NULL || ops == NULL)
 		return -1;
 
-	f.intval = features;
-
 	if (queue_id >= VHOST_MAX_VRING)
 		return -1;
 
@@ -1640,7 +1638,7 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 	if (unlikely(vq == NULL || !dev->async_copy))
 		return -1;
 
-	if (unlikely(!f.async_inorder)) {
+	if (unlikely(!(config.features & RTE_VHOST_ASYNC_INORDER))) {
 		VHOST_LOG_CONFIG(ERR,
 			"async copy is not supported on non-inorder mode "
 			"(vid %d, qid: %d)\n", vid, queue_id);
@@ -1719,9 +1717,7 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 
 	vq->async_ops.check_completed_copies = ops->check_completed_copies;
 	vq->async_ops.transfer_data = ops->transfer_data;
-
-	vq->async_inorder = f.async_inorder;
-	vq->async_threshold = f.async_threshold;
+	vq->async_threshold = config.async_threshold;
 
 	vq->async_registered = true;
 
@@ -1731,7 +1727,8 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 	return 0;
 }
 
-int rte_vhost_async_channel_unregister(int vid, uint16_t queue_id)
+int
+rte_vhost_async_channel_unregister(int vid, uint16_t queue_id)
 {
 	struct vhost_virtqueue *vq;
 	struct virtio_net *dev = get_device(vid);
diff --git a/lib/vhost/vhost.h b/lib/vhost/vhost.h
index 8ffe387..d98ca8a 100644
--- a/lib/vhost/vhost.h
+++ b/lib/vhost/vhost.h
@@ -218,9 +218,8 @@ struct vhost_virtqueue {
 	};
 
 	/* vq async features */
-	bool		async_inorder;
 	bool		async_registered;
-	uint16_t	async_threshold;
+	uint32_t	async_threshold;
 
 	int			notif_enable;
 #define VIRTIO_UNINITIALIZED_NOTIF	(-1)
-- 
2.7.4

[dpdk-dev] [PATCH v5 3/3] vhost: add thread unsafe async registeration functions

[Jiayu Hu]

This patch adds thread unsafe version for async register and
unregister functions.

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
---
 doc/guides/prog_guide/vhost_lib.rst |  14 ++++
 lib/vhost/rte_vhost_async.h         |  41 ++++++++++
 lib/vhost/version.map               |   4 +
 lib/vhost/vhost.c                   | 149 +++++++++++++++++++++++++++---------
 4 files changed, 173 insertions(+), 35 deletions(-)

diff --git a/doc/guides/prog_guide/vhost_lib.rst b/doc/guides/prog_guide/vhost_lib.rst
index 2a61b85..c8638db 100644
--- a/doc/guides/prog_guide/vhost_lib.rst
+++ b/doc/guides/prog_guide/vhost_lib.rst
@@ -256,6 +256,13 @@ The following is an overview of some key Vhost API functions:
     vhost invokes this function to get the copy data completed by async
     devices.
 
+* ``rte_vhost_async_channel_register_thread_unsafe(vid, queue_id, config, ops)``
+  Register an async copy device channel for a vhost queue without
+  performing any locking.
+
+  This function is only safe to call in vhost callback functions
+  (i.e., struct vhost_device_ops).
+
 * ``rte_vhost_async_channel_unregister(vid, queue_id)``
 
   Unregister the async copy device channel from a vhost queue.
@@ -268,6 +275,13 @@ The following is an overview of some key Vhost API functions:
   devices for all vhost queues in destroy_device(), when a
   virtio device is paused or shut down.
 
+* ``rte_vhost_async_channel_unregister_thread_unsafe(vid, queue_id)``
+  Unregister the async copy device channel for a vhost queue without
+  performing any locking.
+
+  This function is only safe to call in vhost callback functions
+  (i.e., struct vhost_device_ops).
+
 * ``rte_vhost_submit_enqueue_burst(vid, queue_id, pkts, count, comp_pkts, comp_count)``
 
   Submit an enqueue request to transmit ``count`` packets from host to guest
diff --git a/lib/vhost/rte_vhost_async.h b/lib/vhost/rte_vhost_async.h
index 52b1727..8f7fd35 100644
--- a/lib/vhost/rte_vhost_async.h
+++ b/lib/vhost/rte_vhost_async.h
@@ -142,6 +142,47 @@ __rte_experimental
 int rte_vhost_async_channel_unregister(int vid, uint16_t queue_id);
 
 /**
+ * Register an async channel for a vhost queue without performing any
+ * locking
+ *
+ * @note This function does not perform any locking, and is only safe to
+ *       call in vhost callback functions.
+ *
+ * @param vid
+ *  vhost device id async channel to be attached to
+ * @param queue_id
+ *  vhost queue id async channel to be attached to
+ * @param config
+ *  Async channel configuration
+ * @param ops
+ *  Async channel operation callbacks
+ * @return
+ *  0 on success, -1 on failures
+ */
+__rte_experimental
+int rte_vhost_async_channel_register_thread_unsafe(int vid, uint16_t queue_id,
+	struct rte_vhost_async_config config,
+	struct rte_vhost_async_channel_ops *ops);
+
+/**
+ * Unregister an async channel for a vhost queue without performing any
+ * locking
+ *
+ * @note This function does not perform any locking, and is only safe to
+ *       call in vhost callback functions.
+ *
+ * @param vid
+ *  vhost device id async channel to be detached from
+ * @param queue_id
+ *  vhost queue id async channel to be detached from
+ * @return
+ *  0 on success, -1 on failures
+ */
+__rte_experimental
+int rte_vhost_async_channel_unregister_thread_unsafe(int vid,
+		uint16_t queue_id);
+
+/**
  * This function submits enqueue data to async engine. Successfully
  * enqueued packets can be transfer completed or being occupied by DMA
  * engines, when this API returns. Transfer completed packets are returned
diff --git a/lib/vhost/version.map b/lib/vhost/version.map
index 9103a23..2363db8 100644
--- a/lib/vhost/version.map
+++ b/lib/vhost/version.map
@@ -79,4 +79,8 @@ EXPERIMENTAL {
 
 	# added in 21.05
 	rte_vhost_get_negotiated_protocol_features;
+
+	# added in 21.08
+	rte_vhost_async_channel_register_thread_unsafe;
+	rte_vhost_async_channel_unregister_thread_unsafe;
 };
diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
index 908758e..d3d0205 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -1619,43 +1619,19 @@ int rte_vhost_extern_callback_register(int vid,
 	return 0;
 }
 
-int
-rte_vhost_async_channel_register(int vid, uint16_t queue_id,
+static __rte_always_inline int
+async_channel_register(int vid, uint16_t queue_id,
 		struct rte_vhost_async_config config,
 		struct rte_vhost_async_channel_ops *ops)
 {
-	struct vhost_virtqueue *vq;
 	struct virtio_net *dev = get_device(vid);
-
-	if (dev == NULL || ops == NULL)
-		return -1;
-
-	if (queue_id >= VHOST_MAX_VRING)
-		return -1;
-
-	vq = dev->virtqueue[queue_id];
-
-	if (unlikely(vq == NULL || !dev->async_copy))
-		return -1;
-
-	if (unlikely(!(config.features & RTE_VHOST_ASYNC_INORDER))) {
-		VHOST_LOG_CONFIG(ERR,
-			"async copy is not supported on non-inorder mode "
-			"(vid %d, qid: %d)\n", vid, queue_id);
-		return -1;
-	}
-
-	if (unlikely(ops->check_completed_copies == NULL ||
-		ops->transfer_data == NULL))
-		return -1;
-
-	rte_spinlock_lock(&vq->access_lock);
+	struct vhost_virtqueue *vq = dev->virtqueue[queue_id];
 
 	if (unlikely(vq->async_registered)) {
 		VHOST_LOG_CONFIG(ERR,
 			"async register failed: channel already registered "
 			"(vid %d, qid: %d)\n", vid, queue_id);
-		goto reg_out;
+		return -1;
 	}
 
 	vq->async_pkts_info = rte_malloc_socket(NULL,
@@ -1666,7 +1642,7 @@ rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 		VHOST_LOG_CONFIG(ERR,
 			"async register failed: cannot allocate memory for async_pkts_info "
 			"(vid %d, qid: %d)\n", vid, queue_id);
-		goto reg_out;
+		return -1;
 	}
 
 	vq->it_pool = rte_malloc_socket(NULL,
@@ -1677,7 +1653,7 @@ rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 		VHOST_LOG_CONFIG(ERR,
 			"async register failed: cannot allocate memory for it_pool "
 			"(vid %d, qid: %d)\n", vid, queue_id);
-		goto reg_out;
+		return -1;
 	}
 
 	vq->vec_pool = rte_malloc_socket(NULL,
@@ -1688,7 +1664,7 @@ rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 		VHOST_LOG_CONFIG(ERR,
 			"async register failed: cannot allocate memory for vec_pool "
 			"(vid %d, qid: %d)\n", vid, queue_id);
-		goto reg_out;
+		return -1;
 	}
 
 	if (vq_is_packed(dev)) {
@@ -1700,7 +1676,7 @@ rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 			VHOST_LOG_CONFIG(ERR,
 				"async register failed: cannot allocate memory for async buffers "
 				"(vid %d, qid: %d)\n", vid, queue_id);
-			goto reg_out;
+			return -1;
 		}
 	} else {
 		vq->async_descs_split = rte_malloc_socket(NULL,
@@ -1711,7 +1687,7 @@ rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 			VHOST_LOG_CONFIG(ERR,
 				"async register failed: cannot allocate memory for async descs "
 				"(vid %d, qid: %d)\n", vid, queue_id);
-			goto reg_out;
+			return -1;
 		}
 	}
 
@@ -1721,10 +1697,78 @@ rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 
 	vq->async_registered = true;
 
-reg_out:
+	return 0;
+}
+
+int
+rte_vhost_async_channel_register(int vid, uint16_t queue_id,
+		struct rte_vhost_async_config config,
+		struct rte_vhost_async_channel_ops *ops)
+{
+	struct vhost_virtqueue *vq;
+	struct virtio_net *dev = get_device(vid);
+	int ret;
+
+	if (dev == NULL || ops == NULL)
+		return -1;
+
+	if (queue_id >= VHOST_MAX_VRING)
+		return -1;
+
+	vq = dev->virtqueue[queue_id];
+
+	if (unlikely(vq == NULL || !dev->async_copy))
+		return -1;
+
+	if (unlikely(!(config.features & RTE_VHOST_ASYNC_INORDER))) {
+		VHOST_LOG_CONFIG(ERR,
+			"async copy is not supported on non-inorder mode "
+			"(vid %d, qid: %d)\n", vid, queue_id);
+		return -1;
+	}
+
+	if (unlikely(ops->check_completed_copies == NULL ||
+		ops->transfer_data == NULL))
+		return -1;
+
+	rte_spinlock_lock(&vq->access_lock);
+	ret = async_channel_register(vid, queue_id, config, ops);
 	rte_spinlock_unlock(&vq->access_lock);
 
-	return 0;
+	return ret;
+}
+
+int
+rte_vhost_async_channel_register_thread_unsafe(int vid, uint16_t queue_id,
+		struct rte_vhost_async_config config,
+		struct rte_vhost_async_channel_ops *ops)
+{
+	struct vhost_virtqueue *vq;
+	struct virtio_net *dev = get_device(vid);
+
+	if (dev == NULL || ops == NULL)
+		return -1;
+
+	if (queue_id >= VHOST_MAX_VRING)
+		return -1;
+
+	vq = dev->virtqueue[queue_id];
+
+	if (unlikely(vq == NULL || !dev->async_copy))
+		return -1;
+
+	if (unlikely(!(config.features & RTE_VHOST_ASYNC_INORDER))) {
+		VHOST_LOG_CONFIG(ERR,
+			"async copy is not supported on non-inorder mode "
+			"(vid %d, qid: %d)\n", vid, queue_id);
+		return -1;
+	}
+
+	if (unlikely(ops->check_completed_copies == NULL ||
+		ops->transfer_data == NULL))
+		return -1;
+
+	return async_channel_register(vid, queue_id, config, ops);
 }
 
 int
@@ -1775,5 +1819,40 @@ rte_vhost_async_channel_unregister(int vid, uint16_t queue_id)
 	return ret;
 }
 
+int
+rte_vhost_async_channel_unregister_thread_unsafe(int vid, uint16_t queue_id)
+{
+	struct vhost_virtqueue *vq;
+	struct virtio_net *dev = get_device(vid);
+
+	if (dev == NULL)
+		return -1;
+
+	if (queue_id >= VHOST_MAX_VRING)
+		return -1;
+
+	vq = dev->virtqueue[queue_id];
+
+	if (vq == NULL)
+		return -1;
+
+	if (!vq->async_registered)
+		return 0;
+
+	if (vq->async_pkts_inflight_n) {
+		VHOST_LOG_CONFIG(ERR, "Failed to unregister async channel. "
+			"async inflight packets must be completed before unregistration.\n");
+		return -1;
+	}
+
+	vhost_free_async_mem(vq);
+
+	vq->async_ops.transfer_data = NULL;
+	vq->async_ops.check_completed_copies = NULL;
+	vq->async_registered = false;
+
+	return 0;
+}
+
 RTE_LOG_REGISTER_SUFFIX(vhost_config_log_level, config, INFO);
 RTE_LOG_REGISTER_SUFFIX(vhost_data_log_level, data, WARNING);
-- 
2.7.4

Re: [dpdk-dev] [PATCH v5 2/3] vhost: rework async configuration structure

[Xia, Chenbo]

> -----Original Message-----
> From: Hu, Jiayu <jiayu.hu@intel.com>
> Sent: Saturday, July 17, 2021 3:51 AM
> To: dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>; Hu, Jiayu
> <jiayu.hu@intel.com>
> Subject: [PATCH v5 2/3] vhost: rework async configuration structure
> 
> This patch reworks the async configuration structure to improve code
> readability. In addition, add preserved padding fields on the structure
> for future usage.
> 
> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> ---
>  doc/guides/prog_guide/vhost_lib.rst | 21 +++++++++--------
>  examples/vhost/main.c               |  8 +++----
>  lib/vhost/rte_vhost_async.h         | 45 ++++++++++++++++++------------------
> -
>  lib/vhost/vhost.c                   | 19 +++++++---------
>  lib/vhost/vhost.h                   |  3 +--
>  5 files changed, 47 insertions(+), 49 deletions(-)
> 
> --
> 2.7.4

Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>

Re: [dpdk-dev] [PATCH v5 3/3] vhost: add thread unsafe async registeration functions

[Xia, Chenbo]

Hi Jiayu,

> -----Original Message-----
> From: Hu, Jiayu <jiayu.hu@intel.com>
> Sent: Saturday, July 17, 2021 3:51 AM
> To: dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>; Hu, Jiayu
> <jiayu.hu@intel.com>
> Subject: [PATCH v5 3/3] vhost: add thread unsafe async registeration functions
> 
> This patch adds thread unsafe version for async register and
> unregister functions.
> 
> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> ---
>  doc/guides/prog_guide/vhost_lib.rst |  14 ++++
>  lib/vhost/rte_vhost_async.h         |  41 ++++++++++
>  lib/vhost/version.map               |   4 +
>  lib/vhost/vhost.c                   | 149 +++++++++++++++++++++++++++--------
> -
>  4 files changed, 173 insertions(+), 35 deletions(-)
> 
> diff --git a/doc/guides/prog_guide/vhost_lib.rst
> b/doc/guides/prog_guide/vhost_lib.rst
> index 2a61b85..c8638db 100644
> --- a/doc/guides/prog_guide/vhost_lib.rst
> +++ b/doc/guides/prog_guide/vhost_lib.rst
> @@ -256,6 +256,13 @@ The following is an overview of some key Vhost API
> functions:
>      vhost invokes this function to get the copy data completed by async
>      devices.
> 
> +* ``rte_vhost_async_channel_register_thread_unsafe(vid, queue_id, config,
> ops)``
> +  Register an async copy device channel for a vhost queue without
> +  performing any locking.
> +
> +  This function is only safe to call in vhost callback functions
> +  (i.e., struct vhost_device_ops).
> +
>  * ``rte_vhost_async_channel_unregister(vid, queue_id)``
> 
>    Unregister the async copy device channel from a vhost queue.
> @@ -268,6 +275,13 @@ The following is an overview of some key Vhost API
> functions:
>    devices for all vhost queues in destroy_device(), when a
>    virtio device is paused or shut down.
> 
> +* ``rte_vhost_async_channel_unregister_thread_unsafe(vid, queue_id)``

We should add a blank line between API name and its description. I will add
them when applying.

With above fixed:

Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>

Re: [dpdk-dev] [PATCH v5 2/3] vhost: rework async configuration structure

[Maxime Coquelin]

On 7/16/21 9:51 PM, Jiayu Hu wrote:
> This patch reworks the async configuration structure to improve code
> readability. In addition, add preserved padding fields on the structure
> for future usage.
> 
> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> ---
>  doc/guides/prog_guide/vhost_lib.rst | 21 +++++++++--------
>  examples/vhost/main.c               |  8 +++----
>  lib/vhost/rte_vhost_async.h         | 45 ++++++++++++++++++-------------------
>  lib/vhost/vhost.c                   | 19 +++++++---------
>  lib/vhost/vhost.h                   |  3 +--
>  5 files changed, 47 insertions(+), 49 deletions(-)
> 
> diff --git a/doc/guides/prog_guide/vhost_lib.rst b/doc/guides/prog_guide/vhost_lib.rst
> index d18fb98..2a61b85 100644
> --- a/doc/guides/prog_guide/vhost_lib.rst
> +++ b/doc/guides/prog_guide/vhost_lib.rst
> @@ -218,26 +218,29 @@ The following is an overview of some key Vhost API functions:
>  
>    Enable or disable zero copy feature of the vhost crypto backend.
>  
> -* ``rte_vhost_async_channel_register(vid, queue_id, features, ops)``
> +* ``rte_vhost_async_channel_register(vid, queue_id, config, ops)``
>  
> -  Register a vhost queue with async copy device channel after vring
> -  is enabled. Following device ``features`` must be specified together
> +  Register an async copy device channel for a vhost queue after vring
> +  is enabled. Following device ``config`` must be specified together
>    with the registration:
>  
> -  * ``async_inorder``
> +  * ``features``
>  
> -    Async copy device can guarantee the ordering of copy completion
> -    sequence. Copies are completed in the same order with that at
> -    the submission time.
> +    This field is used to specify async copy device features.
>  
> -    Currently, only ``async_inorder`` capable device is supported by vhost.
> +    ``RTE_VHOST_ASYNC_INORDER`` represents the async copy device can
> +    guarantee the order of copy completion is the same as the order
> +    of copy submission.
> +
> +    Currently, only ``RTE_VHOST_ASYNC_INORDER`` capable device is
> +    supported by vhost.
>  
>    * ``async_threshold``
>  
>      The copy length (in bytes) below which CPU copy will be used even if
>      applications call async vhost APIs to enqueue/dequeue data.
>  
> -    Typical value is 512~1024 depending on the async device capability.
> +    Typical value is 256~1024 depending on the async device capability.
>  
>    Applications must provide following ``ops`` callbacks for vhost lib to
>    work with the async copy devices:
> diff --git a/examples/vhost/main.c b/examples/vhost/main.c
> index d2179ea..9cd855a 100644
> --- a/examples/vhost/main.c
> +++ b/examples/vhost/main.c
> @@ -1468,7 +1468,7 @@ new_device(int vid)
>  		vid, vdev->coreid);
>  
>  	if (async_vhost_driver) {
> -		struct rte_vhost_async_features f;
> +		struct rte_vhost_async_config config = {0};
>  		struct rte_vhost_async_channel_ops channel_ops;
>  
>  		if (dma_type != NULL && strncmp(dma_type, "ioat", 4) == 0) {
> @@ -1476,11 +1476,11 @@ new_device(int vid)
>  			channel_ops.check_completed_copies =
>  				ioat_check_completed_copies_cb;
>  
> -			f.async_inorder = 1;
> -			f.async_threshold = 256;
> +			config.features = RTE_VHOST_ASYNC_INORDER;
> +			config.async_threshold = 256;
>  
>  			return rte_vhost_async_channel_register(vid, VIRTIO_RXQ,
> -				f.intval, &channel_ops);
> +				config, &channel_ops);
>  		}
>  	}
>  
> diff --git a/lib/vhost/rte_vhost_async.h b/lib/vhost/rte_vhost_async.h
> index 6faa31f..52b1727 100644
> --- a/lib/vhost/rte_vhost_async.h
> +++ b/lib/vhost/rte_vhost_async.h
> @@ -93,49 +93,48 @@ struct async_inflight_info {
>  };
>  
>  /**
> - *  dma channel feature bit definition
> + *  async channel features
>   */
> -struct rte_vhost_async_features {
> -	union {
> -		uint32_t intval;
> -		struct {
> -			uint32_t async_inorder:1;
> -			uint32_t resvd_0:15;
> -			uint32_t async_threshold:12;
> -			uint32_t resvd_1:4;
> -		};
> -	};
> +enum {
> +	RTE_VHOST_ASYNC_FEATURE_UNKNOWN = 0U,
Please remove this entry, it has no meaning.

> +	RTE_VHOST_ASYNC_INORDER = 1U << 0,
>  };
>  
>  /**
> - * register an async channel for vhost
> + *  async channel configuration
> + */
> +struct rte_vhost_async_config {
> +	uint32_t async_threshold;
> +	uint32_t features;
> +	uint32_t rsvd[2];
> +};
> +
> +/**
> + * Register an async channel for a vhost queue
>   *
>   * @param vid
>   *  vhost device id async channel to be attached to
>   * @param queue_id
>   *  vhost queue id async channel to be attached to
> - * @param features
> - *  DMA channel feature bit
> - *    b0       : DMA supports inorder data transfer
> - *    b1  - b15: reserved
> - *    b16 - b27: Packet length threshold for DMA transfer
> - *    b28 - b31: reserved
> + * @param config
> + *  Async channel configuration structure
>   * @param ops
> - *  DMA operation callbacks
> + *  Async channel operation callbacks
>   * @return
>   *  0 on success, -1 on failures
>   */
>  __rte_experimental
>  int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
> -	uint32_t features, struct rte_vhost_async_channel_ops *ops);
> +	struct rte_vhost_async_config config,
> +	struct rte_vhost_async_channel_ops *ops);
>  
>  /**
> - * unregister a dma channel for vhost
> + * Unregister an async channel for a vhost queue
>   *
>   * @param vid
> - *  vhost device id DMA channel to be detached
> + *  vhost device id async channel to be detached from
>   * @param queue_id
> - *  vhost queue id DMA channel to be detached
> + *  vhost queue id async channel to be detached from
>   * @return
>   *  0 on success, -1 on failures
>   */
> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> index 53a470f..908758e 100644
> --- a/lib/vhost/vhost.c
> +++ b/lib/vhost/vhost.c
> @@ -1619,19 +1619,17 @@ int rte_vhost_extern_callback_register(int vid,
>  	return 0;
>  }
>  
> -int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
> -					uint32_t features,
> -					struct rte_vhost_async_channel_ops *ops)
> +int
> +rte_vhost_async_channel_register(int vid, uint16_t queue_id,
> +		struct rte_vhost_async_config config,
> +		struct rte_vhost_async_channel_ops *ops)
>  {
>  	struct vhost_virtqueue *vq;
>  	struct virtio_net *dev = get_device(vid);
> -	struct rte_vhost_async_features f;
>  
>  	if (dev == NULL || ops == NULL)
>  		return -1;
>  
> -	f.intval = features;
> -
>  	if (queue_id >= VHOST_MAX_VRING)
>  		return -1;
>  
> @@ -1640,7 +1638,7 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
>  	if (unlikely(vq == NULL || !dev->async_copy))
>  		return -1;
>  
> -	if (unlikely(!f.async_inorder)) {
> +	if (unlikely(!(config.features & RTE_VHOST_ASYNC_INORDER))) {
>  		VHOST_LOG_CONFIG(ERR,
>  			"async copy is not supported on non-inorder mode "
>  			"(vid %d, qid: %d)\n", vid, queue_id);
> @@ -1719,9 +1717,7 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
>  
>  	vq->async_ops.check_completed_copies = ops->check_completed_copies;
>  	vq->async_ops.transfer_data = ops->transfer_data;
> -
> -	vq->async_inorder = f.async_inorder;
> -	vq->async_threshold = f.async_threshold;
> +	vq->async_threshold = config.async_threshold;
>  
>  	vq->async_registered = true;
>  
> @@ -1731,7 +1727,8 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
>  	return 0;
>  }
>  
> -int rte_vhost_async_channel_unregister(int vid, uint16_t queue_id)
> +int
> +rte_vhost_async_channel_unregister(int vid, uint16_t queue_id)
>  {
>  	struct vhost_virtqueue *vq;
>  	struct virtio_net *dev = get_device(vid);
> diff --git a/lib/vhost/vhost.h b/lib/vhost/vhost.h
> index 8ffe387..d98ca8a 100644
> --- a/lib/vhost/vhost.h
> +++ b/lib/vhost/vhost.h
> @@ -218,9 +218,8 @@ struct vhost_virtqueue {
>  	};
>  
>  	/* vq async features */
> -	bool		async_inorder;
>  	bool		async_registered;
> -	uint16_t	async_threshold;
> +	uint32_t	async_threshold;
>  
>  	int			notif_enable;
>  #define VIRTIO_UNINITIALIZED_NOTIF	(-1)
> 

[dpdk-dev] [PATCH v6 0/3] provide thread unsafe async registration functions

[Jiayu Hu]

Lock protection is needed during the vhost notifies the application of
device readiness, so the first patch adds lock protection. In addition,
the second patch reworks async feature structure to improve readability.
After performing locking, existed async vhost registration functions will
cause deadlock, as they acquire lock too. The last patch provides thread
unsafe registration functions to support calling within vhost callback
functions.

v6:
* remove RTE_VHOST_ASYNC_FEATURE_UNKNOWN
* add blank lines in doc
v5:
* improve coding style
* update commit log, doc and comments
* remove useless field async_inorder
* change async_threshold from uint16_t to uint32_t
v4:
* remove brace {} in single statement block
v3:
* rename and use enum to define async device features
* change padding fields to 8 bytes
v2:
* rework async feature structure
* fix typo in commit log

Jiayu Hu (3):
  vhost: fix lock on device readiness notification
  vhost: rework async configuration structure
  vhost: add thread unsafe async registeration functions

 doc/guides/prog_guide/vhost_lib.rst |  37 ++++++--
 examples/vhost/main.c               |   8 +-
 lib/vhost/rte_vhost_async.h         |  85 ++++++++++++++-----
 lib/vhost/version.map               |   4 +
 lib/vhost/vhost.c                   | 162 ++++++++++++++++++++++++++----------
 lib/vhost/vhost.h                   |   3 +-
 lib/vhost/vhost_user.c              |   5 +-
 7 files changed, 220 insertions(+), 84 deletions(-)

-- 
2.7.4

[dpdk-dev] [PATCH v6 1/3] vhost: fix lock on device readiness notification

[Jiayu Hu]

The vhost notifies the application of device readiness via
vhost_user_notify_queue_state(), but calling this function
is not protected by the lock. This patch is to make this
function call lock protected.

Fixes: d0fcc38f5fa4 ("vhost: improve device readiness notifications")
Cc: stable@dpdk.org

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/vhost/vhost_user.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
index 031c578..31300e1 100644
--- a/lib/vhost/vhost_user.c
+++ b/lib/vhost/vhost_user.c
@@ -2995,9 +2995,6 @@ vhost_user_msg_handler(int vid, int fd)
 		}
 	}
 
-	if (unlock_required)
-		vhost_user_unlock_all_queue_pairs(dev);
-
 	/* If message was not handled at this stage, treat it as an error */
 	if (!handled) {
 		VHOST_LOG_CONFIG(ERR,
@@ -3032,6 +3029,8 @@ vhost_user_msg_handler(int vid, int fd)
 		}
 	}
 
+	if (unlock_required)
+		vhost_user_unlock_all_queue_pairs(dev);
 
 	if (!virtio_is_ready(dev))
 		goto out;
-- 
2.7.4

[dpdk-dev] [PATCH v6 2/3] vhost: rework async configuration structure

[Jiayu Hu]

This patch reworks the async configuration structure to improve code
readability. In addition, add preserved padding fields on the structure
for future usage.

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
---
 doc/guides/prog_guide/vhost_lib.rst | 21 ++++++++++--------
 examples/vhost/main.c               |  8 +++----
 lib/vhost/rte_vhost_async.h         | 44 ++++++++++++++++++-------------------
 lib/vhost/vhost.c                   | 19 +++++++---------
 lib/vhost/vhost.h                   |  3 +--
 5 files changed, 46 insertions(+), 49 deletions(-)

diff --git a/doc/guides/prog_guide/vhost_lib.rst b/doc/guides/prog_guide/vhost_lib.rst
index d18fb98..2a61b85 100644
--- a/doc/guides/prog_guide/vhost_lib.rst
+++ b/doc/guides/prog_guide/vhost_lib.rst
@@ -218,26 +218,29 @@ The following is an overview of some key Vhost API functions:
 
   Enable or disable zero copy feature of the vhost crypto backend.
 
-* ``rte_vhost_async_channel_register(vid, queue_id, features, ops)``
+* ``rte_vhost_async_channel_register(vid, queue_id, config, ops)``
 
-  Register a vhost queue with async copy device channel after vring
-  is enabled. Following device ``features`` must be specified together
+  Register an async copy device channel for a vhost queue after vring
+  is enabled. Following device ``config`` must be specified together
   with the registration:
 
-  * ``async_inorder``
+  * ``features``
 
-    Async copy device can guarantee the ordering of copy completion
-    sequence. Copies are completed in the same order with that at
-    the submission time.
+    This field is used to specify async copy device features.
 
-    Currently, only ``async_inorder`` capable device is supported by vhost.
+    ``RTE_VHOST_ASYNC_INORDER`` represents the async copy device can
+    guarantee the order of copy completion is the same as the order
+    of copy submission.
+
+    Currently, only ``RTE_VHOST_ASYNC_INORDER`` capable device is
+    supported by vhost.
 
   * ``async_threshold``
 
     The copy length (in bytes) below which CPU copy will be used even if
     applications call async vhost APIs to enqueue/dequeue data.
 
-    Typical value is 512~1024 depending on the async device capability.
+    Typical value is 256~1024 depending on the async device capability.
 
   Applications must provide following ``ops`` callbacks for vhost lib to
   work with the async copy devices:
diff --git a/examples/vhost/main.c b/examples/vhost/main.c
index d2179ea..9cd855a 100644
--- a/examples/vhost/main.c
+++ b/examples/vhost/main.c
@@ -1468,7 +1468,7 @@ new_device(int vid)
 		vid, vdev->coreid);
 
 	if (async_vhost_driver) {
-		struct rte_vhost_async_features f;
+		struct rte_vhost_async_config config = {0};
 		struct rte_vhost_async_channel_ops channel_ops;
 
 		if (dma_type != NULL && strncmp(dma_type, "ioat", 4) == 0) {
@@ -1476,11 +1476,11 @@ new_device(int vid)
 			channel_ops.check_completed_copies =
 				ioat_check_completed_copies_cb;
 
-			f.async_inorder = 1;
-			f.async_threshold = 256;
+			config.features = RTE_VHOST_ASYNC_INORDER;
+			config.async_threshold = 256;
 
 			return rte_vhost_async_channel_register(vid, VIRTIO_RXQ,
-				f.intval, &channel_ops);
+				config, &channel_ops);
 		}
 	}
 
diff --git a/lib/vhost/rte_vhost_async.h b/lib/vhost/rte_vhost_async.h
index 6faa31f..ef8c1a2 100644
--- a/lib/vhost/rte_vhost_async.h
+++ b/lib/vhost/rte_vhost_async.h
@@ -93,49 +93,47 @@ struct async_inflight_info {
 };
 
 /**
- *  dma channel feature bit definition
+ *  async channel features
  */
-struct rte_vhost_async_features {
-	union {
-		uint32_t intval;
-		struct {
-			uint32_t async_inorder:1;
-			uint32_t resvd_0:15;
-			uint32_t async_threshold:12;
-			uint32_t resvd_1:4;
-		};
-	};
+enum {
+	RTE_VHOST_ASYNC_INORDER = 1U << 0,
 };
 
 /**
- * register an async channel for vhost
+ *  async channel configuration
+ */
+struct rte_vhost_async_config {
+	uint32_t async_threshold;
+	uint32_t features;
+	uint32_t rsvd[2];
+};
+
+/**
+ * Register an async channel for a vhost queue
  *
  * @param vid
  *  vhost device id async channel to be attached to
  * @param queue_id
  *  vhost queue id async channel to be attached to
- * @param features
- *  DMA channel feature bit
- *    b0       : DMA supports inorder data transfer
- *    b1  - b15: reserved
- *    b16 - b27: Packet length threshold for DMA transfer
- *    b28 - b31: reserved
+ * @param config
+ *  Async channel configuration structure
  * @param ops
- *  DMA operation callbacks
+ *  Async channel operation callbacks
  * @return
  *  0 on success, -1 on failures
  */
 __rte_experimental
 int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
-	uint32_t features, struct rte_vhost_async_channel_ops *ops);
+	struct rte_vhost_async_config config,
+	struct rte_vhost_async_channel_ops *ops);
 
 /**
- * unregister a dma channel for vhost
+ * Unregister an async channel for a vhost queue
  *
  * @param vid
- *  vhost device id DMA channel to be detached
+ *  vhost device id async channel to be detached from
  * @param queue_id
- *  vhost queue id DMA channel to be detached
+ *  vhost queue id async channel to be detached from
  * @return
  *  0 on success, -1 on failures
  */
diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
index 53a470f..908758e 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -1619,19 +1619,17 @@ int rte_vhost_extern_callback_register(int vid,
 	return 0;
 }
 
-int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
-					uint32_t features,
-					struct rte_vhost_async_channel_ops *ops)
+int
+rte_vhost_async_channel_register(int vid, uint16_t queue_id,
+		struct rte_vhost_async_config config,
+		struct rte_vhost_async_channel_ops *ops)
 {
 	struct vhost_virtqueue *vq;
 	struct virtio_net *dev = get_device(vid);
-	struct rte_vhost_async_features f;
 
 	if (dev == NULL || ops == NULL)
 		return -1;
 
-	f.intval = features;
-
 	if (queue_id >= VHOST_MAX_VRING)
 		return -1;
 
@@ -1640,7 +1638,7 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 	if (unlikely(vq == NULL || !dev->async_copy))
 		return -1;
 
-	if (unlikely(!f.async_inorder)) {
+	if (unlikely(!(config.features & RTE_VHOST_ASYNC_INORDER))) {
 		VHOST_LOG_CONFIG(ERR,
 			"async copy is not supported on non-inorder mode "
 			"(vid %d, qid: %d)\n", vid, queue_id);
@@ -1719,9 +1717,7 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 
 	vq->async_ops.check_completed_copies = ops->check_completed_copies;
 	vq->async_ops.transfer_data = ops->transfer_data;
-
-	vq->async_inorder = f.async_inorder;
-	vq->async_threshold = f.async_threshold;
+	vq->async_threshold = config.async_threshold;
 
 	vq->async_registered = true;
 
@@ -1731,7 +1727,8 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 	return 0;
 }
 
-int rte_vhost_async_channel_unregister(int vid, uint16_t queue_id)
+int
+rte_vhost_async_channel_unregister(int vid, uint16_t queue_id)
 {
 	struct vhost_virtqueue *vq;
 	struct virtio_net *dev = get_device(vid);
diff --git a/lib/vhost/vhost.h b/lib/vhost/vhost.h
index 8ffe387..d98ca8a 100644
--- a/lib/vhost/vhost.h
+++ b/lib/vhost/vhost.h
@@ -218,9 +218,8 @@ struct vhost_virtqueue {
 	};
 
 	/* vq async features */
-	bool		async_inorder;
 	bool		async_registered;
-	uint16_t	async_threshold;
+	uint32_t	async_threshold;
 
 	int			notif_enable;
 #define VIRTIO_UNINITIALIZED_NOTIF	(-1)
-- 
2.7.4

[dpdk-dev] [PATCH v6 3/3] vhost: add thread unsafe async registeration functions

[Jiayu Hu]

This patch adds thread unsafe version for async register and
unregister functions.

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
---
 doc/guides/prog_guide/vhost_lib.rst |  16 ++++
 lib/vhost/rte_vhost_async.h         |  41 ++++++++++
 lib/vhost/version.map               |   4 +
 lib/vhost/vhost.c                   | 149 +++++++++++++++++++++++++++---------
 4 files changed, 175 insertions(+), 35 deletions(-)

diff --git a/doc/guides/prog_guide/vhost_lib.rst b/doc/guides/prog_guide/vhost_lib.rst
index 2a61b85..462393d 100644
--- a/doc/guides/prog_guide/vhost_lib.rst
+++ b/doc/guides/prog_guide/vhost_lib.rst
@@ -256,6 +256,14 @@ The following is an overview of some key Vhost API functions:
     vhost invokes this function to get the copy data completed by async
     devices.
 
+* ``rte_vhost_async_channel_register_thread_unsafe(vid, queue_id, config, ops)``
+
+  Register an async copy device channel for a vhost queue without
+  performing any locking.
+
+  This function is only safe to call in vhost callback functions
+  (i.e., struct vhost_device_ops).
+
 * ``rte_vhost_async_channel_unregister(vid, queue_id)``
 
   Unregister the async copy device channel from a vhost queue.
@@ -268,6 +276,14 @@ The following is an overview of some key Vhost API functions:
   devices for all vhost queues in destroy_device(), when a
   virtio device is paused or shut down.
 
+* ``rte_vhost_async_channel_unregister_thread_unsafe(vid, queue_id)``
+
+  Unregister the async copy device channel for a vhost queue without
+  performing any locking.
+
+  This function is only safe to call in vhost callback functions
+  (i.e., struct vhost_device_ops).
+
 * ``rte_vhost_submit_enqueue_burst(vid, queue_id, pkts, count, comp_pkts, comp_count)``
 
   Submit an enqueue request to transmit ``count`` packets from host to guest
diff --git a/lib/vhost/rte_vhost_async.h b/lib/vhost/rte_vhost_async.h
index ef8c1a2..dbfbbcc 100644
--- a/lib/vhost/rte_vhost_async.h
+++ b/lib/vhost/rte_vhost_async.h
@@ -141,6 +141,47 @@ __rte_experimental
 int rte_vhost_async_channel_unregister(int vid, uint16_t queue_id);
 
 /**
+ * Register an async channel for a vhost queue without performing any
+ * locking
+ *
+ * @note This function does not perform any locking, and is only safe to
+ *       call in vhost callback functions.
+ *
+ * @param vid
+ *  vhost device id async channel to be attached to
+ * @param queue_id
+ *  vhost queue id async channel to be attached to
+ * @param config
+ *  Async channel configuration
+ * @param ops
+ *  Async channel operation callbacks
+ * @return
+ *  0 on success, -1 on failures
+ */
+__rte_experimental
+int rte_vhost_async_channel_register_thread_unsafe(int vid, uint16_t queue_id,
+	struct rte_vhost_async_config config,
+	struct rte_vhost_async_channel_ops *ops);
+
+/**
+ * Unregister an async channel for a vhost queue without performing any
+ * locking
+ *
+ * @note This function does not perform any locking, and is only safe to
+ *       call in vhost callback functions.
+ *
+ * @param vid
+ *  vhost device id async channel to be detached from
+ * @param queue_id
+ *  vhost queue id async channel to be detached from
+ * @return
+ *  0 on success, -1 on failures
+ */
+__rte_experimental
+int rte_vhost_async_channel_unregister_thread_unsafe(int vid,
+		uint16_t queue_id);
+
+/**
  * This function submits enqueue data to async engine. Successfully
  * enqueued packets can be transfer completed or being occupied by DMA
  * engines, when this API returns. Transfer completed packets are returned
diff --git a/lib/vhost/version.map b/lib/vhost/version.map
index 9103a23..2363db8 100644
--- a/lib/vhost/version.map
+++ b/lib/vhost/version.map
@@ -79,4 +79,8 @@ EXPERIMENTAL {
 
 	# added in 21.05
 	rte_vhost_get_negotiated_protocol_features;
+
+	# added in 21.08
+	rte_vhost_async_channel_register_thread_unsafe;
+	rte_vhost_async_channel_unregister_thread_unsafe;
 };
diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
index 908758e..d3d0205 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -1619,43 +1619,19 @@ int rte_vhost_extern_callback_register(int vid,
 	return 0;
 }
 
-int
-rte_vhost_async_channel_register(int vid, uint16_t queue_id,
+static __rte_always_inline int
+async_channel_register(int vid, uint16_t queue_id,
 		struct rte_vhost_async_config config,
 		struct rte_vhost_async_channel_ops *ops)
 {
-	struct vhost_virtqueue *vq;
 	struct virtio_net *dev = get_device(vid);
-
-	if (dev == NULL || ops == NULL)
-		return -1;
-
-	if (queue_id >= VHOST_MAX_VRING)
-		return -1;
-
-	vq = dev->virtqueue[queue_id];
-
-	if (unlikely(vq == NULL || !dev->async_copy))
-		return -1;
-
-	if (unlikely(!(config.features & RTE_VHOST_ASYNC_INORDER))) {
-		VHOST_LOG_CONFIG(ERR,
-			"async copy is not supported on non-inorder mode "
-			"(vid %d, qid: %d)\n", vid, queue_id);
-		return -1;
-	}
-
-	if (unlikely(ops->check_completed_copies == NULL ||
-		ops->transfer_data == NULL))
-		return -1;
-
-	rte_spinlock_lock(&vq->access_lock);
+	struct vhost_virtqueue *vq = dev->virtqueue[queue_id];
 
 	if (unlikely(vq->async_registered)) {
 		VHOST_LOG_CONFIG(ERR,
 			"async register failed: channel already registered "
 			"(vid %d, qid: %d)\n", vid, queue_id);
-		goto reg_out;
+		return -1;
 	}
 
 	vq->async_pkts_info = rte_malloc_socket(NULL,
@@ -1666,7 +1642,7 @@ rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 		VHOST_LOG_CONFIG(ERR,
 			"async register failed: cannot allocate memory for async_pkts_info "
 			"(vid %d, qid: %d)\n", vid, queue_id);
-		goto reg_out;
+		return -1;
 	}
 
 	vq->it_pool = rte_malloc_socket(NULL,
@@ -1677,7 +1653,7 @@ rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 		VHOST_LOG_CONFIG(ERR,
 			"async register failed: cannot allocate memory for it_pool "
 			"(vid %d, qid: %d)\n", vid, queue_id);
-		goto reg_out;
+		return -1;
 	}
 
 	vq->vec_pool = rte_malloc_socket(NULL,
@@ -1688,7 +1664,7 @@ rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 		VHOST_LOG_CONFIG(ERR,
 			"async register failed: cannot allocate memory for vec_pool "
 			"(vid %d, qid: %d)\n", vid, queue_id);
-		goto reg_out;
+		return -1;
 	}
 
 	if (vq_is_packed(dev)) {
@@ -1700,7 +1676,7 @@ rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 			VHOST_LOG_CONFIG(ERR,
 				"async register failed: cannot allocate memory for async buffers "
 				"(vid %d, qid: %d)\n", vid, queue_id);
-			goto reg_out;
+			return -1;
 		}
 	} else {
 		vq->async_descs_split = rte_malloc_socket(NULL,
@@ -1711,7 +1687,7 @@ rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 			VHOST_LOG_CONFIG(ERR,
 				"async register failed: cannot allocate memory for async descs "
 				"(vid %d, qid: %d)\n", vid, queue_id);
-			goto reg_out;
+			return -1;
 		}
 	}
 
@@ -1721,10 +1697,78 @@ rte_vhost_async_channel_register(int vid, uint16_t queue_id,
 
 	vq->async_registered = true;
 
-reg_out:
+	return 0;
+}
+
+int
+rte_vhost_async_channel_register(int vid, uint16_t queue_id,
+		struct rte_vhost_async_config config,
+		struct rte_vhost_async_channel_ops *ops)
+{
+	struct vhost_virtqueue *vq;
+	struct virtio_net *dev = get_device(vid);
+	int ret;
+
+	if (dev == NULL || ops == NULL)
+		return -1;
+
+	if (queue_id >= VHOST_MAX_VRING)
+		return -1;
+
+	vq = dev->virtqueue[queue_id];
+
+	if (unlikely(vq == NULL || !dev->async_copy))
+		return -1;
+
+	if (unlikely(!(config.features & RTE_VHOST_ASYNC_INORDER))) {
+		VHOST_LOG_CONFIG(ERR,
+			"async copy is not supported on non-inorder mode "
+			"(vid %d, qid: %d)\n", vid, queue_id);
+		return -1;
+	}
+
+	if (unlikely(ops->check_completed_copies == NULL ||
+		ops->transfer_data == NULL))
+		return -1;
+
+	rte_spinlock_lock(&vq->access_lock);
+	ret = async_channel_register(vid, queue_id, config, ops);
 	rte_spinlock_unlock(&vq->access_lock);
 
-	return 0;
+	return ret;
+}
+
+int
+rte_vhost_async_channel_register_thread_unsafe(int vid, uint16_t queue_id,
+		struct rte_vhost_async_config config,
+		struct rte_vhost_async_channel_ops *ops)
+{
+	struct vhost_virtqueue *vq;
+	struct virtio_net *dev = get_device(vid);
+
+	if (dev == NULL || ops == NULL)
+		return -1;
+
+	if (queue_id >= VHOST_MAX_VRING)
+		return -1;
+
+	vq = dev->virtqueue[queue_id];
+
+	if (unlikely(vq == NULL || !dev->async_copy))
+		return -1;
+
+	if (unlikely(!(config.features & RTE_VHOST_ASYNC_INORDER))) {
+		VHOST_LOG_CONFIG(ERR,
+			"async copy is not supported on non-inorder mode "
+			"(vid %d, qid: %d)\n", vid, queue_id);
+		return -1;
+	}
+
+	if (unlikely(ops->check_completed_copies == NULL ||
+		ops->transfer_data == NULL))
+		return -1;
+
+	return async_channel_register(vid, queue_id, config, ops);
 }
 
 int
@@ -1775,5 +1819,40 @@ rte_vhost_async_channel_unregister(int vid, uint16_t queue_id)
 	return ret;
 }
 
+int
+rte_vhost_async_channel_unregister_thread_unsafe(int vid, uint16_t queue_id)
+{
+	struct vhost_virtqueue *vq;
+	struct virtio_net *dev = get_device(vid);
+
+	if (dev == NULL)
+		return -1;
+
+	if (queue_id >= VHOST_MAX_VRING)
+		return -1;
+
+	vq = dev->virtqueue[queue_id];
+
+	if (vq == NULL)
+		return -1;
+
+	if (!vq->async_registered)
+		return 0;
+
+	if (vq->async_pkts_inflight_n) {
+		VHOST_LOG_CONFIG(ERR, "Failed to unregister async channel. "
+			"async inflight packets must be completed before unregistration.\n");
+		return -1;
+	}
+
+	vhost_free_async_mem(vq);
+
+	vq->async_ops.transfer_data = NULL;
+	vq->async_ops.check_completed_copies = NULL;
+	vq->async_registered = false;
+
+	return 0;
+}
+
 RTE_LOG_REGISTER_SUFFIX(vhost_config_log_level, config, INFO);
 RTE_LOG_REGISTER_SUFFIX(vhost_data_log_level, data, WARNING);
-- 
2.7.4

Re: [dpdk-dev] [PATCH v6 2/3] vhost: rework async configuration structure

[Maxime Coquelin]

On 7/19/21 5:00 PM, Jiayu Hu wrote:
> This patch reworks the async configuration structure to improve code
> readability. In addition, add preserved padding fields on the structure
> for future usage.
> 
> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
> ---
>  doc/guides/prog_guide/vhost_lib.rst | 21 ++++++++++--------
>  examples/vhost/main.c               |  8 +++----
>  lib/vhost/rte_vhost_async.h         | 44 ++++++++++++++++++-------------------
>  lib/vhost/vhost.c                   | 19 +++++++---------
>  lib/vhost/vhost.h                   |  3 +--
>  5 files changed, 46 insertions(+), 49 deletions(-)


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

Please see one comment below:


> diff --git a/doc/guides/prog_guide/vhost_lib.rst b/doc/guides/prog_guide/vhost_lib.rst
> index d18fb98..2a61b85 100644
> --- a/doc/guides/prog_guide/vhost_lib.rst
> +++ b/doc/guides/prog_guide/vhost_lib.rst
> @@ -218,26 +218,29 @@ The following is an overview of some key Vhost API functions:
>  
>    Enable or disable zero copy feature of the vhost crypto backend.
>  
> -* ``rte_vhost_async_channel_register(vid, queue_id, features, ops)``
> +* ``rte_vhost_async_channel_register(vid, queue_id, config, ops)``
>  
> -  Register a vhost queue with async copy device channel after vring
> -  is enabled. Following device ``features`` must be specified together
> +  Register an async copy device channel for a vhost queue after vring
> +  is enabled. Following device ``config`` must be specified together
>    with the registration:
>  
> -  * ``async_inorder``
> +  * ``features``
>  
> -    Async copy device can guarantee the ordering of copy completion
> -    sequence. Copies are completed in the same order with that at
> -    the submission time.
> +    This field is used to specify async copy device features.
>  
> -    Currently, only ``async_inorder`` capable device is supported by vhost.
> +    ``RTE_VHOST_ASYNC_INORDER`` represents the async copy device can
> +    guarantee the order of copy completion is the same as the order
> +    of copy submission.
> +
> +    Currently, only ``RTE_VHOST_ASYNC_INORDER`` capable device is
> +    supported by vhost.
>  
>    * ``async_threshold``
>  
>      The copy length (in bytes) below which CPU copy will be used even if
>      applications call async vhost APIs to enqueue/dequeue data.
>  
> -    Typical value is 512~1024 depending on the async device capability.
> +    Typical value is 256~1024 depending on the async device capability.
>  
>    Applications must provide following ``ops`` callbacks for vhost lib to
>    work with the async copy devices:
> diff --git a/examples/vhost/main.c b/examples/vhost/main.c
> index d2179ea..9cd855a 100644
> --- a/examples/vhost/main.c
> +++ b/examples/vhost/main.c
> @@ -1468,7 +1468,7 @@ new_device(int vid)
>  		vid, vdev->coreid);
>  
>  	if (async_vhost_driver) {
> -		struct rte_vhost_async_features f;
> +		struct rte_vhost_async_config config = {0};
>  		struct rte_vhost_async_channel_ops channel_ops;
>  
>  		if (dma_type != NULL && strncmp(dma_type, "ioat", 4) == 0) {
> @@ -1476,11 +1476,11 @@ new_device(int vid)
>  			channel_ops.check_completed_copies =
>  				ioat_check_completed_copies_cb;
>  
> -			f.async_inorder = 1;
> -			f.async_threshold = 256;
> +			config.features = RTE_VHOST_ASYNC_INORDER;
> +			config.async_threshold = 256;
>  
>  			return rte_vhost_async_channel_register(vid, VIRTIO_RXQ,
> -				f.intval, &channel_ops);
> +				config, &channel_ops);
>  		}
>  	}
>  
> diff --git a/lib/vhost/rte_vhost_async.h b/lib/vhost/rte_vhost_async.h
> index 6faa31f..ef8c1a2 100644
> --- a/lib/vhost/rte_vhost_async.h
> +++ b/lib/vhost/rte_vhost_async.h
> @@ -93,49 +93,47 @@ struct async_inflight_info {
>  };
>  
>  /**
> - *  dma channel feature bit definition
> + *  async channel features
>   */
> -struct rte_vhost_async_features {
> -	union {
> -		uint32_t intval;
> -		struct {
> -			uint32_t async_inorder:1;
> -			uint32_t resvd_0:15;
> -			uint32_t async_threshold:12;
> -			uint32_t resvd_1:4;
> -		};
> -	};
> +enum {
> +	RTE_VHOST_ASYNC_INORDER = 1U << 0,

I think I understand the point of this flag, but the naming is
misleading. The code later on fails registration if the "INORDER" flag
is not passed, but the code behaves in the opposite, it does not support
Virtio INORDER mode as soon as a threashold is used.

Why do we need the DMA to write the descriptors buffers in order? My
understanding is that it is an optimization in order to ease the DMA
completion handling. So we are requiring DMA to be in-order, but on the
other hand we cannot guarantee the Virtio ring to be handled in order,
moving the complexity (and potential performance cost) to the guest.

>  };
>  
>  /**
> - * register an async channel for vhost
> + *  async channel configuration
> + */
> +struct rte_vhost_async_config {
> +	uint32_t async_threshold;
> +	uint32_t features;
> +	uint32_t rsvd[2];
> +};
> +
> +/**
> + * Register an async channel for a vhost queue
>   *
>   * @param vid
>   *  vhost device id async channel to be attached to
>   * @param queue_id
>   *  vhost queue id async channel to be attached to
> - * @param features
> - *  DMA channel feature bit
> - *    b0       : DMA supports inorder data transfer
> - *    b1  - b15: reserved
> - *    b16 - b27: Packet length threshold for DMA transfer
> - *    b28 - b31: reserved
> + * @param config
> + *  Async channel configuration structure
>   * @param ops
> - *  DMA operation callbacks
> + *  Async channel operation callbacks
>   * @return
>   *  0 on success, -1 on failures
>   */
>  __rte_experimental
>  int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
> -	uint32_t features, struct rte_vhost_async_channel_ops *ops);
> +	struct rte_vhost_async_config config,
> +	struct rte_vhost_async_channel_ops *ops);
>  
>  /**
> - * unregister a dma channel for vhost
> + * Unregister an async channel for a vhost queue
>   *
>   * @param vid
> - *  vhost device id DMA channel to be detached
> + *  vhost device id async channel to be detached from
>   * @param queue_id
> - *  vhost queue id DMA channel to be detached
> + *  vhost queue id async channel to be detached from
>   * @return
>   *  0 on success, -1 on failures
>   */
> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> index 53a470f..908758e 100644
> --- a/lib/vhost/vhost.c
> +++ b/lib/vhost/vhost.c
> @@ -1619,19 +1619,17 @@ int rte_vhost_extern_callback_register(int vid,
>  	return 0;
>  }
>  
> -int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
> -					uint32_t features,
> -					struct rte_vhost_async_channel_ops *ops)
> +int
> +rte_vhost_async_channel_register(int vid, uint16_t queue_id,
> +		struct rte_vhost_async_config config,
> +		struct rte_vhost_async_channel_ops *ops)
>  {
>  	struct vhost_virtqueue *vq;
>  	struct virtio_net *dev = get_device(vid);
> -	struct rte_vhost_async_features f;
>  
>  	if (dev == NULL || ops == NULL)
>  		return -1;
>  
> -	f.intval = features;
> -
>  	if (queue_id >= VHOST_MAX_VRING)
>  		return -1;
>  
> @@ -1640,7 +1638,7 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
>  	if (unlikely(vq == NULL || !dev->async_copy))
>  		return -1;
>  
> -	if (unlikely(!f.async_inorder)) {
> +	if (unlikely(!(config.features & RTE_VHOST_ASYNC_INORDER))) {
>  		VHOST_LOG_CONFIG(ERR,
>  			"async copy is not supported on non-inorder mode "
>  			"(vid %d, qid: %d)\n", vid, queue_id);
> @@ -1719,9 +1717,7 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
>  
>  	vq->async_ops.check_completed_copies = ops->check_completed_copies;
>  	vq->async_ops.transfer_data = ops->transfer_data;
> -
> -	vq->async_inorder = f.async_inorder;
> -	vq->async_threshold = f.async_threshold;
> +	vq->async_threshold = config.async_threshold;
>  
>  	vq->async_registered = true;
>  
> @@ -1731,7 +1727,8 @@ int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
>  	return 0;
>  }
>  
> -int rte_vhost_async_channel_unregister(int vid, uint16_t queue_id)
> +int
> +rte_vhost_async_channel_unregister(int vid, uint16_t queue_id)
>  {
>  	struct vhost_virtqueue *vq;
>  	struct virtio_net *dev = get_device(vid);
> diff --git a/lib/vhost/vhost.h b/lib/vhost/vhost.h
> index 8ffe387..d98ca8a 100644
> --- a/lib/vhost/vhost.h
> +++ b/lib/vhost/vhost.h
> @@ -218,9 +218,8 @@ struct vhost_virtqueue {
>  	};
>  
>  	/* vq async features */
> -	bool		async_inorder;
>  	bool		async_registered;
> -	uint16_t	async_threshold;
> +	uint32_t	async_threshold;
>  
>  	int			notif_enable;
>  #define VIRTIO_UNINITIALIZED_NOTIF	(-1)
> 

Re: [dpdk-dev] [PATCH v6 3/3] vhost: add thread unsafe async registeration functions

[Maxime Coquelin]

On 7/19/21 5:00 PM, Jiayu Hu wrote:
> This patch adds thread unsafe version for async register and
> unregister functions.
> 
> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
> ---
>  doc/guides/prog_guide/vhost_lib.rst |  16 ++++
>  lib/vhost/rte_vhost_async.h         |  41 ++++++++++
>  lib/vhost/version.map               |   4 +
>  lib/vhost/vhost.c                   | 149 +++++++++++++++++++++++++++---------
>  4 files changed, 175 insertions(+), 35 deletions(-)
> 

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

Thanks,
Maxime

Re: [dpdk-dev] [PATCH v6 2/3] vhost: rework async configuration structure

[Hu, Jiayu]

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, July 20, 2021 3:18 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
> Cc: Xia, Chenbo <chenbo.xia@intel.com>
> Subject: Re: [PATCH v6 2/3] vhost: rework async configuration structure
> 
> 
> 
> On 7/19/21 5:00 PM, Jiayu Hu wrote:
> > This patch reworks the async configuration structure to improve code
> > readability. In addition, add preserved padding fields on the
> > structure for future usage.
> >
> > Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> > Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
> > ---
> >  doc/guides/prog_guide/vhost_lib.rst | 21 ++++++++++--------
> >  examples/vhost/main.c               |  8 +++----
> >  lib/vhost/rte_vhost_async.h         | 44 ++++++++++++++++++-------------------
> >  lib/vhost/vhost.c                   | 19 +++++++---------
> >  lib/vhost/vhost.h                   |  3 +--
> >  5 files changed, 46 insertions(+), 49 deletions(-)
> 
> 
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> Please see one comment below:
> 
> 
> > diff --git a/doc/guides/prog_guide/vhost_lib.rst
> > b/doc/guides/prog_guide/vhost_lib.rst
> > index d18fb98..2a61b85 100644
> > --- a/doc/guides/prog_guide/vhost_lib.rst
> > +++ b/doc/guides/prog_guide/vhost_lib.rst
> > @@ -218,26 +218,29 @@ The following is an overview of some key Vhost
> API functions:
> >
> >    Enable or disable zero copy feature of the vhost crypto backend.
> >
> > -* ``rte_vhost_async_channel_register(vid, queue_id, features, ops)``
> > +* ``rte_vhost_async_channel_register(vid, queue_id, config, ops)``
> >
> > -  Register a vhost queue with async copy device channel after vring
> > -  is enabled. Following device ``features`` must be specified
> > together
> > +  Register an async copy device channel for a vhost queue after vring
> > + is enabled. Following device ``config`` must be specified together
> >    with the registration:
> >
> > -  * ``async_inorder``
> > +  * ``features``
> >
> > -    Async copy device can guarantee the ordering of copy completion
> > -    sequence. Copies are completed in the same order with that at
> > -    the submission time.
> > +    This field is used to specify async copy device features.
> >
> > -    Currently, only ``async_inorder`` capable device is supported by vhost.
> > +    ``RTE_VHOST_ASYNC_INORDER`` represents the async copy device can
> > +    guarantee the order of copy completion is the same as the order
> > +    of copy submission.
> > +
> > +    Currently, only ``RTE_VHOST_ASYNC_INORDER`` capable device is
> > +    supported by vhost.
> >
> >    * ``async_threshold``
> >
> >      The copy length (in bytes) below which CPU copy will be used even if
> >      applications call async vhost APIs to enqueue/dequeue data.
> >
> > -    Typical value is 512~1024 depending on the async device capability.
> > +    Typical value is 256~1024 depending on the async device capability.
> >
> >    Applications must provide following ``ops`` callbacks for vhost lib to
> >    work with the async copy devices:
> > diff --git a/examples/vhost/main.c b/examples/vhost/main.c index
> > d2179ea..9cd855a 100644
> > --- a/examples/vhost/main.c
> > +++ b/examples/vhost/main.c
> > @@ -1468,7 +1468,7 @@ new_device(int vid)
> >  		vid, vdev->coreid);
> >
> >  	if (async_vhost_driver) {
> > -		struct rte_vhost_async_features f;
> > +		struct rte_vhost_async_config config = {0};
> >  		struct rte_vhost_async_channel_ops channel_ops;
> >
> >  		if (dma_type != NULL && strncmp(dma_type, "ioat", 4) == 0)
> { @@
> > -1476,11 +1476,11 @@ new_device(int vid)
> >  			channel_ops.check_completed_copies =
> >  				ioat_check_completed_copies_cb;
> >
> > -			f.async_inorder = 1;
> > -			f.async_threshold = 256;
> > +			config.features = RTE_VHOST_ASYNC_INORDER;
> > +			config.async_threshold = 256;
> >
> >  			return rte_vhost_async_channel_register(vid,
> VIRTIO_RXQ,
> > -				f.intval, &channel_ops);
> > +				config, &channel_ops);
> >  		}
> >  	}
> >
> > diff --git a/lib/vhost/rte_vhost_async.h b/lib/vhost/rte_vhost_async.h
> > index 6faa31f..ef8c1a2 100644
> > --- a/lib/vhost/rte_vhost_async.h
> > +++ b/lib/vhost/rte_vhost_async.h
> > @@ -93,49 +93,47 @@ struct async_inflight_info {  };
> >
> >  /**
> > - *  dma channel feature bit definition
> > + *  async channel features
> >   */
> > -struct rte_vhost_async_features {
> > -	union {
> > -		uint32_t intval;
> > -		struct {
> > -			uint32_t async_inorder:1;
> > -			uint32_t resvd_0:15;
> > -			uint32_t async_threshold:12;
> > -			uint32_t resvd_1:4;
> > -		};
> > -	};
> > +enum {
> > +	RTE_VHOST_ASYNC_INORDER = 1U << 0,
> 
> I think I understand the point of this flag, but the naming is misleading. The
> code later on fails registration if the "INORDER" flag is not passed, but the
> code behaves in the opposite, it does not support Virtio INORDER mode as
> soon as a threashold is used.
> 
> Why do we need the DMA to write the descriptors buffers in order? My
> understanding is that it is an optimization in order to ease the DMA
> completion handling. So we are requiring DMA to be in-order, but on the
> other hand we cannot guarantee the Virtio ring to be handled in order,
> moving the complexity (and potential performance cost) to the guest.

Yes, the reason of requiring dma in-order is to simply vhost for higher performance.
Another reason is that in some cases, where a vring only uses one DMA channel,
the dma device guarantees the ordering already, so dma callback is in-ordered.
For other apps, who want the most complex vring and dma channel mapping, i.e.,
multiple vring use multiple dma, the order is better to be implemented by itself,
i.e., dma callback functions.

If we assume vring and dma channel mapping is M:N, the vhost lib implementation
is going to be more complex. But in my opinion, the mapping relationship of vring and
dma channel is per app specific. Implementing M:N inside vhost will sacrifice performance
for the simplest use case.

Right, the name is misleading, and virtio INORDER mode doesn't work if setting threshold.
But we'd better consider a tradeoff between dma efficiency and out-of-order impacts.
I will measure the performance with removing threshold supporting in the code, and let's
discuss with some data.

Thanks,
Jiayu
> 
> >  };
> >
> >  /**
> > - * register an async channel for vhost
> > + *  async channel configuration
> > + */
> > +struct rte_vhost_async_config {
> > +	uint32_t async_threshold;
> > +	uint32_t features;
> > +	uint32_t rsvd[2];
> > +};
> > +
> > +/**
> > + * Register an async channel for a vhost queue
> >   *
> >   * @param vid
> >   *  vhost device id async channel to be attached to
> >   * @param queue_id
> >   *  vhost queue id async channel to be attached to
> > - * @param features
> > - *  DMA channel feature bit
> > - *    b0       : DMA supports inorder data transfer
> > - *    b1  - b15: reserved
> > - *    b16 - b27: Packet length threshold for DMA transfer
> > - *    b28 - b31: reserved
> > + * @param config
> > + *  Async channel configuration structure
> >   * @param ops
> > - *  DMA operation callbacks
> > + *  Async channel operation callbacks
> >   * @return
> >   *  0 on success, -1 on failures
> >   */
> >  __rte_experimental
> >  int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
> > -	uint32_t features, struct rte_vhost_async_channel_ops *ops);
> > +	struct rte_vhost_async_config config,
> > +	struct rte_vhost_async_channel_ops *ops);
> >
> >  /**
> > - * unregister a dma channel for vhost
> > + * Unregister an async channel for a vhost queue
> >   *
> >   * @param vid
> > - *  vhost device id DMA channel to be detached
> > + *  vhost device id async channel to be detached from
> >   * @param queue_id
> > - *  vhost queue id DMA channel to be detached
> > + *  vhost queue id async channel to be detached from
> >   * @return
> >   *  0 on success, -1 on failures
> >   */
> > diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c index
> > 53a470f..908758e 100644
> > --- a/lib/vhost/vhost.c
> > +++ b/lib/vhost/vhost.c
> > @@ -1619,19 +1619,17 @@ int rte_vhost_extern_callback_register(int vid,
> >  	return 0;
> >  }
> >
> > -int rte_vhost_async_channel_register(int vid, uint16_t queue_id,
> > -					uint32_t features,
> > -					struct rte_vhost_async_channel_ops
> *ops)
> > +int
> > +rte_vhost_async_channel_register(int vid, uint16_t queue_id,
> > +		struct rte_vhost_async_config config,
> > +		struct rte_vhost_async_channel_ops *ops)
> >  {
> >  	struct vhost_virtqueue *vq;
> >  	struct virtio_net *dev = get_device(vid);
> > -	struct rte_vhost_async_features f;
> >
> >  	if (dev == NULL || ops == NULL)
> >  		return -1;
> >
> > -	f.intval = features;
> > -
> >  	if (queue_id >= VHOST_MAX_VRING)
> >  		return -1;
> >
> > @@ -1640,7 +1638,7 @@ int rte_vhost_async_channel_register(int vid,
> uint16_t queue_id,
> >  	if (unlikely(vq == NULL || !dev->async_copy))
> >  		return -1;
> >
> > -	if (unlikely(!f.async_inorder)) {
> > +	if (unlikely(!(config.features & RTE_VHOST_ASYNC_INORDER))) {
> >  		VHOST_LOG_CONFIG(ERR,
> >  			"async copy is not supported on non-inorder mode "
> >  			"(vid %d, qid: %d)\n", vid, queue_id); @@ -1719,9
> +1717,7 @@ int
> > rte_vhost_async_channel_register(int vid, uint16_t queue_id,
> >
> >  	vq->async_ops.check_completed_copies = ops-
> >check_completed_copies;
> >  	vq->async_ops.transfer_data = ops->transfer_data;
> > -
> > -	vq->async_inorder = f.async_inorder;
> > -	vq->async_threshold = f.async_threshold;
> > +	vq->async_threshold = config.async_threshold;
> >
> >  	vq->async_registered = true;
> >
> > @@ -1731,7 +1727,8 @@ int rte_vhost_async_channel_register(int vid,
> uint16_t queue_id,
> >  	return 0;
> >  }
> >
> > -int rte_vhost_async_channel_unregister(int vid, uint16_t queue_id)
> > +int
> > +rte_vhost_async_channel_unregister(int vid, uint16_t queue_id)
> >  {
> >  	struct vhost_virtqueue *vq;
> >  	struct virtio_net *dev = get_device(vid); diff --git
> > a/lib/vhost/vhost.h b/lib/vhost/vhost.h index 8ffe387..d98ca8a 100644
> > --- a/lib/vhost/vhost.h
> > +++ b/lib/vhost/vhost.h
> > @@ -218,9 +218,8 @@ struct vhost_virtqueue {
> >  	};
> >
> >  	/* vq async features */
> > -	bool		async_inorder;
> >  	bool		async_registered;
> > -	uint16_t	async_threshold;
> > +	uint32_t	async_threshold;
> >
> >  	int			notif_enable;
> >  #define VIRTIO_UNINITIALIZED_NOTIF	(-1)
> >

Re: [dpdk-dev] [PATCH v6 0/3] provide thread unsafe async registration functions

[Xia, Chenbo]

> -----Original Message-----
> From: Hu, Jiayu <jiayu.hu@intel.com>
> Sent: Monday, July 19, 2021 11:01 PM
> To: dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>; Hu, Jiayu
> <jiayu.hu@intel.com>
> Subject: [PATCH v6 0/3] provide thread unsafe async registration functions
> 
> Lock protection is needed during the vhost notifies the application of
> device readiness, so the first patch adds lock protection. In addition,
> the second patch reworks async feature structure to improve readability.
> After performing locking, existed async vhost registration functions will
> cause deadlock, as they acquire lock too. The last patch provides thread
> unsafe registration functions to support calling within vhost callback
> functions.
> 
> v6:
> * remove RTE_VHOST_ASYNC_FEATURE_UNKNOWN
> * add blank lines in doc
> v5:
> * improve coding style
> * update commit log, doc and comments
> * remove useless field async_inorder
> * change async_threshold from uint16_t to uint32_t
> v4:
> * remove brace {} in single statement block
> v3:
> * rename and use enum to define async device features
> * change padding fields to 8 bytes
> v2:
> * rework async feature structure
> * fix typo in commit log
> 
> Jiayu Hu (3):
>   vhost: fix lock on device readiness notification
>   vhost: rework async configuration structure
>   vhost: add thread unsafe async registeration functions
> 
>  doc/guides/prog_guide/vhost_lib.rst |  37 ++++++--
>  examples/vhost/main.c               |   8 +-
>  lib/vhost/rte_vhost_async.h         |  85 ++++++++++++++-----
>  lib/vhost/version.map               |   4 +
>  lib/vhost/vhost.c                   | 162 ++++++++++++++++++++++++++---------
> -
>  lib/vhost/vhost.h                   |   3 +-
>  lib/vhost/vhost_user.c              |   5 +-
>  7 files changed, 220 insertions(+), 84 deletions(-)
> 
> --
> 2.7.4

Series applied to next-virtio/main. Thanks