[dpdk-dev] [PATCH 0/4] Refactor async vhost control path

[dpdk-dev] [PATCH 0/4] Refactor async vhost control path

[Jiayu Hu]

This patch set refactors async vhost control path.

Jiayu Hu (4):
  vhost: fix uninitialized vhost queue
  vhost: remove unnecessary free
  vhost: avoid deadlock on async register
  doc: update async vhost register/unregister

 doc/guides/prog_guide/vhost_lib.rst | 13 ++++++++++---
 lib/librte_vhost/vhost.c            |  2 +-
 lib/librte_vhost/vhost_user.c       | 10 ----------
 3 files changed, 11 insertions(+), 14 deletions(-)

-- 
2.7.4

[dpdk-dev] [PATCH 1/4] vhost: fix uninitialized vhost queue

[Jiayu Hu]

This patch allocates vhost queue by rte_zmalloc() to avoid
undefined values.

Fixes: 8acd7c213353 ("vhost: fix virtqueues metadata allocation")

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
---
 lib/librte_vhost/vhost.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
index 52ab93d..f6fd001 100644
--- a/lib/librte_vhost/vhost.c
+++ b/lib/librte_vhost/vhost.c
@@ -598,7 +598,7 @@ alloc_vring_queue(struct virtio_net *dev, uint32_t vring_idx)
 		if (dev->virtqueue[i])
 			continue;
 
-		vq = rte_malloc(NULL, sizeof(struct vhost_virtqueue), 0);
+		vq = rte_zmalloc(NULL, sizeof(struct vhost_virtqueue), 0);
 		if (vq == NULL) {
 			VHOST_LOG_CONFIG(ERR,
 				"Failed to allocate memory for vring:%u.\n", i);
-- 
2.7.4

Re: [dpdk-dev] [PATCH 1/4] vhost: fix uninitialized vhost queue

[Maxime Coquelin]

On 3/17/21 1:56 PM, Jiayu Hu wrote:
> This patch allocates vhost queue by rte_zmalloc() to avoid
> undefined values.
> 
> Fixes: 8acd7c213353 ("vhost: fix virtqueues metadata allocation")

This is not the right commit. rte_malloc was used prior to it.

> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> ---
>  lib/librte_vhost/vhost.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
> index 52ab93d..f6fd001 100644
> --- a/lib/librte_vhost/vhost.c
> +++ b/lib/librte_vhost/vhost.c
> @@ -598,7 +598,7 @@ alloc_vring_queue(struct virtio_net *dev, uint32_t vring_idx)
>  		if (dev->virtqueue[i])
>  			continue;
>  
> -		vq = rte_malloc(NULL, sizeof(struct vhost_virtqueue), 0);
> +		vq = rte_zmalloc(NULL, sizeof(struct vhost_virtqueue), 0);
>  		if (vq == NULL) {
>  			VHOST_LOG_CONFIG(ERR,
>  				"Failed to allocate memory for vring:%u.\n", i);
> 

[dpdk-dev] [PATCH 2/4] vhost: remove unnecessary free

[Jiayu Hu]

This patch removes unnecessary rte_free() for async_pkts_info
and async_descs_split.

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
---
 lib/librte_vhost/vhost_user.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index a60bb94..399675c 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -2010,13 +2010,6 @@ vhost_user_get_vring_base(struct virtio_net **pdev,
 	} else {
 		rte_free(vq->shadow_used_split);
 		vq->shadow_used_split = NULL;
-
-		if (vq->async_pkts_info)
-			rte_free(vq->async_pkts_info);
-		if (vq->async_descs_split)
-			rte_free(vq->async_descs_split);
-		vq->async_pkts_info = NULL;
-		vq->async_descs_split = NULL;
 	}
 
 	rte_free(vq->batch_copy_elems);
-- 
2.7.4

Re: [dpdk-dev] [PATCH 2/4] vhost: remove unnecessary free

[Maxime Coquelin]

On 3/17/21 1:56 PM, Jiayu Hu wrote:
> This patch removes unnecessary rte_free() for async_pkts_info
> and async_descs_split.
> 
> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> ---
>  lib/librte_vhost/vhost_user.c | 7 -------
>  1 file changed, 7 deletions(-)
> 

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

Thanks,
Maxime

[dpdk-dev] [PATCH 3/4] vhost: avoid deadlock on async register

[Jiayu Hu]

Users register async copy device when vhost queue is enabled.
However, if VHOST_USER_F_PROTOCOL_FEATURES is not supported,
a deadlock occurs inside rte_vhost_async_channel_register(),
as vhost_user_msg_handler() already takes vq->access_lock
before processing VHOST_USER_SET_VRING_KICK message.

This patch removes calling vring_state_changed() in
vhost_user_set_vring_kick() to avoid deadlock on async register.

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
---
 lib/librte_vhost/vhost_user.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 399675c..a319c1c 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -1919,9 +1919,6 @@ vhost_user_set_vring_kick(struct virtio_net **pdev, struct VhostUserMsg *msg,
 	 */
 	if (!(dev->features & (1ULL << VHOST_USER_F_PROTOCOL_FEATURES))) {
 		vq->enabled = 1;
-		if (dev->notify_ops->vring_state_changed)
-			dev->notify_ops->vring_state_changed(
-				dev->vid, file.index, 1);
 	}
 
 	if (vq->ready) {
-- 
2.7.4

Re: [dpdk-dev] [PATCH 3/4] vhost: avoid deadlock on async register

[Maxime Coquelin]

On 3/17/21 1:56 PM, Jiayu Hu wrote:
> Users register async copy device when vhost queue is enabled.
> However, if VHOST_USER_F_PROTOCOL_FEATURES is not supported,
> a deadlock occurs inside rte_vhost_async_channel_register(),
> as vhost_user_msg_handler() already takes vq->access_lock
> before processing VHOST_USER_SET_VRING_KICK message.
> 
> This patch removes calling vring_state_changed() in
> vhost_user_set_vring_kick() to avoid deadlock on async register.
> 
> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> ---
>  lib/librte_vhost/vhost_user.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index 399675c..a319c1c 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -1919,9 +1919,6 @@ vhost_user_set_vring_kick(struct virtio_net **pdev, struct VhostUserMsg *msg,
>  	 */
>  	if (!(dev->features & (1ULL << VHOST_USER_F_PROTOCOL_FEATURES))) {
>  		vq->enabled = 1;
> -		if (dev->notify_ops->vring_state_changed)
> -			dev->notify_ops->vring_state_changed(
> -				dev->vid, file.index, 1);

That looks very wrong, as:
1. The apps want to receive this notification. It looks like breaking
existing apps in order to support the experimental async datapath. E.g.
OVS needs it to start polling the queues when protocol features is not
negotiated.

2. The fix in your case seems to indicate that your app's
vring_state_changed callback called rte_vhost_async_channel_register.
And your fix consists in no more calling the callback, and so no more
calling rte_vhost_async_channel_register?

>  	}
>  
>  	if (vq->ready) {
> 

Re: [dpdk-dev] [PATCH 3/4] vhost: avoid deadlock on async register

[Hu, Jiayu]

Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Monday, March 29, 2021 11:19 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Wang, Yinan
> <yinan.wang@intel.com>; Jiang, Cheng1 <cheng1.jiang@intel.com>; Pai G,
> Sunil <sunil.pai.g@intel.com>
> Subject: Re: [PATCH 3/4] vhost: avoid deadlock on async register
> 
> 
> 
> On 3/17/21 1:56 PM, Jiayu Hu wrote:
> > Users register async copy device when vhost queue is enabled.
> > However, if VHOST_USER_F_PROTOCOL_FEATURES is not supported,
> > a deadlock occurs inside rte_vhost_async_channel_register(),
> > as vhost_user_msg_handler() already takes vq->access_lock
> > before processing VHOST_USER_SET_VRING_KICK message.
> >
> > This patch removes calling vring_state_changed() in
> > vhost_user_set_vring_kick() to avoid deadlock on async register.
> >
> > Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> > ---
> >  lib/librte_vhost/vhost_user.c | 3 ---
> >  1 file changed, 3 deletions(-)
> >
> > diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> > index 399675c..a319c1c 100644
> > --- a/lib/librte_vhost/vhost_user.c
> > +++ b/lib/librte_vhost/vhost_user.c
> > @@ -1919,9 +1919,6 @@ vhost_user_set_vring_kick(struct virtio_net
> **pdev, struct VhostUserMsg *msg,
> >  	 */
> >  	if (!(dev->features & (1ULL <<
> VHOST_USER_F_PROTOCOL_FEATURES))) {
> >  		vq->enabled = 1;
> > -		if (dev->notify_ops->vring_state_changed)
> > -			dev->notify_ops->vring_state_changed(
> > -				dev->vid, file.index, 1);
> 
> That looks very wrong, as:
> 1. The apps want to receive this notification. It looks like breaking
> existing apps in order to support the experimental async datapath. E.g.
> OVS needs it to start polling the queues when protocol features is not
> negotiated.

IMHO, if protocol feature is not negotiated, vring_state_chaned will also
be called in vhost_user_msg_handler. In the case you mentioned,
vq->enabled is set to true in set_vring_kick, and in vhost_user_msg_handler,
"cur_ready != (vq && vq->ready)" is true, as vq->ready is false when init. So
vhost_user_msg_handler will call vhost_user_notify_queue_state, which
calls set_vring_kick inside.

In addition, calling vring_state_changed in set_vring_kick is protected by lock,
but it's not in in vhost_user_msg_handler. It looks confusing to me. Is there
any special reason for this design?

> 
> 2. The fix in your case seems to indicate that your app's
> vring_state_changed callback called rte_vhost_async_channel_register.
> And your fix consists in no more calling the callback, and so no more
> calling rte_vhost_async_channel_register?

rte_vhost_async_channel_register is recommended to call in
vring_state_changed, and vring_state_changed will be called
by vhost_user_msg_handler.

Thanks,
Jiayu
> 
> >  	}
> >
> >  	if (vq->ready) {
> >

Re: [dpdk-dev] [PATCH 3/4] vhost: avoid deadlock on async register

[Maxime Coquelin]

On 3/30/21 3:20 AM, Hu, Jiayu wrote:
> Hi Maxime,
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Monday, March 29, 2021 11:19 PM
>> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
>> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Wang, Yinan
>> <yinan.wang@intel.com>; Jiang, Cheng1 <cheng1.jiang@intel.com>; Pai G,
>> Sunil <sunil.pai.g@intel.com>
>> Subject: Re: [PATCH 3/4] vhost: avoid deadlock on async register
>>
>>
>>
>> On 3/17/21 1:56 PM, Jiayu Hu wrote:
>>> Users register async copy device when vhost queue is enabled.
>>> However, if VHOST_USER_F_PROTOCOL_FEATURES is not supported,
>>> a deadlock occurs inside rte_vhost_async_channel_register(),
>>> as vhost_user_msg_handler() already takes vq->access_lock
>>> before processing VHOST_USER_SET_VRING_KICK message.
>>>
>>> This patch removes calling vring_state_changed() in
>>> vhost_user_set_vring_kick() to avoid deadlock on async register.
>>>
>>> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
>>> ---
>>>  lib/librte_vhost/vhost_user.c | 3 ---
>>>  1 file changed, 3 deletions(-)
>>>
>>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
>>> index 399675c..a319c1c 100644
>>> --- a/lib/librte_vhost/vhost_user.c
>>> +++ b/lib/librte_vhost/vhost_user.c
>>> @@ -1919,9 +1919,6 @@ vhost_user_set_vring_kick(struct virtio_net
>> **pdev, struct VhostUserMsg *msg,
>>>  	 */
>>>  	if (!(dev->features & (1ULL <<
>> VHOST_USER_F_PROTOCOL_FEATURES))) {
>>>  		vq->enabled = 1;
>>> -		if (dev->notify_ops->vring_state_changed)
>>> -			dev->notify_ops->vring_state_changed(
>>> -				dev->vid, file.index, 1);
>>
>> That looks very wrong, as:
>> 1. The apps want to receive this notification. It looks like breaking
>> existing apps in order to support the experimental async datapath. E.g.
>> OVS needs it to start polling the queues when protocol features is not
>> negotiated.
> 
> IMHO, if protocol feature is not negotiated, vring_state_chaned will also
> be called in vhost_user_msg_handler. In the case you mentioned,
> vq->enabled is set to true in set_vring_kick, and in vhost_user_msg_handler,
> "cur_ready != (vq && vq->ready)" is true, as vq->ready is false when init. So
> vhost_user_msg_handler will call vhost_user_notify_queue_state, which
> calls set_vring_kick inside.

OK, I agree, we can drop this one.
But it is not enough as vhost_user_notify_queue_state() is called at
several place with the lock taken.

> In addition, calling vring_state_changed in set_vring_kick is protected by lock,
> but it's not in in vhost_user_msg_handler. It looks confusing to me. Is there
> any special reason for this design?

I think we need the lock help every time the callback is called, to
avoid the case an application calls a Vhost API that would modify the vq
struct. We could get undefined behavior if it happened.

> 
>>
>> 2. The fix in your case seems to indicate that your app's
>> vring_state_changed callback called rte_vhost_async_channel_register.
>> And your fix consists in no more calling the callback, and so no more
>> calling rte_vhost_async_channel_register?
> 
> rte_vhost_async_channel_register is recommended to call in
> vring_state_changed, and vring_state_changed will be called
> by vhost_user_msg_handler.

You might want to schedule a thread to call channel registration. Maybe
using rte_set_alarm?

Regards,
Maxime

> 
> Thanks,
> Jiayu
>>
>>>  	}
>>>
>>>  	if (vq->ready) {
>>>
> 

[dpdk-dev] [PATCH 4/4] doc: update async vhost register/unregister

[Jiayu Hu]

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
---
 doc/guides/prog_guide/vhost_lib.rst | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/doc/guides/prog_guide/vhost_lib.rst b/doc/guides/prog_guide/vhost_lib.rst
index dc29229..333bd0a 100644
--- a/doc/guides/prog_guide/vhost_lib.rst
+++ b/doc/guides/prog_guide/vhost_lib.rst
@@ -208,9 +208,9 @@ The following is an overview of some key Vhost API functions:
 
 * ``rte_vhost_async_channel_register(vid, queue_id, features, ops)``
 
-  Register a vhost queue with async copy device channel.
-  Following device ``features`` must be specified together with the
-  registration:
+  Register a vhost queue with async copy device channel after vring
+  is enabled. Following device ``features`` must be specified together
+  with the registration:
 
   * ``async_inorder``
 
@@ -244,6 +244,13 @@ The following is an overview of some key Vhost API functions:
 * ``rte_vhost_async_channel_unregister(vid, queue_id)``
 
   Unregister the async copy device channel from a vhost queue.
+  Unregistration will fail, if the vhost queue has in-flight
+  packets that are not completed.
+
+  To guarantee correct behaviors, users must unregister async
+  copy devices for all vhost queues, when virtio device is paused
+  or shut down. Note that this API tries to acquire the spinlock
+  of vhost queue, so users need stop polling thread before unregister.
 
 * ``rte_vhost_submit_enqueue_burst(vid, queue_id, pkts, count, comp_pkts, comp_count)``
 
-- 
2.7.4

[dpdk-dev] [PATCH v2 0/4] Refactor async vhost control path

[Jiayu Hu]

This patch set refactors async vhost control path.

Change log
==========
v2:
- correct fix commit
- update commit log

Jiayu Hu (4):
  vhost: fix uninitialized vhost queue
  vhost: remove unnecessary free
  vhost: avoid deadlock on async register
  doc: update async vhost register/unregister

 doc/guides/prog_guide/vhost_lib.rst | 13 ++++++++++---
 lib/librte_vhost/vhost.c            |  2 +-
 lib/librte_vhost/vhost_user.c       | 10 ----------
 3 files changed, 11 insertions(+), 14 deletions(-)

-- 
2.7.4

Re: [dpdk-dev] [PATCH v2 0/4] Refactor async vhost control path

[Wang, Yinan]

Tested-by: Wang, Yinan <yinan.wang@intel.com>

> -----Original Message-----
> From: Hu, Jiayu <jiayu.hu@intel.com>
> Sent: 2021?4?2? 21:04
> To: dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>;
> Wang, Yinan <yinan.wang@intel.com>; Pai G, Sunil <sunil.pai.g@intel.com>;
> Jiang, Cheng1 <cheng1.jiang@intel.com>; Hu, Jiayu <jiayu.hu@intel.com>
> Subject: [PATCH v2 0/4] Refactor async vhost control path
> 
> This patch set refactors async vhost control path.
> 
> Change log
> ==========
> v2:
> - correct fix commit
> - update commit log
> 
> Jiayu Hu (4):
>   vhost: fix uninitialized vhost queue
>   vhost: remove unnecessary free
>   vhost: avoid deadlock on async register
>   doc: update async vhost register/unregister
> 
>  doc/guides/prog_guide/vhost_lib.rst | 13 ++++++++++---
>  lib/librte_vhost/vhost.c            |  2 +-
>  lib/librte_vhost/vhost_user.c       | 10 ----------
>  3 files changed, 11 insertions(+), 14 deletions(-)
> 
> --
> 2.7.4

[dpdk-dev] [PATCH v2 1/4] vhost: fix uninitialized vhost queue

[Jiayu Hu]

This patch allocates vhost queue by rte_zmalloc() to avoid
undefined values.

Fixes: a277c7159876 ("vhost: refactor code structure")
Cc: stable@dpdk.org

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
---
 lib/librte_vhost/vhost.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
index c9d1371..8657bbe 100644
--- a/lib/librte_vhost/vhost.c
+++ b/lib/librte_vhost/vhost.c
@@ -609,7 +609,7 @@ alloc_vring_queue(struct virtio_net *dev, uint32_t vring_idx)
 		if (dev->virtqueue[i])
 			continue;
 
-		vq = rte_malloc(NULL, sizeof(struct vhost_virtqueue), 0);
+		vq = rte_zmalloc(NULL, sizeof(struct vhost_virtqueue), 0);
 		if (vq == NULL) {
 			VHOST_LOG_CONFIG(ERR,
 				"Failed to allocate memory for vring:%u.\n", i);
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2 1/4] vhost: fix uninitialized vhost queue

[Maxime Coquelin]

On 4/2/21 3:03 PM, Jiayu Hu wrote:
> This patch allocates vhost queue by rte_zmalloc() to avoid
> undefined values.
> 
> Fixes: a277c7159876 ("vhost: refactor code structure")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> ---
>  lib/librte_vhost/vhost.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
> index c9d1371..8657bbe 100644
> --- a/lib/librte_vhost/vhost.c
> +++ b/lib/librte_vhost/vhost.c
> @@ -609,7 +609,7 @@ alloc_vring_queue(struct virtio_net *dev, uint32_t vring_idx)
>  		if (dev->virtqueue[i])
>  			continue;
>  
> -		vq = rte_malloc(NULL, sizeof(struct vhost_virtqueue), 0);
> +		vq = rte_zmalloc(NULL, sizeof(struct vhost_virtqueue), 0);
>  		if (vq == NULL) {
>  			VHOST_LOG_CONFIG(ERR,
>  				"Failed to allocate memory for vring:%u.\n", i);
> 

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

Thanks,
Maxime

[dpdk-dev] [PATCH v2 2/4] vhost: remove unnecessary free

[Jiayu Hu]

This patch removes unnecessary rte_free() for async_pkts_info
and async_descs_split.

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

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 2f4f89a..44c0452 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -2009,13 +2009,6 @@ vhost_user_get_vring_base(struct virtio_net **pdev,
 	} else {
 		rte_free(vq->shadow_used_split);
 		vq->shadow_used_split = NULL;
-
-		if (vq->async_pkts_info)
-			rte_free(vq->async_pkts_info);
-		if (vq->async_descs_split)
-			rte_free(vq->async_descs_split);
-		vq->async_pkts_info = NULL;
-		vq->async_descs_split = NULL;
 	}
 
 	rte_free(vq->batch_copy_elems);
-- 
2.7.4

[dpdk-dev] [PATCH v2 3/4] vhost: avoid deadlock on async register

[Jiayu Hu]

Users can register async copy device in vring_state_changed(),
when vhost queue is enabled. However, a deadlock occurs inside
rte_vhost_async_channel_register(), if VHOST_USER_F_PROTOCOL_FEATURES
is not supported, as vhost_user_msg_handler() takes vq->access_lock
before calling vhost_user_set_vring_kick().

This patch avoids async register deadlock by removing calling
vring_state_changed() in vhost_user_set_vring_kick(). It's safe
as vhost_user_msg_handler() will call vring_state_changed() anyway.

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
---
 lib/librte_vhost/vhost_user.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 44c0452..8f0eba6 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -1918,9 +1918,6 @@ vhost_user_set_vring_kick(struct virtio_net **pdev, struct VhostUserMsg *msg,
 	 */
 	if (!(dev->features & (1ULL << VHOST_USER_F_PROTOCOL_FEATURES))) {
 		vq->enabled = true;
-		if (dev->notify_ops->vring_state_changed)
-			dev->notify_ops->vring_state_changed(
-				dev->vid, file.index, 1);
 	}
 
 	if (vq->ready) {
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2 3/4] vhost: avoid deadlock on async register

[Maxime Coquelin]

On 4/2/21 3:04 PM, Jiayu Hu wrote:
> Users can register async copy device in vring_state_changed(),
> when vhost queue is enabled. However, a deadlock occurs inside
> rte_vhost_async_channel_register(), if VHOST_USER_F_PROTOCOL_FEATURES
> is not supported, as vhost_user_msg_handler() takes vq->access_lock
> before calling vhost_user_set_vring_kick().
> 
> This patch avoids async register deadlock by removing calling
> vring_state_changed() in vhost_user_set_vring_kick(). It's safe
> as vhost_user_msg_handler() will call vring_state_changed() anyway.
> 
> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> ---
>  lib/librte_vhost/vhost_user.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index 44c0452..8f0eba6 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -1918,9 +1918,6 @@ vhost_user_set_vring_kick(struct virtio_net **pdev, struct VhostUserMsg *msg,
>  	 */
>  	if (!(dev->features & (1ULL << VHOST_USER_F_PROTOCOL_FEATURES))) {
>  		vq->enabled = true;
> -		if (dev->notify_ops->vring_state_changed)
> -			dev->notify_ops->vring_state_changed(
> -				dev->vid, file.index, 1);
>  	}
>  
>  	if (vq->ready) {
> 

As replied earlier on v1, I agree the call to vring_state_changed here
is not needed. But it might not be enough, there are other cases where
you could have issues.

Please add stable and Fixes tag.

Thanks,
Maxime

Re: [dpdk-dev] [PATCH v2 3/4] vhost: avoid deadlock on async register

[Hu, Jiayu]

Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, April 13, 2021 7:33 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Wang, Yinan
> <yinan.wang@intel.com>; Pai G, Sunil <sunil.pai.g@intel.com>; Jiang, Cheng1
> <cheng1.jiang@intel.com>
> Subject: Re: [PATCH v2 3/4] vhost: avoid deadlock on async register
> 
> 
> 
> On 4/2/21 3:04 PM, Jiayu Hu wrote:
> > Users can register async copy device in vring_state_changed(),
> > when vhost queue is enabled. However, a deadlock occurs inside
> > rte_vhost_async_channel_register(), if
> VHOST_USER_F_PROTOCOL_FEATURES
> > is not supported, as vhost_user_msg_handler() takes vq->access_lock
> > before calling vhost_user_set_vring_kick().
> >
> > This patch avoids async register deadlock by removing calling
> > vring_state_changed() in vhost_user_set_vring_kick(). It's safe
> > as vhost_user_msg_handler() will call vring_state_changed() anyway.
> >
> > Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> > ---
> >  lib/librte_vhost/vhost_user.c | 3 ---
> >  1 file changed, 3 deletions(-)
> >
> > diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> > index 44c0452..8f0eba6 100644
> > --- a/lib/librte_vhost/vhost_user.c
> > +++ b/lib/librte_vhost/vhost_user.c
> > @@ -1918,9 +1918,6 @@ vhost_user_set_vring_kick(struct virtio_net
> **pdev, struct VhostUserMsg *msg,
> >  	 */
> >  	if (!(dev->features & (1ULL <<
> VHOST_USER_F_PROTOCOL_FEATURES))) {
> >  		vq->enabled = true;
> > -		if (dev->notify_ops->vring_state_changed)
> > -			dev->notify_ops->vring_state_changed(
> > -				dev->vid, file.index, 1);
> >  	}
> >
> >  	if (vq->ready) {
> >
> 
> As replied earlier on v1, I agree the call to vring_state_changed here
> is not needed. But it might not be enough, there are other cases where
> you could have issues.

vhost_user_notify_queue_state() can be called in three cases:
1. when vq ready status changes, vhost_user_msg_handler() calls it to notify
backend. But vhost_user_msg_handler() doesn't take lock before calling it.
So in this case, no deadlock occurs in async register.

2. if vq->ready is true, vhost_user_set_vring_call() calls it to notify backend
vq is not enabled. Although vhost_user_set_vring_call() is protected by lock,
async register is called only if vq is enabled, so async register will not be called
in this case.

3. If vq->ready is true, vhost_user_set_vring_kick() calls it to notify backend
vq is not enabled. Same as #2, async register is called only when vq is enabled.
Even if vhost_user_set_vring_kick() is protected by lock, there is no deadlock in
async register, as it will not be called in this case.

In summary,  I think there is no deadlock issue in async register if we
can remove calling vring_state_change() in vhost_user_set_vring_kick().

> 
> Please add stable and Fixes tag.

Do you suggest to make the patch as a fix for 8639d54563a
("vhost: introduce async enqueue registration API")? But the
thing is that code removed in this patch is not introduced
by this commit.

Thanks,
Jiayu
> 
> Thanks,
> Maxime

Re: [dpdk-dev] [PATCH v2 3/4] vhost: avoid deadlock on async register

[Maxime Coquelin]

On 4/14/21 3:40 AM, Hu, Jiayu wrote:
> Hi Maxime,
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Tuesday, April 13, 2021 7:33 PM
>> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
>> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Wang, Yinan
>> <yinan.wang@intel.com>; Pai G, Sunil <sunil.pai.g@intel.com>; Jiang, Cheng1
>> <cheng1.jiang@intel.com>
>> Subject: Re: [PATCH v2 3/4] vhost: avoid deadlock on async register
>>
>>
>>
>> On 4/2/21 3:04 PM, Jiayu Hu wrote:
>>> Users can register async copy device in vring_state_changed(),
>>> when vhost queue is enabled. However, a deadlock occurs inside
>>> rte_vhost_async_channel_register(), if
>> VHOST_USER_F_PROTOCOL_FEATURES
>>> is not supported, as vhost_user_msg_handler() takes vq->access_lock
>>> before calling vhost_user_set_vring_kick().
>>>
>>> This patch avoids async register deadlock by removing calling
>>> vring_state_changed() in vhost_user_set_vring_kick(). It's safe
>>> as vhost_user_msg_handler() will call vring_state_changed() anyway.
>>>
>>> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
>>> ---
>>>  lib/librte_vhost/vhost_user.c | 3 ---
>>>  1 file changed, 3 deletions(-)
>>>
>>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
>>> index 44c0452..8f0eba6 100644
>>> --- a/lib/librte_vhost/vhost_user.c
>>> +++ b/lib/librte_vhost/vhost_user.c
>>> @@ -1918,9 +1918,6 @@ vhost_user_set_vring_kick(struct virtio_net
>> **pdev, struct VhostUserMsg *msg,
>>>  	 */
>>>  	if (!(dev->features & (1ULL <<
>> VHOST_USER_F_PROTOCOL_FEATURES))) {
>>>  		vq->enabled = true;
>>> -		if (dev->notify_ops->vring_state_changed)
>>> -			dev->notify_ops->vring_state_changed(
>>> -				dev->vid, file.index, 1);
>>>  	}
>>>
>>>  	if (vq->ready) {
>>>
>>
>> As replied earlier on v1, I agree the call to vring_state_changed here
>> is not needed. But it might not be enough, there are other cases where
>> you could have issues.
> 
> vhost_user_notify_queue_state() can be called in three cases:
> 1. when vq ready status changes, vhost_user_msg_handler() calls it to notify
> backend. But vhost_user_msg_handler() doesn't take lock before calling it.
> So in this case, no deadlock occurs in async register.
>
> 2. if vq->ready is true, vhost_user_set_vring_call() calls it to notify backend
> vq is not enabled. Although vhost_user_set_vring_call() is protected by lock,
> async register is called only if vq is enabled, so async register will not be called
> in this case.
> 
> 3. If vq->ready is true, vhost_user_set_vring_kick() calls it to notify backend
> vq is not enabled. Same as #2, async register is called only when vq is enabled.
> Even if vhost_user_set_vring_kick() is protected by lock, there is no deadlock in
> async register, as it will not be called in this case.
> 
> In summary,  I think there is no deadlock issue in async register if we
> can remove calling vring_state_change() in vhost_user_set_vring_kick().


But unregister one could be called in theory no? Otherwise it would look
unbalanced. At least on disabled notification, the app should make sure
the DMA transfers to and from the vring are stopped before it returns
from the callabck. Otherwise it could lead to undefined behavior.

>>
>> Please add stable and Fixes tag.
> 
> Do you suggest to make the patch as a fix for 8639d54563a
> ("vhost: introduce async enqueue registration API")? But the
> thing is that code removed in this patch is not introduced
> by this commit.

The commit you need to point to is the one introducing the
.vring_state_changed() call.

> Thanks,
> Jiayu
>>
>> Thanks,
>> Maxime
> 

Re: [dpdk-dev] [PATCH v2 3/4] vhost: avoid deadlock on async register

[Hu, Jiayu]

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Wednesday, April 14, 2021 6:09 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Wang, Yinan
> <yinan.wang@intel.com>; Pai G, Sunil <sunil.pai.g@intel.com>; Jiang, Cheng1
> <cheng1.jiang@intel.com>
> Subject: Re: [PATCH v2 3/4] vhost: avoid deadlock on async register
> 
> 
> 
> On 4/14/21 3:40 AM, Hu, Jiayu wrote:
> > Hi Maxime,
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Sent: Tuesday, April 13, 2021 7:33 PM
> >> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
> >> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Wang, Yinan
> >> <yinan.wang@intel.com>; Pai G, Sunil <sunil.pai.g@intel.com>; Jiang,
> Cheng1
> >> <cheng1.jiang@intel.com>
> >> Subject: Re: [PATCH v2 3/4] vhost: avoid deadlock on async register
> >>
> >>
> >>
> >> On 4/2/21 3:04 PM, Jiayu Hu wrote:
> >>> Users can register async copy device in vring_state_changed(),
> >>> when vhost queue is enabled. However, a deadlock occurs inside
> >>> rte_vhost_async_channel_register(), if
> >> VHOST_USER_F_PROTOCOL_FEATURES
> >>> is not supported, as vhost_user_msg_handler() takes vq->access_lock
> >>> before calling vhost_user_set_vring_kick().
> >>>
> >>> This patch avoids async register deadlock by removing calling
> >>> vring_state_changed() in vhost_user_set_vring_kick(). It's safe
> >>> as vhost_user_msg_handler() will call vring_state_changed() anyway.
> >>>
> >>> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> >>> ---
> >>>  lib/librte_vhost/vhost_user.c | 3 ---
> >>>  1 file changed, 3 deletions(-)
> >>>
> >>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> >>> index 44c0452..8f0eba6 100644
> >>> --- a/lib/librte_vhost/vhost_user.c
> >>> +++ b/lib/librte_vhost/vhost_user.c
> >>> @@ -1918,9 +1918,6 @@ vhost_user_set_vring_kick(struct virtio_net
> >> **pdev, struct VhostUserMsg *msg,
> >>>  	 */
> >>>  	if (!(dev->features & (1ULL <<
> >> VHOST_USER_F_PROTOCOL_FEATURES))) {
> >>>  		vq->enabled = true;
> >>> -		if (dev->notify_ops->vring_state_changed)
> >>> -			dev->notify_ops->vring_state_changed(
> >>> -				dev->vid, file.index, 1);
> >>>  	}
> >>>
> >>>  	if (vq->ready) {
> >>>
> >>
> >> As replied earlier on v1, I agree the call to vring_state_changed here
> >> is not needed. But it might not be enough, there are other cases where
> >> you could have issues.
> >
> > vhost_user_notify_queue_state() can be called in three cases:
> > 1. when vq ready status changes, vhost_user_msg_handler() calls it to
> notify
> > backend. But vhost_user_msg_handler() doesn't take lock before calling it.
> > So in this case, no deadlock occurs in async register.
> >
> > 2. if vq->ready is true, vhost_user_set_vring_call() calls it to notify backend
> > vq is not enabled. Although vhost_user_set_vring_call() is protected by lock,
> > async register is called only if vq is enabled, so async register will not be
> called
> > in this case.
> >
> > 3. If vq->ready is true, vhost_user_set_vring_kick() calls it to notify backend
> > vq is not enabled. Same as #2, async register is called only when vq is
> enabled.
> > Even if vhost_user_set_vring_kick() is protected by lock, there is no
> deadlock in
> > async register, as it will not be called in this case.
> >
> > In summary,  I think there is no deadlock issue in async register if we
> > can remove calling vring_state_change() in vhost_user_set_vring_kick().
> 
> 
> But unregister one could be called in theory no? Otherwise it would look
> unbalanced. At least on disabled notification, the app should make sure
> the DMA transfers to and from the vring are stopped before it returns
> from the callabck. Otherwise it could lead to undefined behavior.

Right, users need to call unregister, but we cannot remove calling
vhost_user_notify_queue_state() in case #2 and #3, IMHO. So to
avoid deadlock, we recommended users to call async unregister in
destroy_device(), instead of on vring disabled notification. Does it
make sense to you?

> 
> >>
> >> Please add stable and Fixes tag.
> >
> > Do you suggest to make the patch as a fix for 8639d54563a
> > ("vhost: introduce async enqueue registration API")? But the
> > thing is that code removed in this patch is not introduced
> > by this commit.
> 
> The commit you need to point to is the one introducing the
> .vring_state_changed() call.

So this patch is still a fix for deadlock on async register? Or it is
a fix for unnecessary .vring_state_changed() call?

Thanks,
Jiayu

> 
> > Thanks,
> > Jiayu
> >>
> >> Thanks,
> >> Maxime
> >

Re: [dpdk-dev] [PATCH v2 3/4] vhost: avoid deadlock on async register

[Maxime Coquelin]

On 4/15/21 3:08 AM, Hu, Jiayu wrote:
> 
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Wednesday, April 14, 2021 6:09 PM
>> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
>> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Wang, Yinan
>> <yinan.wang@intel.com>; Pai G, Sunil <sunil.pai.g@intel.com>; Jiang, Cheng1
>> <cheng1.jiang@intel.com>
>> Subject: Re: [PATCH v2 3/4] vhost: avoid deadlock on async register
>>
>>
>>
>> On 4/14/21 3:40 AM, Hu, Jiayu wrote:
>>> Hi Maxime,
>>>
>>>> -----Original Message-----
>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>> Sent: Tuesday, April 13, 2021 7:33 PM
>>>> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
>>>> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Wang, Yinan
>>>> <yinan.wang@intel.com>; Pai G, Sunil <sunil.pai.g@intel.com>; Jiang,
>> Cheng1
>>>> <cheng1.jiang@intel.com>
>>>> Subject: Re: [PATCH v2 3/4] vhost: avoid deadlock on async register
>>>>
>>>>
>>>>
>>>> On 4/2/21 3:04 PM, Jiayu Hu wrote:
>>>>> Users can register async copy device in vring_state_changed(),
>>>>> when vhost queue is enabled. However, a deadlock occurs inside
>>>>> rte_vhost_async_channel_register(), if
>>>> VHOST_USER_F_PROTOCOL_FEATURES
>>>>> is not supported, as vhost_user_msg_handler() takes vq->access_lock
>>>>> before calling vhost_user_set_vring_kick().
>>>>>
>>>>> This patch avoids async register deadlock by removing calling
>>>>> vring_state_changed() in vhost_user_set_vring_kick(). It's safe
>>>>> as vhost_user_msg_handler() will call vring_state_changed() anyway.
>>>>>
>>>>> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
>>>>> ---
>>>>>  lib/librte_vhost/vhost_user.c | 3 ---
>>>>>  1 file changed, 3 deletions(-)
>>>>>
>>>>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
>>>>> index 44c0452..8f0eba6 100644
>>>>> --- a/lib/librte_vhost/vhost_user.c
>>>>> +++ b/lib/librte_vhost/vhost_user.c
>>>>> @@ -1918,9 +1918,6 @@ vhost_user_set_vring_kick(struct virtio_net
>>>> **pdev, struct VhostUserMsg *msg,
>>>>>  	 */
>>>>>  	if (!(dev->features & (1ULL <<
>>>> VHOST_USER_F_PROTOCOL_FEATURES))) {
>>>>>  		vq->enabled = true;
>>>>> -		if (dev->notify_ops->vring_state_changed)
>>>>> -			dev->notify_ops->vring_state_changed(
>>>>> -				dev->vid, file.index, 1);
>>>>>  	}
>>>>>
>>>>>  	if (vq->ready) {
>>>>>
>>>>
>>>> As replied earlier on v1, I agree the call to vring_state_changed here
>>>> is not needed. But it might not be enough, there are other cases where
>>>> you could have issues.
>>>
>>> vhost_user_notify_queue_state() can be called in three cases:
>>> 1. when vq ready status changes, vhost_user_msg_handler() calls it to
>> notify
>>> backend. But vhost_user_msg_handler() doesn't take lock before calling it.
>>> So in this case, no deadlock occurs in async register.
>>>
>>> 2. if vq->ready is true, vhost_user_set_vring_call() calls it to notify backend
>>> vq is not enabled. Although vhost_user_set_vring_call() is protected by lock,
>>> async register is called only if vq is enabled, so async register will not be
>> called
>>> in this case.
>>>
>>> 3. If vq->ready is true, vhost_user_set_vring_kick() calls it to notify backend
>>> vq is not enabled. Same as #2, async register is called only when vq is
>> enabled.
>>> Even if vhost_user_set_vring_kick() is protected by lock, there is no
>> deadlock in
>>> async register, as it will not be called in this case.
>>>
>>> In summary,  I think there is no deadlock issue in async register if we
>>> can remove calling vring_state_change() in vhost_user_set_vring_kick().
>>
>>
>> But unregister one could be called in theory no? Otherwise it would look
>> unbalanced. At least on disabled notification, the app should make sure
>> the DMA transfers to and from the vring are stopped before it returns
>> from the callabck. Otherwise it could lead to undefined behavior.
> 
> Right, users need to call unregister, but we cannot remove calling
> vhost_user_notify_queue_state() in case #2 and #3, IMHO. So to
> avoid deadlock, we recommended users to call async unregister in
> destroy_device(), instead of on vring disabled notification. Does it
> make sense to you?

Calling async unregister in destroy device is fine by me. But I'm more
concerned about DMA transations not being stopped when the ring becomes
disabled.

I cannot say if you are doing it right, because the vhost example does
not implement the vring_state_changed callback.
It is not a problem with the sync datapath as we have the lock
protection + enabled variable that prevents to process the rings when it
gets stopped.
But for the async path, if you have programmed DMA transfers, you need
to rely on the vring_state_change to block the control path while the
transfers are cancelled or done.

>>
>>>>
>>>> Please add stable and Fixes tag.
>>>
>>> Do you suggest to make the patch as a fix for 8639d54563a
>>> ("vhost: introduce async enqueue registration API")? But the
>>> thing is that code removed in this patch is not introduced
>>> by this commit.
>>
>> The commit you need to point to is the one introducing the
>> .vring_state_changed() call.
> 
> So this patch is still a fix for deadlock on async register? Or it is
> a fix for unnecessary .vring_state_changed() call?

You made the point that the vring_state_changed() call was not necessary
in any case. So it can fix the commit introducing it.

> Thanks,
> Jiayu
> 
>>
>>> Thanks,
>>> Jiayu
>>>>
>>>> Thanks,
>>>> Maxime
>>>
> 

Re: [dpdk-dev] [PATCH v2 3/4] vhost: avoid deadlock on async register

[Hu, Jiayu]

Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Thursday, April 15, 2021 3:09 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Wang, Yinan
> <yinan.wang@intel.com>; Pai G, Sunil <sunil.pai.g@intel.com>; Jiang, Cheng1
> <cheng1.jiang@intel.com>
> Subject: Re: [PATCH v2 3/4] vhost: avoid deadlock on async register
> 
> 
> 
> On 4/15/21 3:08 AM, Hu, Jiayu wrote:
> >
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Sent: Wednesday, April 14, 2021 6:09 PM
> >> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
> >> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Wang, Yinan
> >> <yinan.wang@intel.com>; Pai G, Sunil <sunil.pai.g@intel.com>; Jiang,
> Cheng1
> >> <cheng1.jiang@intel.com>
> >> Subject: Re: [PATCH v2 3/4] vhost: avoid deadlock on async register
> >>
> >>
> >>
> >> On 4/14/21 3:40 AM, Hu, Jiayu wrote:
> >>> Hi Maxime,
> >>>
> >>>> -----Original Message-----
> >>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>> Sent: Tuesday, April 13, 2021 7:33 PM
> >>>> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
> >>>> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Wang, Yinan
> >>>> <yinan.wang@intel.com>; Pai G, Sunil <sunil.pai.g@intel.com>; Jiang,
> >> Cheng1
> >>>> <cheng1.jiang@intel.com>
> >>>> Subject: Re: [PATCH v2 3/4] vhost: avoid deadlock on async register
> >>>>
> >>>>
> >>>>
> >>>> On 4/2/21 3:04 PM, Jiayu Hu wrote:
> >>>>> Users can register async copy device in vring_state_changed(),
> >>>>> when vhost queue is enabled. However, a deadlock occurs inside
> >>>>> rte_vhost_async_channel_register(), if
> >>>> VHOST_USER_F_PROTOCOL_FEATURES
> >>>>> is not supported, as vhost_user_msg_handler() takes vq->access_lock
> >>>>> before calling vhost_user_set_vring_kick().
> >>>>>
> >>>>> This patch avoids async register deadlock by removing calling
> >>>>> vring_state_changed() in vhost_user_set_vring_kick(). It's safe
> >>>>> as vhost_user_msg_handler() will call vring_state_changed() anyway.
> >>>>>
> >>>>> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> >>>>> ---
> >>>>>  lib/librte_vhost/vhost_user.c | 3 ---
> >>>>>  1 file changed, 3 deletions(-)
> >>>>>
> >>>>> diff --git a/lib/librte_vhost/vhost_user.c
> b/lib/librte_vhost/vhost_user.c
> >>>>> index 44c0452..8f0eba6 100644
> >>>>> --- a/lib/librte_vhost/vhost_user.c
> >>>>> +++ b/lib/librte_vhost/vhost_user.c
> >>>>> @@ -1918,9 +1918,6 @@ vhost_user_set_vring_kick(struct virtio_net
> >>>> **pdev, struct VhostUserMsg *msg,
> >>>>>  	 */
> >>>>>  	if (!(dev->features & (1ULL <<
> >>>> VHOST_USER_F_PROTOCOL_FEATURES))) {
> >>>>>  		vq->enabled = true;
> >>>>> -		if (dev->notify_ops->vring_state_changed)
> >>>>> -			dev->notify_ops->vring_state_changed(
> >>>>> -				dev->vid, file.index, 1);
> >>>>>  	}
> >>>>>
> >>>>>  	if (vq->ready) {
> >>>>>
> >>>>
> >>>> As replied earlier on v1, I agree the call to vring_state_changed here
> >>>> is not needed. But it might not be enough, there are other cases where
> >>>> you could have issues.
> >>>
> >>> vhost_user_notify_queue_state() can be called in three cases:
> >>> 1. when vq ready status changes, vhost_user_msg_handler() calls it to
> >> notify
> >>> backend. But vhost_user_msg_handler() doesn't take lock before calling
> it.
> >>> So in this case, no deadlock occurs in async register.
> >>>
> >>> 2. if vq->ready is true, vhost_user_set_vring_call() calls it to notify
> backend
> >>> vq is not enabled. Although vhost_user_set_vring_call() is protected by
> lock,
> >>> async register is called only if vq is enabled, so async register will not be
> >> called
> >>> in this case.
> >>>
> >>> 3. If vq->ready is true, vhost_user_set_vring_kick() calls it to notify
> backend
> >>> vq is not enabled. Same as #2, async register is called only when vq is
> >> enabled.
> >>> Even if vhost_user_set_vring_kick() is protected by lock, there is no
> >> deadlock in
> >>> async register, as it will not be called in this case.
> >>>
> >>> In summary,  I think there is no deadlock issue in async register if we
> >>> can remove calling vring_state_change() in vhost_user_set_vring_kick().
> >>
> >>
> >> But unregister one could be called in theory no? Otherwise it would look
> >> unbalanced. At least on disabled notification, the app should make sure
> >> the DMA transfers to and from the vring are stopped before it returns
> >> from the callabck. Otherwise it could lead to undefined behavior.
> >
> > Right, users need to call unregister, but we cannot remove calling
> > vhost_user_notify_queue_state() in case #2 and #3, IMHO. So to
> > avoid deadlock, we recommended users to call async unregister in
> > destroy_device(), instead of on vring disabled notification. Does it
> > make sense to you?
> 
> Calling async unregister in destroy device is fine by me. But I'm more
> concerned about DMA transations not being stopped when the ring becomes
> disabled.
If ring becomes disabled, no more new pkts go into async data path, as
virtio_dev_rx_async_submit() returns if vq->enabled is false.

> 
> I cannot say if you are doing it right, because the vhost example does
> not implement the vring_state_changed callback.
> It is not a problem with the sync datapath as we have the lock
> protection + enabled variable that prevents to process the rings when it
> gets stopped.
> But for the async path, if you have programmed DMA transfers, you need
> to rely on the vring_state_change to block the control path while the
> transfers are cancelled or done.

I am not sure if I understand your concern correctly, but for async data path,
enable variable can prevent it from enqueue new pkts when ring is disabled, as
virtio_dev_rx_async_submit() check enable variable before processing ring;
in addition, lock protection can stop async data path as virtio_dev_rx_async_submit()
acquires lock too. For example, in the case that front-end updates kickfd,
set_vring_kick() will notify backend that vring is stopped by calling
vhost_user_notify_queue_state(). In this case, sync data path will stop as a result of
lock protection, and I think it's the same for async data path, as it acquires lock before
processing ring.

Thanks,
Jiayu

> 
> >>
> >>>>
> >>>> Please add stable and Fixes tag.
> >>>
> >>> Do you suggest to make the patch as a fix for 8639d54563a
> >>> ("vhost: introduce async enqueue registration API")? But the
> >>> thing is that code removed in this patch is not introduced
> >>> by this commit.
> >>
> >> The commit you need to point to is the one introducing the
> >> .vring_state_changed() call.
> >
> > So this patch is still a fix for deadlock on async register? Or it is
> > a fix for unnecessary .vring_state_changed() call?
> 
> You made the point that the vring_state_changed() call was not necessary
> in any case. So it can fix the commit introducing it.
> 
> > Thanks,
> > Jiayu
> >
> >>
> >>> Thanks,
> >>> Jiayu
> >>>>
> >>>> Thanks,
> >>>> Maxime
> >>>
> >

Re: [dpdk-dev] [PATCH v2 3/4] vhost: avoid deadlock on async register

[Maxime Coquelin]

Hi Jiayu,

On 4/16/21 4:19 AM, Hu, Jiayu wrote:
> Hi Maxime,
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Thursday, April 15, 2021 3:09 PM
>> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
>> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Wang, Yinan
>> <yinan.wang@intel.com>; Pai G, Sunil <sunil.pai.g@intel.com>; Jiang, Cheng1
>> <cheng1.jiang@intel.com>
>> Subject: Re: [PATCH v2 3/4] vhost: avoid deadlock on async register
>>
>>
>>
>> On 4/15/21 3:08 AM, Hu, Jiayu wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>> Sent: Wednesday, April 14, 2021 6:09 PM
>>>> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
>>>> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Wang, Yinan
>>>> <yinan.wang@intel.com>; Pai G, Sunil <sunil.pai.g@intel.com>; Jiang,
>> Cheng1
>>>> <cheng1.jiang@intel.com>
>>>> Subject: Re: [PATCH v2 3/4] vhost: avoid deadlock on async register
>>>>
>>>>
>>>>
>>>> On 4/14/21 3:40 AM, Hu, Jiayu wrote:
>>>>> Hi Maxime,
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>>> Sent: Tuesday, April 13, 2021 7:33 PM
>>>>>> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
>>>>>> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Wang, Yinan
>>>>>> <yinan.wang@intel.com>; Pai G, Sunil <sunil.pai.g@intel.com>; Jiang,
>>>> Cheng1
>>>>>> <cheng1.jiang@intel.com>
>>>>>> Subject: Re: [PATCH v2 3/4] vhost: avoid deadlock on async register
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 4/2/21 3:04 PM, Jiayu Hu wrote:
>>>>>>> Users can register async copy device in vring_state_changed(),
>>>>>>> when vhost queue is enabled. However, a deadlock occurs inside
>>>>>>> rte_vhost_async_channel_register(), if
>>>>>> VHOST_USER_F_PROTOCOL_FEATURES
>>>>>>> is not supported, as vhost_user_msg_handler() takes vq->access_lock
>>>>>>> before calling vhost_user_set_vring_kick().
>>>>>>>
>>>>>>> This patch avoids async register deadlock by removing calling
>>>>>>> vring_state_changed() in vhost_user_set_vring_kick(). It's safe
>>>>>>> as vhost_user_msg_handler() will call vring_state_changed() anyway.
>>>>>>>
>>>>>>> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
>>>>>>> ---
>>>>>>>  lib/librte_vhost/vhost_user.c | 3 ---
>>>>>>>  1 file changed, 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/lib/librte_vhost/vhost_user.c
>> b/lib/librte_vhost/vhost_user.c
>>>>>>> index 44c0452..8f0eba6 100644
>>>>>>> --- a/lib/librte_vhost/vhost_user.c
>>>>>>> +++ b/lib/librte_vhost/vhost_user.c
>>>>>>> @@ -1918,9 +1918,6 @@ vhost_user_set_vring_kick(struct virtio_net
>>>>>> **pdev, struct VhostUserMsg *msg,
>>>>>>>  	 */
>>>>>>>  	if (!(dev->features & (1ULL <<
>>>>>> VHOST_USER_F_PROTOCOL_FEATURES))) {
>>>>>>>  		vq->enabled = true;
>>>>>>> -		if (dev->notify_ops->vring_state_changed)
>>>>>>> -			dev->notify_ops->vring_state_changed(
>>>>>>> -				dev->vid, file.index, 1);
>>>>>>>  	}
>>>>>>>
>>>>>>>  	if (vq->ready) {
>>>>>>>
>>>>>>
>>>>>> As replied earlier on v1, I agree the call to vring_state_changed here
>>>>>> is not needed. But it might not be enough, there are other cases where
>>>>>> you could have issues.
>>>>>
>>>>> vhost_user_notify_queue_state() can be called in three cases:
>>>>> 1. when vq ready status changes, vhost_user_msg_handler() calls it to
>>>> notify
>>>>> backend. But vhost_user_msg_handler() doesn't take lock before calling
>> it.
>>>>> So in this case, no deadlock occurs in async register.
>>>>>
>>>>> 2. if vq->ready is true, vhost_user_set_vring_call() calls it to notify
>> backend
>>>>> vq is not enabled. Although vhost_user_set_vring_call() is protected by
>> lock,
>>>>> async register is called only if vq is enabled, so async register will not be
>>>> called
>>>>> in this case.
>>>>>
>>>>> 3. If vq->ready is true, vhost_user_set_vring_kick() calls it to notify
>> backend
>>>>> vq is not enabled. Same as #2, async register is called only when vq is
>>>> enabled.
>>>>> Even if vhost_user_set_vring_kick() is protected by lock, there is no
>>>> deadlock in
>>>>> async register, as it will not be called in this case.
>>>>>
>>>>> In summary,  I think there is no deadlock issue in async register if we
>>>>> can remove calling vring_state_change() in vhost_user_set_vring_kick().
>>>>
>>>>
>>>> But unregister one could be called in theory no? Otherwise it would look
>>>> unbalanced. At least on disabled notification, the app should make sure
>>>> the DMA transfers to and from the vring are stopped before it returns
>>>> from the callabck. Otherwise it could lead to undefined behavior.
>>>
>>> Right, users need to call unregister, but we cannot remove calling
>>> vhost_user_notify_queue_state() in case #2 and #3, IMHO. So to
>>> avoid deadlock, we recommended users to call async unregister in
>>> destroy_device(), instead of on vring disabled notification. Does it
>>> make sense to you?
>>
>> Calling async unregister in destroy device is fine by me. But I'm more
>> concerned about DMA transations not being stopped when the ring becomes
>> disabled.
> If ring becomes disabled, no more new pkts go into async data path, as
> virtio_dev_rx_async_submit() returns if vq->enabled is false.
> 
>>
>> I cannot say if you are doing it right, because the vhost example does
>> not implement the vring_state_changed callback.
>> It is not a problem with the sync datapath as we have the lock
>> protection + enabled variable that prevents to process the rings when it
>> gets stopped.
>> But for the async path, if you have programmed DMA transfers, you need
>> to rely on the vring_state_change to block the control path while the
>> transfers are cancelled or done.
> 
> I am not sure if I understand your concern correctly, but for async data path,
> enable variable can prevent it from enqueue new pkts when ring is disabled, as
> virtio_dev_rx_async_submit() check enable variable before processing ring;
> in addition, lock protection can stop async data path as virtio_dev_rx_async_submit()
> acquires lock too. For example, in the case that front-end updates kickfd,
> set_vring_kick() will notify backend that vring is stopped by calling
> vhost_user_notify_queue_state(). In this case, sync data path will stop as a result of
> lock protection, and I think it's the same for async data path, as it acquires lock before
> processing ring.


What about memory hotplug happening while the DMA transfers are on-going
for example?

In this case, the lock is enough for sync datapath, but is not for async
one.

If a VHOST_USER_SET_MEM_TABLE request is received while
virtio_dev_rx_async_submit(), it will be blocked until
virtio_dev_rx_async_submit() returns but the DMA transfers may not be
finished yet.
When unblocked the control thread will call vhost_user_set_mem_table(),
which will mnumap the current memory regions before mmaping the new
ones while DMA transfers are on-going.

To fix this, we need to call the vring_state_changed callback for all
the virtqueues with disable state. in your app, you need to stop DMA
transfers when disabled state notification happens, or block the
callback while the transfer is done.

Maxime

> Thanks,
> Jiayu
> 
>>
>>>>
>>>>>>
>>>>>> Please add stable and Fixes tag.
>>>>>
>>>>> Do you suggest to make the patch as a fix for 8639d54563a
>>>>> ("vhost: introduce async enqueue registration API")? But the
>>>>> thing is that code removed in this patch is not introduced
>>>>> by this commit.
>>>>
>>>> The commit you need to point to is the one introducing the
>>>> .vring_state_changed() call.
>>>
>>> So this patch is still a fix for deadlock on async register? Or it is
>>> a fix for unnecessary .vring_state_changed() call?
>>
>> You made the point that the vring_state_changed() call was not necessary
>> in any case. So it can fix the commit introducing it.
>>
>>> Thanks,
>>> Jiayu
>>>
>>>>
>>>>> Thanks,
>>>>> Jiayu
>>>>>>
>>>>>> Thanks,
>>>>>> Maxime
>>>>>
>>>
> 

Re: [dpdk-dev] [PATCH v2 3/4] vhost: avoid deadlock on async register

[Hu, Jiayu]

Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Friday, April 16, 2021 2:35 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Wang, Yinan
> <yinan.wang@intel.com>; Pai G, Sunil <sunil.pai.g@intel.com>; Jiang, Cheng1
> <cheng1.jiang@intel.com>
> Subject: Re: [PATCH v2 3/4] vhost: avoid deadlock on async register
> 
> Hi Jiayu,
> 
> On 4/16/21 4:19 AM, Hu, Jiayu wrote:
> > Hi Maxime,
> >>>> On 4/14/21 3:40 AM, Hu, Jiayu wrote:
> >>>>> Hi Maxime,
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>>>> Sent: Tuesday, April 13, 2021 7:33 PM
> >>>>>> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
> >>>>>> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Wang, Yinan
> >>>>>> <yinan.wang@intel.com>; Pai G, Sunil <sunil.pai.g@intel.com>; Jiang,
> >>>> Cheng1
> >>>>>> <cheng1.jiang@intel.com>
> >>>>>> Subject: Re: [PATCH v2 3/4] vhost: avoid deadlock on async register
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 4/2/21 3:04 PM, Jiayu Hu wrote:
> >>>>>>> Users can register async copy device in vring_state_changed(),
> >>>>>>> when vhost queue is enabled. However, a deadlock occurs inside
> >>>>>>> rte_vhost_async_channel_register(), if
> >>>>>> VHOST_USER_F_PROTOCOL_FEATURES
> >>>>>>> is not supported, as vhost_user_msg_handler() takes vq-
> >access_lock
> >>>>>>> before calling vhost_user_set_vring_kick().
> >>>>>>>
> >>>>>>> This patch avoids async register deadlock by removing calling
> >>>>>>> vring_state_changed() in vhost_user_set_vring_kick(). It's safe
> >>>>>>> as vhost_user_msg_handler() will call vring_state_changed()
> anyway.
> >>>>>>>
> >>>>>>> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> >>>>>>> ---
> >>>>>>>  lib/librte_vhost/vhost_user.c | 3 ---
> >>>>>>>  1 file changed, 3 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/lib/librte_vhost/vhost_user.c
> >> b/lib/librte_vhost/vhost_user.c
> >>>>>>> index 44c0452..8f0eba6 100644
> >>>>>>> --- a/lib/librte_vhost/vhost_user.c
> >>>>>>> +++ b/lib/librte_vhost/vhost_user.c
> >>>>>>> @@ -1918,9 +1918,6 @@ vhost_user_set_vring_kick(struct
> virtio_net
> >>>>>> **pdev, struct VhostUserMsg *msg,
> >>>>>>>  	 */
> >>>>>>>  	if (!(dev->features & (1ULL <<
> >>>>>> VHOST_USER_F_PROTOCOL_FEATURES))) {
> >>>>>>>  		vq->enabled = true;
> >>>>>>> -		if (dev->notify_ops->vring_state_changed)
> >>>>>>> -			dev->notify_ops->vring_state_changed(
> >>>>>>> -				dev->vid, file.index, 1);
> >>>>>>>  	}
> >>>>>>>
> >>>>>>>  	if (vq->ready) {
> >>>>>>>
> >>>>>>
> >>>>>> As replied earlier on v1, I agree the call to vring_state_changed here
> >>>>>> is not needed. But it might not be enough, there are other cases
> where
> >>>>>> you could have issues.
> >>>>>
> >>>>> vhost_user_notify_queue_state() can be called in three cases:
> >>>>> 1. when vq ready status changes, vhost_user_msg_handler() calls it to
> >>>> notify
> >>>>> backend. But vhost_user_msg_handler() doesn't take lock before
> calling
> >> it.
> >>>>> So in this case, no deadlock occurs in async register.
> >>>>>
> >>>>> 2. if vq->ready is true, vhost_user_set_vring_call() calls it to notify
> >> backend
> >>>>> vq is not enabled. Although vhost_user_set_vring_call() is protected by
> >> lock,
> >>>>> async register is called only if vq is enabled, so async register will not
> be
> >>>> called
> >>>>> in this case.
> >>>>>
> >>>>> 3. If vq->ready is true, vhost_user_set_vring_kick() calls it to notify
> >> backend
> >>>>> vq is not enabled. Same as #2, async register is called only when vq is
> >>>> enabled.
> >>>>> Even if vhost_user_set_vring_kick() is protected by lock, there is no
> >>>> deadlock in
> >>>>> async register, as it will not be called in this case.
> >>>>>
> >>>>> In summary,  I think there is no deadlock issue in async register if we
> >>>>> can remove calling vring_state_change() in
> vhost_user_set_vring_kick().
> >>>>
> >>>>
> >>>> But unregister one could be called in theory no? Otherwise it would
> look
> >>>> unbalanced. At least on disabled notification, the app should make sure
> >>>> the DMA transfers to and from the vring are stopped before it returns
> >>>> from the callabck. Otherwise it could lead to undefined behavior.
> >>>
> >>> Right, users need to call unregister, but we cannot remove calling
> >>> vhost_user_notify_queue_state() in case #2 and #3, IMHO. So to
> >>> avoid deadlock, we recommended users to call async unregister in
> >>> destroy_device(), instead of on vring disabled notification. Does it
> >>> make sense to you?
> >>
> >> Calling async unregister in destroy device is fine by me. But I'm more
> >> concerned about DMA transations not being stopped when the ring
> becomes
> >> disabled.
> > If ring becomes disabled, no more new pkts go into async data path, as
> > virtio_dev_rx_async_submit() returns if vq->enabled is false.
> >
> >>
> >> I cannot say if you are doing it right, because the vhost example does
> >> not implement the vring_state_changed callback.
> >> It is not a problem with the sync datapath as we have the lock
> >> protection + enabled variable that prevents to process the rings when it
> >> gets stopped.
> >> But for the async path, if you have programmed DMA transfers, you need
> >> to rely on the vring_state_change to block the control path while the
> >> transfers are cancelled or done.
> >
> > I am not sure if I understand your concern correctly, but for async data
> path,
> > enable variable can prevent it from enqueue new pkts when ring is
> disabled, as
> > virtio_dev_rx_async_submit() check enable variable before processing ring;
> > in addition, lock protection can stop async data path as
> virtio_dev_rx_async_submit()
> > acquires lock too. For example, in the case that front-end updates kickfd,
> > set_vring_kick() will notify backend that vring is stopped by calling
> > vhost_user_notify_queue_state(). In this case, sync data path will stop as a
> result of
> > lock protection, and I think it's the same for async data path, as it acquires
> lock before
> > processing ring.
> 
> 
> What about memory hotplug happening while the DMA transfers are on-
> going
> for example?
> 
> In this case, the lock is enough for sync datapath, but is not for async
> one.
> 
> If a VHOST_USER_SET_MEM_TABLE request is received while
> virtio_dev_rx_async_submit(), it will be blocked until
> virtio_dev_rx_async_submit() returns but the DMA transfers may not be
> finished yet.
> When unblocked the control thread will call vhost_user_set_mem_table(),
> which will mnumap the current memory regions before mmaping the new
> ones while DMA transfers are on-going.
> 
> To fix this, we need to call the vring_state_changed callback for all
> the virtqueues with disable state. in your app, you need to stop DMA
> transfers when disabled state notification happens, or block the
> callback while the transfer is done.

Let me summarize the problem:
when frontend memory is hot-plug, host application needs to stop
DMA transfer for all virtqueues, which is done by emptying in-flight
DMA copies and unregister async inside vring_state_changed(). But
vhost library takes lock before entering vring_state_changed(), and
async unregister and rte_vhost_poll_enqueue_completed() both
acquire lock, so deadlock occurs inside rte_vhost_poll_enqueue_
completed() or async unregister. (please correct me if am wrong)

There may be two possible solutions:
1. remove lock before calling vring_state_change() in vhost library;
2. provide a new callback without acquiring lock for DMA accelerated
case. The callback is used to notify backend that you need to stop
DMA transfer.

Thanks,
Jiayu

Re: [dpdk-dev] [PATCH v2 3/4] vhost: avoid deadlock on async register

[Maxime Coquelin]

On 4/16/21 10:18 AM, Hu, Jiayu wrote:
> Hi Maxime,
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Friday, April 16, 2021 2:35 PM
>> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
>> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Wang, Yinan
>> <yinan.wang@intel.com>; Pai G, Sunil <sunil.pai.g@intel.com>; Jiang, Cheng1
>> <cheng1.jiang@intel.com>
>> Subject: Re: [PATCH v2 3/4] vhost: avoid deadlock on async register
>>
>> Hi Jiayu,
>>
>> On 4/16/21 4:19 AM, Hu, Jiayu wrote:
>>> Hi Maxime,
>>>>>> On 4/14/21 3:40 AM, Hu, Jiayu wrote:
>>>>>>> Hi Maxime,
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>>>>> Sent: Tuesday, April 13, 2021 7:33 PM
>>>>>>>> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
>>>>>>>> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Wang, Yinan
>>>>>>>> <yinan.wang@intel.com>; Pai G, Sunil <sunil.pai.g@intel.com>; Jiang,
>>>>>> Cheng1
>>>>>>>> <cheng1.jiang@intel.com>
>>>>>>>> Subject: Re: [PATCH v2 3/4] vhost: avoid deadlock on async register
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 4/2/21 3:04 PM, Jiayu Hu wrote:
>>>>>>>>> Users can register async copy device in vring_state_changed(),
>>>>>>>>> when vhost queue is enabled. However, a deadlock occurs inside
>>>>>>>>> rte_vhost_async_channel_register(), if
>>>>>>>> VHOST_USER_F_PROTOCOL_FEATURES
>>>>>>>>> is not supported, as vhost_user_msg_handler() takes vq-
>>> access_lock
>>>>>>>>> before calling vhost_user_set_vring_kick().
>>>>>>>>>
>>>>>>>>> This patch avoids async register deadlock by removing calling
>>>>>>>>> vring_state_changed() in vhost_user_set_vring_kick(). It's safe
>>>>>>>>> as vhost_user_msg_handler() will call vring_state_changed()
>> anyway.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
>>>>>>>>> ---
>>>>>>>>>  lib/librte_vhost/vhost_user.c | 3 ---
>>>>>>>>>  1 file changed, 3 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/lib/librte_vhost/vhost_user.c
>>>> b/lib/librte_vhost/vhost_user.c
>>>>>>>>> index 44c0452..8f0eba6 100644
>>>>>>>>> --- a/lib/librte_vhost/vhost_user.c
>>>>>>>>> +++ b/lib/librte_vhost/vhost_user.c
>>>>>>>>> @@ -1918,9 +1918,6 @@ vhost_user_set_vring_kick(struct
>> virtio_net
>>>>>>>> **pdev, struct VhostUserMsg *msg,
>>>>>>>>>  	 */
>>>>>>>>>  	if (!(dev->features & (1ULL <<
>>>>>>>> VHOST_USER_F_PROTOCOL_FEATURES))) {
>>>>>>>>>  		vq->enabled = true;
>>>>>>>>> -		if (dev->notify_ops->vring_state_changed)
>>>>>>>>> -			dev->notify_ops->vring_state_changed(
>>>>>>>>> -				dev->vid, file.index, 1);
>>>>>>>>>  	}
>>>>>>>>>
>>>>>>>>>  	if (vq->ready) {
>>>>>>>>>
>>>>>>>>
>>>>>>>> As replied earlier on v1, I agree the call to vring_state_changed here
>>>>>>>> is not needed. But it might not be enough, there are other cases
>> where
>>>>>>>> you could have issues.
>>>>>>>
>>>>>>> vhost_user_notify_queue_state() can be called in three cases:
>>>>>>> 1. when vq ready status changes, vhost_user_msg_handler() calls it to
>>>>>> notify
>>>>>>> backend. But vhost_user_msg_handler() doesn't take lock before
>> calling
>>>> it.
>>>>>>> So in this case, no deadlock occurs in async register.
>>>>>>>
>>>>>>> 2. if vq->ready is true, vhost_user_set_vring_call() calls it to notify
>>>> backend
>>>>>>> vq is not enabled. Although vhost_user_set_vring_call() is protected by
>>>> lock,
>>>>>>> async register is called only if vq is enabled, so async register will not
>> be
>>>>>> called
>>>>>>> in this case.
>>>>>>>
>>>>>>> 3. If vq->ready is true, vhost_user_set_vring_kick() calls it to notify
>>>> backend
>>>>>>> vq is not enabled. Same as #2, async register is called only when vq is
>>>>>> enabled.
>>>>>>> Even if vhost_user_set_vring_kick() is protected by lock, there is no
>>>>>> deadlock in
>>>>>>> async register, as it will not be called in this case.
>>>>>>>
>>>>>>> In summary,  I think there is no deadlock issue in async register if we
>>>>>>> can remove calling vring_state_change() in
>> vhost_user_set_vring_kick().
>>>>>>
>>>>>>
>>>>>> But unregister one could be called in theory no? Otherwise it would
>> look
>>>>>> unbalanced. At least on disabled notification, the app should make sure
>>>>>> the DMA transfers to and from the vring are stopped before it returns
>>>>>> from the callabck. Otherwise it could lead to undefined behavior.
>>>>>
>>>>> Right, users need to call unregister, but we cannot remove calling
>>>>> vhost_user_notify_queue_state() in case #2 and #3, IMHO. So to
>>>>> avoid deadlock, we recommended users to call async unregister in
>>>>> destroy_device(), instead of on vring disabled notification. Does it
>>>>> make sense to you?
>>>>
>>>> Calling async unregister in destroy device is fine by me. But I'm more
>>>> concerned about DMA transations not being stopped when the ring
>> becomes
>>>> disabled.
>>> If ring becomes disabled, no more new pkts go into async data path, as
>>> virtio_dev_rx_async_submit() returns if vq->enabled is false.
>>>
>>>>
>>>> I cannot say if you are doing it right, because the vhost example does
>>>> not implement the vring_state_changed callback.
>>>> It is not a problem with the sync datapath as we have the lock
>>>> protection + enabled variable that prevents to process the rings when it
>>>> gets stopped.
>>>> But for the async path, if you have programmed DMA transfers, you need
>>>> to rely on the vring_state_change to block the control path while the
>>>> transfers are cancelled or done.
>>>
>>> I am not sure if I understand your concern correctly, but for async data
>> path,
>>> enable variable can prevent it from enqueue new pkts when ring is
>> disabled, as
>>> virtio_dev_rx_async_submit() check enable variable before processing ring;
>>> in addition, lock protection can stop async data path as
>> virtio_dev_rx_async_submit()
>>> acquires lock too. For example, in the case that front-end updates kickfd,
>>> set_vring_kick() will notify backend that vring is stopped by calling
>>> vhost_user_notify_queue_state(). In this case, sync data path will stop as a
>> result of
>>> lock protection, and I think it's the same for async data path, as it acquires
>> lock before
>>> processing ring.
>>
>>
>> What about memory hotplug happening while the DMA transfers are on-
>> going
>> for example?
>>
>> In this case, the lock is enough for sync datapath, but is not for async
>> one.
>>
>> If a VHOST_USER_SET_MEM_TABLE request is received while
>> virtio_dev_rx_async_submit(), it will be blocked until
>> virtio_dev_rx_async_submit() returns but the DMA transfers may not be
>> finished yet.
>> When unblocked the control thread will call vhost_user_set_mem_table(),
>> which will mnumap the current memory regions before mmaping the new
>> ones while DMA transfers are on-going.
>>
>> To fix this, we need to call the vring_state_changed callback for all
>> the virtqueues with disable state. in your app, you need to stop DMA
>> transfers when disabled state notification happens, or block the
>> callback while the transfer is done.
> 
> Let me summarize the problem:
> when frontend memory is hot-plug, host application needs to stop
> DMA transfer for all virtqueues, which is done by emptying in-flight
> DMA copies and unregister async inside vring_state_changed().

I don't think unregistering async channels is mandatory.
On disable notification, the callback has to block until all on-going
DMA transfers are done. New transfers won't be schedules since
virtio_dev_rx_async_submit() will be blocked by the lock while the
memory regions update is being done.

> But
> vhost library takes lock before entering vring_state_changed(), and
> async unregister and rte_vhost_poll_enqueue_completed() both
> acquire lock, so deadlock occurs inside rte_vhost_poll_enqueue_
> completed() or async unregister. (please correct me if am wrong)
> 
> There may be two possible solutions:
> 1. remove lock before calling vring_state_change() in vhost library;

I don't think so. If you release the lock in vring_state_change(), it
will enable the app to enqueue/dequeue descriptors in the middle of
handling the request.

> 2. provide a new callback without acquiring lock for DMA accelerated
> case. The callback is used to notify backend that you need to stop
> DMA transfer.

No, in your app just wait for DMA transfers to be finished before
returning from the callback.

Please implement vring_state_changed callback in Vhost example, it is
now mandatory with async datapath, and it will help to have an example
on what real applications should do.

Thanks,
Maxime

> Thanks,
> Jiayu
> 

Re: [dpdk-dev] [PATCH v2 3/4] vhost: avoid deadlock on async register

[Hu, Jiayu]

Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Friday, April 16, 2021 4:34 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Wang, Yinan
> <yinan.wang@intel.com>; Pai G, Sunil <sunil.pai.g@intel.com>; Jiang, Cheng1
> <cheng1.jiang@intel.com>
> Subject: Re: [PATCH v2 3/4] vhost: avoid deadlock on async register
> 
> 
> 
> On 4/16/21 10:18 AM, Hu, Jiayu wrote:
> > Hi Maxime,
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Sent: Friday, April 16, 2021 2:35 PM
> >> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
> >> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Wang, Yinan
> >> <yinan.wang@intel.com>; Pai G, Sunil <sunil.pai.g@intel.com>; Jiang,
> Cheng1
> >> <cheng1.jiang@intel.com>
> >> Subject: Re: [PATCH v2 3/4] vhost: avoid deadlock on async register
> >>
> >> Hi Jiayu,
> >>
> >> On 4/16/21 4:19 AM, Hu, Jiayu wrote:
> >>> Hi Maxime,
> >>>>>> On 4/14/21 3:40 AM, Hu, Jiayu wrote:
> >>>>>>> Hi Maxime,
> >>>>>>>
> >>>>>>>> -----Original Message-----
> >>>>>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>>>>>> Sent: Tuesday, April 13, 2021 7:33 PM
> >>>>>>>> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
> >>>>>>>> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Wang, Yinan
> >>>>>>>> <yinan.wang@intel.com>; Pai G, Sunil <sunil.pai.g@intel.com>;
> Jiang,
> >>>>>> Cheng1
> >>>>>>>> <cheng1.jiang@intel.com>
> >>>>>>>> Subject: Re: [PATCH v2 3/4] vhost: avoid deadlock on async register
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On 4/2/21 3:04 PM, Jiayu Hu wrote:
> >>>>>>>>> Users can register async copy device in vring_state_changed(),
> >>>>>>>>> when vhost queue is enabled. However, a deadlock occurs inside
> >>>>>>>>> rte_vhost_async_channel_register(), if
> >>>>>>>> VHOST_USER_F_PROTOCOL_FEATURES
> >>>>>>>>> is not supported, as vhost_user_msg_handler() takes vq-
> >>> access_lock
> >>>>>>>>> before calling vhost_user_set_vring_kick().
> >>>>>>>>>
> >>>>>>>>> This patch avoids async register deadlock by removing calling
> >>>>>>>>> vring_state_changed() in vhost_user_set_vring_kick(). It's safe
> >>>>>>>>> as vhost_user_msg_handler() will call vring_state_changed()
> >> anyway.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> >>>>>>>>> ---
> >>>>>>>>>  lib/librte_vhost/vhost_user.c | 3 ---
> >>>>>>>>>  1 file changed, 3 deletions(-)
> >>>>>>>>>
> >>>>>>>>> diff --git a/lib/librte_vhost/vhost_user.c
> >>>> b/lib/librte_vhost/vhost_user.c
> >>>>>>>>> index 44c0452..8f0eba6 100644
> >>>>>>>>> --- a/lib/librte_vhost/vhost_user.c
> >>>>>>>>> +++ b/lib/librte_vhost/vhost_user.c
> >>>>>>>>> @@ -1918,9 +1918,6 @@ vhost_user_set_vring_kick(struct
> >> virtio_net
> >>>>>>>> **pdev, struct VhostUserMsg *msg,
> >>>>>>>>>  	 */
> >>>>>>>>>  	if (!(dev->features & (1ULL <<
> >>>>>>>> VHOST_USER_F_PROTOCOL_FEATURES))) {
> >>>>>>>>>  		vq->enabled = true;
> >>>>>>>>> -		if (dev->notify_ops->vring_state_changed)
> >>>>>>>>> -			dev->notify_ops->vring_state_changed(
> >>>>>>>>> -				dev->vid, file.index, 1);
> >>>>>>>>>  	}
> >>>>>>>>>
> >>>>>>>>>  	if (vq->ready) {
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> As replied earlier on v1, I agree the call to vring_state_changed
> here
> >>>>>>>> is not needed. But it might not be enough, there are other cases
> >> where
> >>>>>>>> you could have issues.
> >>>>>>>
> >>>>>>> vhost_user_notify_queue_state() can be called in three cases:
> >>>>>>> 1. when vq ready status changes, vhost_user_msg_handler() calls it
> to
> >>>>>> notify
> >>>>>>> backend. But vhost_user_msg_handler() doesn't take lock before
> >> calling
> >>>> it.
> >>>>>>> So in this case, no deadlock occurs in async register.
> >>>>>>>
> >>>>>>> 2. if vq->ready is true, vhost_user_set_vring_call() calls it to notify
> >>>> backend
> >>>>>>> vq is not enabled. Although vhost_user_set_vring_call() is protected
> by
> >>>> lock,
> >>>>>>> async register is called only if vq is enabled, so async register will not
> >> be
> >>>>>> called
> >>>>>>> in this case.
> >>>>>>>
> >>>>>>> 3. If vq->ready is true, vhost_user_set_vring_kick() calls it to notify
> >>>> backend
> >>>>>>> vq is not enabled. Same as #2, async register is called only when vq
> is
> >>>>>> enabled.
> >>>>>>> Even if vhost_user_set_vring_kick() is protected by lock, there is no
> >>>>>> deadlock in
> >>>>>>> async register, as it will not be called in this case.
> >>>>>>>
> >>>>>>> In summary,  I think there is no deadlock issue in async register if we
> >>>>>>> can remove calling vring_state_change() in
> >> vhost_user_set_vring_kick().
> >>>>>>
> >>>>>>
> >>>>>> But unregister one could be called in theory no? Otherwise it would
> >> look
> >>>>>> unbalanced. At least on disabled notification, the app should make
> sure
> >>>>>> the DMA transfers to and from the vring are stopped before it
> returns
> >>>>>> from the callabck. Otherwise it could lead to undefined behavior.
> >>>>>
> >>>>> Right, users need to call unregister, but we cannot remove calling
> >>>>> vhost_user_notify_queue_state() in case #2 and #3, IMHO. So to
> >>>>> avoid deadlock, we recommended users to call async unregister in
> >>>>> destroy_device(), instead of on vring disabled notification. Does it
> >>>>> make sense to you?
> >>>>
> >>>> Calling async unregister in destroy device is fine by me. But I'm more
> >>>> concerned about DMA transations not being stopped when the ring
> >> becomes
> >>>> disabled.
> >>> If ring becomes disabled, no more new pkts go into async data path, as
> >>> virtio_dev_rx_async_submit() returns if vq->enabled is false.
> >>>
> >>>>
> >>>> I cannot say if you are doing it right, because the vhost example does
> >>>> not implement the vring_state_changed callback.
> >>>> It is not a problem with the sync datapath as we have the lock
> >>>> protection + enabled variable that prevents to process the rings when it
> >>>> gets stopped.
> >>>> But for the async path, if you have programmed DMA transfers, you
> need
> >>>> to rely on the vring_state_change to block the control path while the
> >>>> transfers are cancelled or done.
> >>>
> >>> I am not sure if I understand your concern correctly, but for async data
> >> path,
> >>> enable variable can prevent it from enqueue new pkts when ring is
> >> disabled, as
> >>> virtio_dev_rx_async_submit() check enable variable before processing
> ring;
> >>> in addition, lock protection can stop async data path as
> >> virtio_dev_rx_async_submit()
> >>> acquires lock too. For example, in the case that front-end updates kickfd,
> >>> set_vring_kick() will notify backend that vring is stopped by calling
> >>> vhost_user_notify_queue_state(). In this case, sync data path will stop as
> a
> >> result of
> >>> lock protection, and I think it's the same for async data path, as it
> acquires
> >> lock before
> >>> processing ring.
> >>
> >>
> >> What about memory hotplug happening while the DMA transfers are on-
> >> going
> >> for example?
> >>
> >> In this case, the lock is enough for sync datapath, but is not for async
> >> one.
> >>
> >> If a VHOST_USER_SET_MEM_TABLE request is received while
> >> virtio_dev_rx_async_submit(), it will be blocked until
> >> virtio_dev_rx_async_submit() returns but the DMA transfers may not be
> >> finished yet.
> >> When unblocked the control thread will call vhost_user_set_mem_table(),
> >> which will mnumap the current memory regions before mmaping the new
> >> ones while DMA transfers are on-going.
> >>
> >> To fix this, we need to call the vring_state_changed callback for all
> >> the virtqueues with disable state. in your app, you need to stop DMA
> >> transfers when disabled state notification happens, or block the
> >> callback while the transfer is done.
> >
> > Let me summarize the problem:
> > when frontend memory is hot-plug, host application needs to stop
> > DMA transfer for all virtqueues, which is done by emptying in-flight
> > DMA copies and unregister async inside vring_state_changed().
> 
> I don't think unregistering async channels is mandatory.
> On disable notification, the callback has to block until all on-going
> DMA transfers are done. New transfers won't be schedules since
> virtio_dev_rx_async_submit() will be blocked by the lock while the
> memory regions update is being done.

Yes, unregister is not a must, but the problem is that
rte_vhost_poll_enqueue_completed() takes lock too.
In VM memory hot-plug case, emptying in-flight pkts in
vring_state_changed() may be OK, as it is called by
vhost_user_msg_handler() (please correct me if I am wrong),
which doesn't take lock. But if it is called inside set_vring_kick()/_call(),
a deadlock occurs in rte_vhost_poll_enqueue_completed(), even if user
app doesn't call async unregister API.

> 
> > But
> > vhost library takes lock before entering vring_state_changed(), and
> > async unregister and rte_vhost_poll_enqueue_completed() both
> > acquire lock, so deadlock occurs inside rte_vhost_poll_enqueue_
> > completed() or async unregister. (please correct me if am wrong)
> >
> > There may be two possible solutions:
> > 1. remove lock before calling vring_state_change() in vhost library;
> 
> I don't think so. If you release the lock in vring_state_change(), it
> will enable the app to enqueue/dequeue descriptors in the middle of
> handling the request.

Sorry, I didn't express my opinion clearly. I mean remove the following the
code in set_vring_kick() and set_vring_call():
-       if (vq->ready) {
-               vq->ready = false;
-               vhost_user_notify_queue_state(dev, file.index, 0);
-       }

IMHO, in the both cases, the purpose of calling vhost_user_notify_queue_state()
is to tell backend vring is not ready to use as a result of callfd/kickfd update;
after that, vhost_user_msg_handler() calls it again to notify backend of vring
ready to process. But there is lock protection before entering
set_vring_kick()/_call(), so data path threads will not process vring during updating
callfd/kickfd anyway. After updating callfd/kickfd, I think data path threads are already
safe to start to process vring, so there is no need for a ready notification in
vhost_user_msg_hander().

BTW, lock protection for vhost_user_notify_queue_state() is only in
set_vring_kick()/_call(), but not in vhost_msg_handler().

Thanks,
Jiayu
> 
> > 2. provide a new callback without acquiring lock for DMA accelerated
> > case. The callback is used to notify backend that you need to stop
> > DMA transfer.
> 
> No, in your app just wait for DMA transfers to be finished before
> returning from the callback.
> 
> Please implement vring_state_changed callback in Vhost example, it is
> now mandatory with async datapath, and it will help to have an example
> on what real applications should do.
> 
> Thanks,
> Maxime
> 
> > Thanks,
> > Jiayu
> >

Re: [dpdk-dev] [PATCH v2 3/4] vhost: avoid deadlock on async register

[Maxime Coquelin]

On 4/19/21 6:10 AM, Hu, Jiayu wrote:
> Hi Maxime,
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Friday, April 16, 2021 4:34 PM
>> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
>> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Wang, Yinan
>> <yinan.wang@intel.com>; Pai G, Sunil <sunil.pai.g@intel.com>; Jiang, Cheng1
>> <cheng1.jiang@intel.com>
>> Subject: Re: [PATCH v2 3/4] vhost: avoid deadlock on async register
>>
>>
>>
>> On 4/16/21 10:18 AM, Hu, Jiayu wrote:
>>> Hi Maxime,
>>>
>>>> -----Original Message-----
>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>> Sent: Friday, April 16, 2021 2:35 PM
>>>> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
>>>> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Wang, Yinan
>>>> <yinan.wang@intel.com>; Pai G, Sunil <sunil.pai.g@intel.com>; Jiang,
>> Cheng1
>>>> <cheng1.jiang@intel.com>
>>>> Subject: Re: [PATCH v2 3/4] vhost: avoid deadlock on async register
>>>>
>>>> Hi Jiayu,
>>>>
>>>> On 4/16/21 4:19 AM, Hu, Jiayu wrote:
>>>>> Hi Maxime,
>>>>>>>> On 4/14/21 3:40 AM, Hu, Jiayu wrote:
>>>>>>>>> Hi Maxime,
>>>>>>>>>
>>>>>>>>>> -----Original Message-----
>>>>>>>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>>>>>>> Sent: Tuesday, April 13, 2021 7:33 PM
>>>>>>>>>> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
>>>>>>>>>> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Wang, Yinan
>>>>>>>>>> <yinan.wang@intel.com>; Pai G, Sunil <sunil.pai.g@intel.com>;
>> Jiang,
>>>>>>>> Cheng1
>>>>>>>>>> <cheng1.jiang@intel.com>
>>>>>>>>>> Subject: Re: [PATCH v2 3/4] vhost: avoid deadlock on async register
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 4/2/21 3:04 PM, Jiayu Hu wrote:
>>>>>>>>>>> Users can register async copy device in vring_state_changed(),
>>>>>>>>>>> when vhost queue is enabled. However, a deadlock occurs inside
>>>>>>>>>>> rte_vhost_async_channel_register(), if
>>>>>>>>>> VHOST_USER_F_PROTOCOL_FEATURES
>>>>>>>>>>> is not supported, as vhost_user_msg_handler() takes vq-
>>>>> access_lock
>>>>>>>>>>> before calling vhost_user_set_vring_kick().
>>>>>>>>>>>
>>>>>>>>>>> This patch avoids async register deadlock by removing calling
>>>>>>>>>>> vring_state_changed() in vhost_user_set_vring_kick(). It's safe
>>>>>>>>>>> as vhost_user_msg_handler() will call vring_state_changed()
>>>> anyway.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
>>>>>>>>>>> ---
>>>>>>>>>>>  lib/librte_vhost/vhost_user.c | 3 ---
>>>>>>>>>>>  1 file changed, 3 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/lib/librte_vhost/vhost_user.c
>>>>>> b/lib/librte_vhost/vhost_user.c
>>>>>>>>>>> index 44c0452..8f0eba6 100644
>>>>>>>>>>> --- a/lib/librte_vhost/vhost_user.c
>>>>>>>>>>> +++ b/lib/librte_vhost/vhost_user.c
>>>>>>>>>>> @@ -1918,9 +1918,6 @@ vhost_user_set_vring_kick(struct
>>>> virtio_net
>>>>>>>>>> **pdev, struct VhostUserMsg *msg,
>>>>>>>>>>>  	 */
>>>>>>>>>>>  	if (!(dev->features & (1ULL <<
>>>>>>>>>> VHOST_USER_F_PROTOCOL_FEATURES))) {
>>>>>>>>>>>  		vq->enabled = true;
>>>>>>>>>>> -		if (dev->notify_ops->vring_state_changed)
>>>>>>>>>>> -			dev->notify_ops->vring_state_changed(
>>>>>>>>>>> -				dev->vid, file.index, 1);
>>>>>>>>>>>  	}
>>>>>>>>>>>
>>>>>>>>>>>  	if (vq->ready) {
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> As replied earlier on v1, I agree the call to vring_state_changed
>> here
>>>>>>>>>> is not needed. But it might not be enough, there are other cases
>>>> where
>>>>>>>>>> you could have issues.
>>>>>>>>>
>>>>>>>>> vhost_user_notify_queue_state() can be called in three cases:
>>>>>>>>> 1. when vq ready status changes, vhost_user_msg_handler() calls it
>> to
>>>>>>>> notify
>>>>>>>>> backend. But vhost_user_msg_handler() doesn't take lock before
>>>> calling
>>>>>> it.
>>>>>>>>> So in this case, no deadlock occurs in async register.
>>>>>>>>>
>>>>>>>>> 2. if vq->ready is true, vhost_user_set_vring_call() calls it to notify
>>>>>> backend
>>>>>>>>> vq is not enabled. Although vhost_user_set_vring_call() is protected
>> by
>>>>>> lock,
>>>>>>>>> async register is called only if vq is enabled, so async register will not
>>>> be
>>>>>>>> called
>>>>>>>>> in this case.
>>>>>>>>>
>>>>>>>>> 3. If vq->ready is true, vhost_user_set_vring_kick() calls it to notify
>>>>>> backend
>>>>>>>>> vq is not enabled. Same as #2, async register is called only when vq
>> is
>>>>>>>> enabled.
>>>>>>>>> Even if vhost_user_set_vring_kick() is protected by lock, there is no
>>>>>>>> deadlock in
>>>>>>>>> async register, as it will not be called in this case.
>>>>>>>>>
>>>>>>>>> In summary,  I think there is no deadlock issue in async register if we
>>>>>>>>> can remove calling vring_state_change() in
>>>> vhost_user_set_vring_kick().
>>>>>>>>
>>>>>>>>
>>>>>>>> But unregister one could be called in theory no? Otherwise it would
>>>> look
>>>>>>>> unbalanced. At least on disabled notification, the app should make
>> sure
>>>>>>>> the DMA transfers to and from the vring are stopped before it
>> returns
>>>>>>>> from the callabck. Otherwise it could lead to undefined behavior.
>>>>>>>
>>>>>>> Right, users need to call unregister, but we cannot remove calling
>>>>>>> vhost_user_notify_queue_state() in case #2 and #3, IMHO. So to
>>>>>>> avoid deadlock, we recommended users to call async unregister in
>>>>>>> destroy_device(), instead of on vring disabled notification. Does it
>>>>>>> make sense to you?
>>>>>>
>>>>>> Calling async unregister in destroy device is fine by me. But I'm more
>>>>>> concerned about DMA transations not being stopped when the ring
>>>> becomes
>>>>>> disabled.
>>>>> If ring becomes disabled, no more new pkts go into async data path, as
>>>>> virtio_dev_rx_async_submit() returns if vq->enabled is false.
>>>>>
>>>>>>
>>>>>> I cannot say if you are doing it right, because the vhost example does
>>>>>> not implement the vring_state_changed callback.
>>>>>> It is not a problem with the sync datapath as we have the lock
>>>>>> protection + enabled variable that prevents to process the rings when it
>>>>>> gets stopped.
>>>>>> But for the async path, if you have programmed DMA transfers, you
>> need
>>>>>> to rely on the vring_state_change to block the control path while the
>>>>>> transfers are cancelled or done.
>>>>>
>>>>> I am not sure if I understand your concern correctly, but for async data
>>>> path,
>>>>> enable variable can prevent it from enqueue new pkts when ring is
>>>> disabled, as
>>>>> virtio_dev_rx_async_submit() check enable variable before processing
>> ring;
>>>>> in addition, lock protection can stop async data path as
>>>> virtio_dev_rx_async_submit()
>>>>> acquires lock too. For example, in the case that front-end updates kickfd,
>>>>> set_vring_kick() will notify backend that vring is stopped by calling
>>>>> vhost_user_notify_queue_state(). In this case, sync data path will stop as
>> a
>>>> result of
>>>>> lock protection, and I think it's the same for async data path, as it
>> acquires
>>>> lock before
>>>>> processing ring.
>>>>
>>>>
>>>> What about memory hotplug happening while the DMA transfers are on-
>>>> going
>>>> for example?
>>>>
>>>> In this case, the lock is enough for sync datapath, but is not for async
>>>> one.
>>>>
>>>> If a VHOST_USER_SET_MEM_TABLE request is received while
>>>> virtio_dev_rx_async_submit(), it will be blocked until
>>>> virtio_dev_rx_async_submit() returns but the DMA transfers may not be
>>>> finished yet.
>>>> When unblocked the control thread will call vhost_user_set_mem_table(),
>>>> which will mnumap the current memory regions before mmaping the new
>>>> ones while DMA transfers are on-going.
>>>>
>>>> To fix this, we need to call the vring_state_changed callback for all
>>>> the virtqueues with disable state. in your app, you need to stop DMA
>>>> transfers when disabled state notification happens, or block the
>>>> callback while the transfer is done.
>>>
>>> Let me summarize the problem:
>>> when frontend memory is hot-plug, host application needs to stop
>>> DMA transfer for all virtqueues, which is done by emptying in-flight
>>> DMA copies and unregister async inside vring_state_changed().
>>
>> I don't think unregistering async channels is mandatory.
>> On disable notification, the callback has to block until all on-going
>> DMA transfers are done. New transfers won't be schedules since
>> virtio_dev_rx_async_submit() will be blocked by the lock while the
>> memory regions update is being done.
> 
> Yes, unregister is not a must, but the problem is that
> rte_vhost_poll_enqueue_completed() takes lock too.
> In VM memory hot-plug case, emptying in-flight pkts in
> vring_state_changed() may be OK, as it is called by
> vhost_user_msg_handler() (please correct me if I am wrong),

This is wrong. As I said in previous email, we need a fix in
vhost_user_set_mem_table() to call the vring_state_changed callback with
disabled state before the shared memory get unmapped. This was not
necessary for sync datapath as the lock already protects that, but this
is mandatory for async path (and maybe SPDK too, but I didn't checked).

> which doesn't take lock. But if it is called inside set_vring_kick()/_call(),
> a deadlock occurs in rte_vhost_poll_enqueue_completed(), even if user
> app doesn't call async unregister API.

It will be taken with a lock too in set_mem_table().

>>
>>> But
>>> vhost library takes lock before entering vring_state_changed(), and
>>> async unregister and rte_vhost_poll_enqueue_completed() both
>>> acquire lock, so deadlock occurs inside rte_vhost_poll_enqueue_
>>> completed() or async unregister. (please correct me if am wrong)
>>>
>>> There may be two possible solutions:
>>> 1. remove lock before calling vring_state_change() in vhost library;
>>
>> I don't think so. If you release the lock in vring_state_change(), it
>> will enable the app to enqueue/dequeue descriptors in the middle of
>> handling the request.
> 
> Sorry, I didn't express my opinion clearly. I mean remove the following the
> code in set_vring_kick() and set_vring_call():
> -       if (vq->ready) {
> -               vq->ready = false;
> -               vhost_user_notify_queue_state(dev, file.index, 0);
> -       }
> 
> IMHO, in the both cases, the purpose of calling vhost_user_notify_queue_state()
> is to tell backend vring is not ready to use as a result of callfd/kickfd update;
> after that, vhost_user_msg_handler() calls it again to notify backend of vring
> ready to process. But there is lock protection before entering
> set_vring_kick()/_call(), so data path threads will not process vring during updating
> callfd/kickfd anyway. After updating callfd/kickfd, I think data path threads are already
> safe to start to process vring, so there is no need for a ready notification in
> vhost_user_msg_hander().
> 
> BTW, lock protection for vhost_user_notify_queue_state() is only in
> set_vring_kick()/_call(), but not in vhost_msg_handler().

My point in previous reply was that the lock is mandatory in
set_vring_state and other places, so in order to have consistent
behaviour, the lock should be taken every time we call the callback so
even in vhost_msg_handler().

> Thanks,
> Jiayu
>>
>>> 2. provide a new callback without acquiring lock for DMA accelerated
>>> case. The callback is used to notify backend that you need to stop
>>> DMA transfer.
>>
>> No, in your app just wait for DMA transfers to be finished before
>> returning from the callback.
>>
>> Please implement vring_state_changed callback in Vhost example, it is
>> now mandatory with async datapath, and it will help to have an example
>> on what real applications should do.
>>
>> Thanks,
>> Maxime
>>
>>> Thanks,
>>> Jiayu
>>>
> 

Re: [dpdk-dev] [PATCH v2 3/4] vhost: avoid deadlock on async register

[Hu, Jiayu]

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Monday, April 19, 2021 3:13 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Wang, Yinan
> <yinan.wang@intel.com>; Pai G, Sunil <sunil.pai.g@intel.com>; Jiang, Cheng1
> <cheng1.jiang@intel.com>
> Subject: Re: [PATCH v2 3/4] vhost: avoid deadlock on async register
> >>>>
> >>>> What about memory hotplug happening while the DMA transfers are
> on-
> >>>> going
> >>>> for example?
> >>>>
> >>>> In this case, the lock is enough for sync datapath, but is not for async
> >>>> one.
> >>>>
> >>>> If a VHOST_USER_SET_MEM_TABLE request is received while
> >>>> virtio_dev_rx_async_submit(), it will be blocked until
> >>>> virtio_dev_rx_async_submit() returns but the DMA transfers may not
> be
> >>>> finished yet.
> >>>> When unblocked the control thread will call
> vhost_user_set_mem_table(),
> >>>> which will mnumap the current memory regions before mmaping the
> new
> >>>> ones while DMA transfers are on-going.
> >>>>
> >>>> To fix this, we need to call the vring_state_changed callback for all
> >>>> the virtqueues with disable state. in your app, you need to stop DMA
> >>>> transfers when disabled state notification happens, or block the
> >>>> callback while the transfer is done.
> >>>
> >>> Let me summarize the problem:
> >>> when frontend memory is hot-plug, host application needs to stop
> >>> DMA transfer for all virtqueues, which is done by emptying in-flight
> >>> DMA copies and unregister async inside vring_state_changed().
> >>
> >> I don't think unregistering async channels is mandatory.
> >> On disable notification, the callback has to block until all on-going
> >> DMA transfers are done. New transfers won't be schedules since
> >> virtio_dev_rx_async_submit() will be blocked by the lock while the
> >> memory regions update is being done.
> >
> > Yes, unregister is not a must, but the problem is that
> > rte_vhost_poll_enqueue_completed() takes lock too.
> > In VM memory hot-plug case, emptying in-flight pkts in
> > vring_state_changed() may be OK, as it is called by
> > vhost_user_msg_handler() (please correct me if I am wrong),
> 
> This is wrong. As I said in previous email, we need a fix in
> vhost_user_set_mem_table() to call the vring_state_changed callback with
> disabled state before the shared memory get unmapped. This was not
> necessary for sync datapath as the lock already protects that, but this
> is mandatory for async path (and maybe SPDK too, but I didn't checked).

OK, I will add calling vring_state_changed() in set_mem_table(). But there is
still a deadlock issue in rte_vhost_poll_enqueue_completed(), as it takes lock.
Providing a new API for emptying in-flight pkts and w/o taking lock may be a method
to solve the deadlock issue, but its code will be almost the same as
rte_vhost_poll_enqueue_completed() except not taking lock. I wonder if you
have other suggestions?

> 
> > which doesn't take lock. But if it is called inside set_vring_kick()/_call(),
> > a deadlock occurs in rte_vhost_poll_enqueue_completed(), even if user
> > app doesn't call async unregister API.
> 
> It will be taken with a lock too in set_mem_table().
> 
> >>
> >>> But
> >>> vhost library takes lock before entering vring_state_changed(), and
> >>> async unregister and rte_vhost_poll_enqueue_completed() both
> >>> acquire lock, so deadlock occurs inside rte_vhost_poll_enqueue_
> >>> completed() or async unregister. (please correct me if am wrong)
> >>>
> >>> There may be two possible solutions:
> >>> 1. remove lock before calling vring_state_change() in vhost library;
> >>
> >> I don't think so. If you release the lock in vring_state_change(), it
> >> will enable the app to enqueue/dequeue descriptors in the middle of
> >> handling the request.
> >
> > Sorry, I didn't express my opinion clearly. I mean remove the following the
> > code in set_vring_kick() and set_vring_call():
> > -       if (vq->ready) {
> > -               vq->ready = false;
> > -               vhost_user_notify_queue_state(dev, file.index, 0);
> > -       }
> >
> > IMHO, in the both cases, the purpose of calling
> vhost_user_notify_queue_state()
> > is to tell backend vring is not ready to use as a result of callfd/kickfd update;
> > after that, vhost_user_msg_handler() calls it again to notify backend of
> vring
> > ready to process. But there is lock protection before entering
> > set_vring_kick()/_call(), so data path threads will not process vring during
> updating
> > callfd/kickfd anyway. After updating callfd/kickfd, I think data path threads
> are already
> > safe to start to process vring, so there is no need for a ready notification in
> > vhost_user_msg_hander().
> >
> > BTW, lock protection for vhost_user_notify_queue_state() is only in
> > set_vring_kick()/_call(), but not in vhost_msg_handler().
> 
> My point in previous reply was that the lock is mandatory in
> set_vring_state and other places, so in order to have consistent
> behaviour, the lock should be taken every time we call the callback so
> even in vhost_msg_handler().

OK, I can send a patch to fix it.

Thanks,
Jiayu

[dpdk-dev] [PATCH v2 4/4] doc: update async vhost register/unregister

[Jiayu Hu]

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
---
 doc/guides/prog_guide/vhost_lib.rst | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/doc/guides/prog_guide/vhost_lib.rst b/doc/guides/prog_guide/vhost_lib.rst
index dc29229..333bd0a 100644
--- a/doc/guides/prog_guide/vhost_lib.rst
+++ b/doc/guides/prog_guide/vhost_lib.rst
@@ -208,9 +208,9 @@ The following is an overview of some key Vhost API functions:
 
 * ``rte_vhost_async_channel_register(vid, queue_id, features, ops)``
 
-  Register a vhost queue with async copy device channel.
-  Following device ``features`` must be specified together with the
-  registration:
+  Register a vhost queue with async copy device channel after vring
+  is enabled. Following device ``features`` must be specified together
+  with the registration:
 
   * ``async_inorder``
 
@@ -244,6 +244,13 @@ The following is an overview of some key Vhost API functions:
 * ``rte_vhost_async_channel_unregister(vid, queue_id)``
 
   Unregister the async copy device channel from a vhost queue.
+  Unregistration will fail, if the vhost queue has in-flight
+  packets that are not completed.
+
+  To guarantee correct behaviors, users must unregister async
+  copy devices for all vhost queues, when virtio device is paused
+  or shut down. Note that this API tries to acquire the spinlock
+  of vhost queue, so users need stop polling thread before unregister.
 
 * ``rte_vhost_submit_enqueue_burst(vid, queue_id, pkts, count, comp_pkts, comp_count)``
 
-- 
2.7.4

[dpdk-dev] [PATCH v3 0/4] Refactor async vhost control path

[Jiayu Hu]

This patch set refactors async vhost control path.

Change log
==========
v3:
- correct fix commit
- update doc and commit log
v2:
- correct fix commit
- update commit lo

Jiayu Hu (4):
  vhost: fix uninitialized vhost queue
  vhost: remove unnecessary free
  vhost: fix unnecessary vring_state_changed call
  doc: update async vhost register/unregister

 doc/guides/prog_guide/vhost_lib.rst | 14 +++++++++++---
 lib/librte_vhost/vhost.c            |  2 +-
 lib/librte_vhost/vhost_user.c       | 10 ----------
 3 files changed, 12 insertions(+), 14 deletions(-)

-- 
2.7.4

[dpdk-dev] [PATCH v3 1/4] vhost: fix uninitialized vhost queue

[Jiayu Hu]

This patch allocates vhost queue by rte_zmalloc() to avoid
undefined values.

Fixes: a277c7159876 ("vhost: refactor code structure")
Cc: stable@dpdk.org

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Tested-by: Yinan Wang <yinan.wang@intel.com>
---
 lib/librte_vhost/vhost.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
index a70fe01..ea38cf2 100644
--- a/lib/librte_vhost/vhost.c
+++ b/lib/librte_vhost/vhost.c
@@ -608,7 +608,7 @@ alloc_vring_queue(struct virtio_net *dev, uint32_t vring_idx)
 		if (dev->virtqueue[i])
 			continue;
 
-		vq = rte_malloc(NULL, sizeof(struct vhost_virtqueue), 0);
+		vq = rte_zmalloc(NULL, sizeof(struct vhost_virtqueue), 0);
 		if (vq == NULL) {
 			VHOST_LOG_CONFIG(ERR,
 				"Failed to allocate memory for vring:%u.\n", i);
-- 
2.7.4

[dpdk-dev] [PATCH v3 2/4] vhost: remove unnecessary free

[Jiayu Hu]

This patch removes unnecessary rte_free() for async_pkts_info
and async_descs_split.

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Tested-by: Yinan Wang <yinan.wang@intel.com>
---
 lib/librte_vhost/vhost_user.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index cdd46a0..fa8929f 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -2013,13 +2013,6 @@ vhost_user_get_vring_base(struct virtio_net **pdev,
 	} else {
 		rte_free(vq->shadow_used_split);
 		vq->shadow_used_split = NULL;
-
-		if (vq->async_pkts_info)
-			rte_free(vq->async_pkts_info);
-		if (vq->async_descs_split)
-			rte_free(vq->async_descs_split);
-		vq->async_pkts_info = NULL;
-		vq->async_descs_split = NULL;
 	}
 
 	rte_free(vq->batch_copy_elems);
-- 
2.7.4

[dpdk-dev] [PATCH v3 3/4] vhost: fix unnecessary vring_state_changed call

[Jiayu Hu]

When VHOST_USER_F_PROTOCOL_FEATURES is not negotiated,
there is no need for vhost_user_set_vring_kick() to
notify the application of vring enabled, as
vhost_user_msg_handler() also notifies the application.

This patch is to remove unnecessary vring_state_changed() call.

Fixes: 966027b4b3a3 ("vhost: fix silent queue enabling with legacy guests")
Cc: stable@dpdk.org

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
Tested-by: Yinan Wang <yinan.wang@intel.com>
---
 lib/librte_vhost/vhost_user.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index fa8929f..611ff20 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -1922,9 +1922,6 @@ vhost_user_set_vring_kick(struct virtio_net **pdev, struct VhostUserMsg *msg,
 	 */
 	if (!(dev->features & (1ULL << VHOST_USER_F_PROTOCOL_FEATURES))) {
 		vq->enabled = true;
-		if (dev->notify_ops->vring_state_changed)
-			dev->notify_ops->vring_state_changed(
-				dev->vid, file.index, 1);
 	}
 
 	if (vq->ready) {
-- 
2.7.4

Re: [dpdk-dev] [PATCH v3 3/4] vhost: fix unnecessary vring_state_changed call

[Maxime Coquelin]

On 4/20/21 10:57 AM, Jiayu Hu wrote:
> When VHOST_USER_F_PROTOCOL_FEATURES is not negotiated,
> there is no need for vhost_user_set_vring_kick() to
> notify the application of vring enabled, as
> vhost_user_msg_handler() also notifies the application.
> 
> This patch is to remove unnecessary vring_state_changed() call.
> 
> Fixes: 966027b4b3a3 ("vhost: fix silent queue enabling with legacy guests")

Sorry, I thought the vring_state_changed cb was introduced when Matan
did the rework last year, but it is actually older.

If we backport the patch on v19.11, we will create a regression as it
will revert Ilya's patch.

I think that your fix is correct only once Matan's series is present, so
we should have below Fixes tag:

Fixes: d0fcc38f5fa4 ("vhost: improve device readiness notifications")

> Cc: stable@dpdk.org
> 
> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> Tested-by: Yinan Wang <yinan.wang@intel.com>
> ---
>  lib/librte_vhost/vhost_user.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index fa8929f..611ff20 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -1922,9 +1922,6 @@ vhost_user_set_vring_kick(struct virtio_net **pdev, struct VhostUserMsg *msg,
>  	 */
>  	if (!(dev->features & (1ULL << VHOST_USER_F_PROTOCOL_FEATURES))) {
>  		vq->enabled = true;
> -		if (dev->notify_ops->vring_state_changed)
> -			dev->notify_ops->vring_state_changed(
> -				dev->vid, file.index, 1);
>  	}
>  
>  	if (vq->ready) {
> 

With the fixes tag changed:
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Chanbo, can you change the Fixes tag while applying or Jiayu needs to
send a new revision?

Thanks,
Maxime

Re: [dpdk-dev] [PATCH v3 3/4] vhost: fix unnecessary vring_state_changed call

[Xia, Chenbo]

Hi Maxime & Jiayu,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, April 20, 2021 5:40 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org; Xia, Chenbo
> <chenbo.xia@intel.com>
> Cc: Wang, Yinan <yinan.wang@intel.com>; Pai G, Sunil <sunil.pai.g@intel.com>;
> Jiang, Cheng1 <cheng1.jiang@intel.com>; stable@dpdk.org
> Subject: Re: [PATCH v3 3/4] vhost: fix unnecessary vring_state_changed call
> 
> 
> 
> On 4/20/21 10:57 AM, Jiayu Hu wrote:
> > When VHOST_USER_F_PROTOCOL_FEATURES is not negotiated,
> > there is no need for vhost_user_set_vring_kick() to
> > notify the application of vring enabled, as
> > vhost_user_msg_handler() also notifies the application.
> >
> > This patch is to remove unnecessary vring_state_changed() call.
> >
> > Fixes: 966027b4b3a3 ("vhost: fix silent queue enabling with legacy guests")
> 
> Sorry, I thought the vring_state_changed cb was introduced when Matan
> did the rework last year, but it is actually older.
> 
> If we backport the patch on v19.11, we will create a regression as it
> will revert Ilya's patch.
> 
> I think that your fix is correct only once Matan's series is present, so
> we should have below Fixes tag:
> 
> Fixes: d0fcc38f5fa4 ("vhost: improve device readiness notifications")
> 
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> > Tested-by: Yinan Wang <yinan.wang@intel.com>
> > ---
> >  lib/librte_vhost/vhost_user.c | 3 ---
> >  1 file changed, 3 deletions(-)
> >
> > diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> > index fa8929f..611ff20 100644
> > --- a/lib/librte_vhost/vhost_user.c
> > +++ b/lib/librte_vhost/vhost_user.c
> > @@ -1922,9 +1922,6 @@ vhost_user_set_vring_kick(struct virtio_net **pdev,
> struct VhostUserMsg *msg,
> >  	 */
> >  	if (!(dev->features & (1ULL << VHOST_USER_F_PROTOCOL_FEATURES))) {
> >  		vq->enabled = true;
> > -		if (dev->notify_ops->vring_state_changed)
> > -			dev->notify_ops->vring_state_changed(
> > -				dev->vid, file.index, 1);
> >  	}
> >
> >  	if (vq->ready) {
> >
> 
> With the fixes tag changed:
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> Chanbo, can you change the Fixes tag while applying or Jiayu needs to
> send a new revision?

No worry. Will change fix tag while applying.

Thanks!
Chenbo

> 
> Thanks,
> Maxime

[dpdk-dev] [PATCH v3 4/4] doc: update async vhost register/unregister

[Jiayu Hu]

This patch is to update programmer guide for register/unregister
copy devices in vhost.

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
---
 doc/guides/prog_guide/vhost_lib.rst | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/doc/guides/prog_guide/vhost_lib.rst b/doc/guides/prog_guide/vhost_lib.rst
index dc29229..7afa351 100644
--- a/doc/guides/prog_guide/vhost_lib.rst
+++ b/doc/guides/prog_guide/vhost_lib.rst
@@ -208,9 +208,9 @@ The following is an overview of some key Vhost API functions:
 
 * ``rte_vhost_async_channel_register(vid, queue_id, features, ops)``
 
-  Register a vhost queue with async copy device channel.
-  Following device ``features`` must be specified together with the
-  registration:
+  Register a vhost queue with async copy device channel after vring
+  is enabled. Following device ``features`` must be specified together
+  with the registration:
 
   * ``async_inorder``
 
@@ -244,6 +244,14 @@ The following is an overview of some key Vhost API functions:
 * ``rte_vhost_async_channel_unregister(vid, queue_id)``
 
   Unregister the async copy device channel from a vhost queue.
+  Unregistration will fail, if the vhost queue has in-flight
+  packets that are not completed.
+
+  Unregister async copy devices in vring_state_changed() may
+  fail, as this API tries to acquire the spinlock of vhost
+  queue. The recommended way is to unregister async copy
+  devices for all vhost queues in destroy_device(), when a
+  virtio device is paused or shut down.
 
 * ``rte_vhost_submit_enqueue_burst(vid, queue_id, pkts, count, comp_pkts, comp_count)``
 
-- 
2.7.4

Re: [dpdk-dev] [PATCH v3 4/4] doc: update async vhost register/unregister

[Maxime Coquelin]

On 4/20/21 10:57 AM, Jiayu Hu wrote:
> This patch is to update programmer guide for register/unregister
> copy devices in vhost.
> 
> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> ---
>  doc/guides/prog_guide/vhost_lib.rst | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/doc/guides/prog_guide/vhost_lib.rst b/doc/guides/prog_guide/vhost_lib.rst
> index dc29229..7afa351 100644
> --- a/doc/guides/prog_guide/vhost_lib.rst
> +++ b/doc/guides/prog_guide/vhost_lib.rst
> @@ -208,9 +208,9 @@ The following is an overview of some key Vhost API functions:
>  
>  * ``rte_vhost_async_channel_register(vid, queue_id, features, ops)``
>  
> -  Register a vhost queue with async copy device channel.
> -  Following device ``features`` must be specified together with the
> -  registration:
> +  Register a vhost queue with async copy device channel after vring
> +  is enabled. Following device ``features`` must be specified together
> +  with the registration:
>  
>    * ``async_inorder``
>  
> @@ -244,6 +244,14 @@ The following is an overview of some key Vhost API functions:
>  * ``rte_vhost_async_channel_unregister(vid, queue_id)``
>  
>    Unregister the async copy device channel from a vhost queue.
> +  Unregistration will fail, if the vhost queue has in-flight
> +  packets that are not completed.
> +
> +  Unregister async copy devices in vring_state_changed() may
> +  fail, as this API tries to acquire the spinlock of vhost
> +  queue. The recommended way is to unregister async copy
> +  devices for all vhost queues in destroy_device(), when a
> +  virtio device is paused or shut down.
>  
>  * ``rte_vhost_submit_enqueue_burst(vid, queue_id, pkts, count, comp_pkts, comp_count)``
>  
> 

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

Thanks,
Maxime

Re: [dpdk-dev] [PATCH v3 0/4] Refactor async vhost control path

[Xia, Chenbo]

> -----Original Message-----
> From: Hu, Jiayu <jiayu.hu@intel.com>
> Sent: Tuesday, April 20, 2021 4:58 PM
> To: dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>; Wang,
> Yinan <yinan.wang@intel.com>; Pai G, Sunil <sunil.pai.g@intel.com>; Jiang,
> Cheng1 <cheng1.jiang@intel.com>; Hu, Jiayu <jiayu.hu@intel.com>
> Subject: [PATCH v3 0/4] Refactor async vhost control path
> 
> This patch set refactors async vhost control path.
> 
> Change log
> ==========
> v3:
> - correct fix commit
> - update doc and commit log
> v2:
> - correct fix commit
> - update commit lo
> 
> Jiayu Hu (4):
>   vhost: fix uninitialized vhost queue
>   vhost: remove unnecessary free
>   vhost: fix unnecessary vring_state_changed call
>   doc: update async vhost register/unregister
> 
>  doc/guides/prog_guide/vhost_lib.rst | 14 +++++++++++---
>  lib/librte_vhost/vhost.c            |  2 +-
>  lib/librte_vhost/vhost_user.c       | 10 ----------
>  3 files changed, 12 insertions(+), 14 deletions(-)
> 
> --
> 2.7.4

Series applied to next-virtio/main with commit log fixed.

Thanks