[PATCH] vhost: use try_lock in rte_vhost_vring_call

[PATCH] vhost: use try_lock in rte_vhost_vring_call

[Changpeng Liu]

Note that this function is in data path, so the thread context
may not same as socket messages processing context, by using
try_lock here, users can have another try in case of VQ's access
lock is held by `vhost-events` thread.

Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
---
 lib/vhost/vhost.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
index 60cb05a0ff..072d2acb7b 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -1329,7 +1329,11 @@ rte_vhost_vring_call(int vid, uint16_t vring_idx)
 	if (!vq)
 		return -1;
 
-	rte_spinlock_lock(&vq->access_lock);
+	if (!rte_spinlock_trylock(&vq->access_lock)) {
+		VHOST_LOG_CONFIG(dev->ifname, DEBUG,
+			"failed to kick guest, virtqueue busy.\n");
+		return -1;
+	}
 
 	if (vq_is_packed(dev))
 		vhost_vring_call_packed(dev, vq);
-- 
2.21.3

Re: [PATCH] vhost: use try_lock in rte_vhost_vring_call

[Stephen Hemminger]

On Tue,  6 Sep 2022 10:22:25 +0800
Changpeng Liu <changpeng.liu@intel.com> wrote:

> Note that this function is in data path, so the thread context
> may not same as socket messages processing context, by using
> try_lock here, users can have another try in case of VQ's access
> lock is held by `vhost-events` thread.
> 
> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> ---
>  lib/vhost/vhost.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> index 60cb05a0ff..072d2acb7b 100644
> --- a/lib/vhost/vhost.c
> +++ b/lib/vhost/vhost.c
> @@ -1329,7 +1329,11 @@ rte_vhost_vring_call(int vid, uint16_t vring_idx)
>  	if (!vq)
>  		return -1;
>  
> -	rte_spinlock_lock(&vq->access_lock);
> +	if (!rte_spinlock_trylock(&vq->access_lock)) {
> +		VHOST_LOG_CONFIG(dev->ifname, DEBUG,
> +			"failed to kick guest, virtqueue busy.\n");
> +		return -1;
> +	}
>  

If it is a race, logging a message is not a good idea; the log will fill
with this noise.

Instead make it statistic that can be seen by xstats.

RE: [PATCH] vhost: use try_lock in rte_vhost_vring_call

[Liu, Changpeng]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Wednesday, September 7, 2022 5:16 AM
> To: Liu, Changpeng <changpeng.liu@intel.com>
> Cc: dev@dpdk.org; Maxime Coquelin <maxime.coquelin@redhat.com>; Xia,
> Chenbo <chenbo.xia@intel.com>
> Subject: Re: [PATCH] vhost: use try_lock in rte_vhost_vring_call
> 
> On Tue,  6 Sep 2022 10:22:25 +0800
> Changpeng Liu <changpeng.liu@intel.com> wrote:
> 
> > Note that this function is in data path, so the thread context
> > may not same as socket messages processing context, by using
> > try_lock here, users can have another try in case of VQ's access
> > lock is held by `vhost-events` thread.
> >
> > Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> > ---
> >  lib/vhost/vhost.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> > index 60cb05a0ff..072d2acb7b 100644
> > --- a/lib/vhost/vhost.c
> > +++ b/lib/vhost/vhost.c
> > @@ -1329,7 +1329,11 @@ rte_vhost_vring_call(int vid, uint16_t vring_idx)
> >  	if (!vq)
> >  		return -1;
> >
> > -	rte_spinlock_lock(&vq->access_lock);
> > +	if (!rte_spinlock_trylock(&vq->access_lock)) {
> > +		VHOST_LOG_CONFIG(dev->ifname, DEBUG,
> > +			"failed to kick guest, virtqueue busy.\n");
> > +		return -1;
> > +	}
> >
> 
> If it is a race, logging a message is not a good idea; the log will fill
> with this noise.
> 
> Instead make it statistic that can be seen by xstats.
It's a DEBUG log, users can't see it in practice.

Re: [PATCH] vhost: use try_lock in rte_vhost_vring_call

[Maxime Coquelin]

On 9/7/22 02:40, Liu, Changpeng wrote:
> 
> 
>> -----Original Message-----
>> From: Stephen Hemminger <stephen@networkplumber.org>
>> Sent: Wednesday, September 7, 2022 5:16 AM
>> To: Liu, Changpeng <changpeng.liu@intel.com>
>> Cc: dev@dpdk.org; Maxime Coquelin <maxime.coquelin@redhat.com>; Xia,
>> Chenbo <chenbo.xia@intel.com>
>> Subject: Re: [PATCH] vhost: use try_lock in rte_vhost_vring_call
>>
>> On Tue,  6 Sep 2022 10:22:25 +0800
>> Changpeng Liu <changpeng.liu@intel.com> wrote:
>>
>>> Note that this function is in data path, so the thread context
>>> may not same as socket messages processing context, by using
>>> try_lock here, users can have another try in case of VQ's access
>>> lock is held by `vhost-events` thread.
>>>
>>> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
>>> ---
>>>   lib/vhost/vhost.c | 6 +++++-
>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
>>> index 60cb05a0ff..072d2acb7b 100644
>>> --- a/lib/vhost/vhost.c
>>> +++ b/lib/vhost/vhost.c
>>> @@ -1329,7 +1329,11 @@ rte_vhost_vring_call(int vid, uint16_t vring_idx)
>>>   	if (!vq)
>>>   		return -1;
>>>
>>> -	rte_spinlock_lock(&vq->access_lock);
>>> +	if (!rte_spinlock_trylock(&vq->access_lock)) {
>>> +		VHOST_LOG_CONFIG(dev->ifname, DEBUG,
>>> +			"failed to kick guest, virtqueue busy.\n");
>>> +		return -1;
>>> +	}
>>>
>>
>> If it is a race, logging a message is not a good idea; the log will fill
>> with this noise.
>>
>> Instead make it statistic that can be seen by xstats.
> It's a DEBUG log, users can't see it in practice.
> 

Having an xstat would enable live debugging & post-mortem analysis.
You can have both the stats and the debug log.

Regards,
Maxime

RE: [PATCH] vhost: use try_lock in rte_vhost_vring_call

[Xia, Chenbo]

Hi Changpeng,

> -----Original Message-----
> From: Liu, Changpeng <changpeng.liu@intel.com>
> Sent: Tuesday, September 6, 2022 10:22 AM
> To: dev@dpdk.org
> Cc: Liu, Changpeng <changpeng.liu@intel.com>; Maxime Coquelin
> <maxime.coquelin@redhat.com>; Xia, Chenbo <chenbo.xia@intel.com>
> Subject: [PATCH] vhost: use try_lock in rte_vhost_vring_call
> 
> Note that this function is in data path, so the thread context
> may not same as socket messages processing context, by using
> try_lock here, users can have another try in case of VQ's access
> lock is held by `vhost-events` thread.

Better to describe the issue this patch wants to fix and how does
it fix.

I remember it's a bz issue, do you want to backport? And it has
some bz ID, we need to add it in commit message.

> 
> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> ---
>  lib/vhost/vhost.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> index 60cb05a0ff..072d2acb7b 100644
> --- a/lib/vhost/vhost.c
> +++ b/lib/vhost/vhost.c
> @@ -1329,7 +1329,11 @@ rte_vhost_vring_call(int vid, uint16_t vring_idx)
>  	if (!vq)
>  		return -1;
> 
> -	rte_spinlock_lock(&vq->access_lock);
> +	if (!rte_spinlock_trylock(&vq->access_lock)) {
> +		VHOST_LOG_CONFIG(dev->ifname, DEBUG,

Should use VHOST_LOG_DATA

Thanks,
Chenbo

> +			"failed to kick guest, virtqueue busy.\n");
> +		return -1;
> +	}
> 
>  	if (vq_is_packed(dev))
>  		vhost_vring_call_packed(dev, vq);
> --
> 2.21.3

RE: [PATCH] vhost: use try_lock in rte_vhost_vring_call

[Liu, Changpeng]

Hi Bo,

> -----Original Message-----
> From: Xia, Chenbo <chenbo.xia@intel.com>
> Sent: Tuesday, September 20, 2022 10:25 AM
> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
> Subject: RE: [PATCH] vhost: use try_lock in rte_vhost_vring_call
> 
> Hi Changpeng,
> 
> > -----Original Message-----
> > From: Liu, Changpeng <changpeng.liu@intel.com>
> > Sent: Tuesday, September 6, 2022 10:22 AM
> > To: dev@dpdk.org
> > Cc: Liu, Changpeng <changpeng.liu@intel.com>; Maxime Coquelin
> > <maxime.coquelin@redhat.com>; Xia, Chenbo <chenbo.xia@intel.com>
> > Subject: [PATCH] vhost: use try_lock in rte_vhost_vring_call
> >
> > Note that this function is in data path, so the thread context
> > may not same as socket messages processing context, by using
> > try_lock here, users can have another try in case of VQ's access
> > lock is held by `vhost-events` thread.
> 
> Better to describe the issue this patch wants to fix and how does
> it fix.
> 
> I remember it's a bz issue, do you want to backport? And it has
> some bz ID, we need to add it in commit message.
Actually it's my intention not to add bz ID, as I think for this bz ID,
It's better not to lock all VQ's access lock for KICK/CALLFD messages,
What do you think? If this is identified as a fix, I can backport it to 22.05.
> 
> >
> > Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> > ---
> >  lib/vhost/vhost.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> > index 60cb05a0ff..072d2acb7b 100644
> > --- a/lib/vhost/vhost.c
> > +++ b/lib/vhost/vhost.c
> > @@ -1329,7 +1329,11 @@ rte_vhost_vring_call(int vid, uint16_t vring_idx)
> >  	if (!vq)
> >  		return -1;
> >
> > -	rte_spinlock_lock(&vq->access_lock);
> > +	if (!rte_spinlock_trylock(&vq->access_lock)) {
> > +		VHOST_LOG_CONFIG(dev->ifname, DEBUG,
> 
> Should use VHOST_LOG_DATA
OK.
> 
> Thanks,
> Chenbo
> 
> > +			"failed to kick guest, virtqueue busy.\n");
> > +		return -1;
> > +	}
> >
> >  	if (vq_is_packed(dev))
> >  		vhost_vring_call_packed(dev, vq);
> > --
> > 2.21.3

RE: [PATCH] vhost: use try_lock in rte_vhost_vring_call

[Xia, Chenbo]

> -----Original Message-----
> From: Liu, Changpeng <changpeng.liu@intel.com>
> Sent: Tuesday, September 20, 2022 10:34 AM
> To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
> Subject: RE: [PATCH] vhost: use try_lock in rte_vhost_vring_call
> 
> Hi Bo,
> 
> > -----Original Message-----
> > From: Xia, Chenbo <chenbo.xia@intel.com>
> > Sent: Tuesday, September 20, 2022 10:25 AM
> > To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
> > Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
> > Subject: RE: [PATCH] vhost: use try_lock in rte_vhost_vring_call
> >
> > Hi Changpeng,
> >
> > > -----Original Message-----
> > > From: Liu, Changpeng <changpeng.liu@intel.com>
> > > Sent: Tuesday, September 6, 2022 10:22 AM
> > > To: dev@dpdk.org
> > > Cc: Liu, Changpeng <changpeng.liu@intel.com>; Maxime Coquelin
> > > <maxime.coquelin@redhat.com>; Xia, Chenbo <chenbo.xia@intel.com>
> > > Subject: [PATCH] vhost: use try_lock in rte_vhost_vring_call
> > >
> > > Note that this function is in data path, so the thread context
> > > may not same as socket messages processing context, by using
> > > try_lock here, users can have another try in case of VQ's access
> > > lock is held by `vhost-events` thread.
> >
> > Better to describe the issue this patch wants to fix and how does
> > it fix.
> >
> > I remember it's a bz issue, do you want to backport? And it has
> > some bz ID, we need to add it in commit message.
> Actually it's my intention not to add bz ID, as I think for this bz ID,
> It's better not to lock all VQ's access lock for KICK/CALLFD messages,

Do you plan to add this change? I think that may be an improvement to current
locking implementation.

Maxime, what do you think of this idea about only locking specific queue when
handling vring related message (not global config like mem table)?

> What do you think? If this is identified as a fix, I can backport it to
> 22.05.

You can decide, if this is planned to be the fix, just backport. I am just
thinking if this is not the fix for the bz, do we still need this?

Thanks,
Chenbo

> >
> > >
> > > Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> > > ---
> > >  lib/vhost/vhost.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> > > index 60cb05a0ff..072d2acb7b 100644
> > > --- a/lib/vhost/vhost.c
> > > +++ b/lib/vhost/vhost.c
> > > @@ -1329,7 +1329,11 @@ rte_vhost_vring_call(int vid, uint16_t
> vring_idx)
> > >  	if (!vq)
> > >  		return -1;
> > >
> > > -	rte_spinlock_lock(&vq->access_lock);
> > > +	if (!rte_spinlock_trylock(&vq->access_lock)) {
> > > +		VHOST_LOG_CONFIG(dev->ifname, DEBUG,
> >
> > Should use VHOST_LOG_DATA
> OK.
> >
> > Thanks,
> > Chenbo
> >
> > > +			"failed to kick guest, virtqueue busy.\n");
> > > +		return -1;
> > > +	}
> > >
> > >  	if (vq_is_packed(dev))
> > >  		vhost_vring_call_packed(dev, vq);
> > > --
> > > 2.21.3

RE: [PATCH] vhost: use try_lock in rte_vhost_vring_call

[Liu, Changpeng]

> -----Original Message-----
> From: Xia, Chenbo <chenbo.xia@intel.com>
> Sent: Tuesday, September 20, 2022 10:54 AM
> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
> Subject: RE: [PATCH] vhost: use try_lock in rte_vhost_vring_call
> 
> > -----Original Message-----
> > From: Liu, Changpeng <changpeng.liu@intel.com>
> > Sent: Tuesday, September 20, 2022 10:34 AM
> > To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org
> > Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
> > Subject: RE: [PATCH] vhost: use try_lock in rte_vhost_vring_call
> >
> > Hi Bo,
> >
> > > -----Original Message-----
> > > From: Xia, Chenbo <chenbo.xia@intel.com>
> > > Sent: Tuesday, September 20, 2022 10:25 AM
> > > To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
> > > Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
> > > Subject: RE: [PATCH] vhost: use try_lock in rte_vhost_vring_call
> > >
> > > Hi Changpeng,
> > >
> > > > -----Original Message-----
> > > > From: Liu, Changpeng <changpeng.liu@intel.com>
> > > > Sent: Tuesday, September 6, 2022 10:22 AM
> > > > To: dev@dpdk.org
> > > > Cc: Liu, Changpeng <changpeng.liu@intel.com>; Maxime Coquelin
> > > > <maxime.coquelin@redhat.com>; Xia, Chenbo <chenbo.xia@intel.com>
> > > > Subject: [PATCH] vhost: use try_lock in rte_vhost_vring_call
> > > >
> > > > Note that this function is in data path, so the thread context
> > > > may not same as socket messages processing context, by using
> > > > try_lock here, users can have another try in case of VQ's access
> > > > lock is held by `vhost-events` thread.
> > >
> > > Better to describe the issue this patch wants to fix and how does
> > > it fix.
> > >
> > > I remember it's a bz issue, do you want to backport? And it has
> > > some bz ID, we need to add it in commit message.
> > Actually it's my intention not to add bz ID, as I think for this bz ID,
> > It's better not to lock all VQ's access lock for KICK/CALLFD messages,
> 
> Do you plan to add this change? I think that may be an improvement to current
> locking implementation.
No, I don't have such a plan.
> 
> Maxime, what do you think of this idea about only locking specific queue when
> handling vring related message (not global config like mem table)?
> 
> > What do you think? If this is identified as a fix, I can backport it to
> > 22.05.
> 
> You can decide, if this is planned to be the fix, just backport. I am just
> thinking if this is not the fix for the bz, do we still need this?
Adding the bz ID is OK to me. From SPDK's view indeed it's a fix. Will send
V2 later. Thanks.
> 
> Thanks,
> Chenbo
> 
> > >
> > > >
> > > > Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> > > > ---
> > > >  lib/vhost/vhost.c | 6 +++++-
> > > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> > > > index 60cb05a0ff..072d2acb7b 100644
> > > > --- a/lib/vhost/vhost.c
> > > > +++ b/lib/vhost/vhost.c
> > > > @@ -1329,7 +1329,11 @@ rte_vhost_vring_call(int vid, uint16_t
> > vring_idx)
> > > >  	if (!vq)
> > > >  		return -1;
> > > >
> > > > -	rte_spinlock_lock(&vq->access_lock);
> > > > +	if (!rte_spinlock_trylock(&vq->access_lock)) {
> > > > +		VHOST_LOG_CONFIG(dev->ifname, DEBUG,
> > >
> > > Should use VHOST_LOG_DATA
> > OK.
> > >
> > > Thanks,
> > > Chenbo
> > >
> > > > +			"failed to kick guest, virtqueue busy.\n");
> > > > +		return -1;
> > > > +	}
> > > >
> > > >  	if (vq_is_packed(dev))
> > > >  		vhost_vring_call_packed(dev, vq);
> > > > --
> > > > 2.21.3

Re: [PATCH] vhost: use try_lock in rte_vhost_vring_call

[Maxime Coquelin]

On 9/20/22 04:53, Xia, Chenbo wrote:
>> -----Original Message-----
>> From: Liu, Changpeng <changpeng.liu@intel.com>
>> Sent: Tuesday, September 20, 2022 10:34 AM
>> To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org
>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Subject: RE: [PATCH] vhost: use try_lock in rte_vhost_vring_call
>>
>> Hi Bo,
>>
>>> -----Original Message-----
>>> From: Xia, Chenbo <chenbo.xia@intel.com>
>>> Sent: Tuesday, September 20, 2022 10:25 AM
>>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
>>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> Subject: RE: [PATCH] vhost: use try_lock in rte_vhost_vring_call
>>>
>>> Hi Changpeng,
>>>
>>>> -----Original Message-----
>>>> From: Liu, Changpeng <changpeng.liu@intel.com>
>>>> Sent: Tuesday, September 6, 2022 10:22 AM
>>>> To: dev@dpdk.org
>>>> Cc: Liu, Changpeng <changpeng.liu@intel.com>; Maxime Coquelin
>>>> <maxime.coquelin@redhat.com>; Xia, Chenbo <chenbo.xia@intel.com>
>>>> Subject: [PATCH] vhost: use try_lock in rte_vhost_vring_call
>>>>
>>>> Note that this function is in data path, so the thread context
>>>> may not same as socket messages processing context, by using
>>>> try_lock here, users can have another try in case of VQ's access
>>>> lock is held by `vhost-events` thread.
>>>
>>> Better to describe the issue this patch wants to fix and how does
>>> it fix.
>>>
>>> I remember it's a bz issue, do you want to backport? And it has
>>> some bz ID, we need to add it in commit message.
>> Actually it's my intention not to add bz ID, as I think for this bz ID,
>> It's better not to lock all VQ's access lock for KICK/CALLFD messages,
> 
> Do you plan to add this change? I think that may be an improvement to current
> locking implementation.
> 
> Maxime, what do you think of this idea about only locking specific queue when
> handling vring related message (not global config like mem table)?

I think this is not a good idea.
For example SET_VRING_KICK can currently call
translate_ring_addresses(), which itself can call numa_realloc().

numa_realloc() may reallocate the dev, so you don't want it to be used
by other queues while it happens.

>> What do you think? If this is identified as a fix, I can backport it to
>> 22.05.
> 
> You can decide, if this is planned to be the fix, just backport. I am just
> thinking if this is not the fix for the bz, do we still need this?
> 
> Thanks,
> Chenbo
> 
>>>
>>>>
>>>> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
>>>> ---
>>>>   lib/vhost/vhost.c | 6 +++++-
>>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
>>>> index 60cb05a0ff..072d2acb7b 100644
>>>> --- a/lib/vhost/vhost.c
>>>> +++ b/lib/vhost/vhost.c
>>>> @@ -1329,7 +1329,11 @@ rte_vhost_vring_call(int vid, uint16_t
>> vring_idx)
>>>>   	if (!vq)
>>>>   		return -1;
>>>>
>>>> -	rte_spinlock_lock(&vq->access_lock);
>>>> +	if (!rte_spinlock_trylock(&vq->access_lock)) {
>>>> +		VHOST_LOG_CONFIG(dev->ifname, DEBUG,
>>>
>>> Should use VHOST_LOG_DATA
>> OK.
>>>
>>> Thanks,
>>> Chenbo
>>>
>>>> +			"failed to kick guest, virtqueue busy.\n");
>>>> +		return -1;
>>>> +	}
>>>>
>>>>   	if (vq_is_packed(dev))
>>>>   		vhost_vring_call_packed(dev, vq);
>>>> --
>>>> 2.21.3
> 

Re: [PATCH] vhost: use try_lock in rte_vhost_vring_call

[Maxime Coquelin]

On 9/20/22 09:23, Maxime Coquelin wrote:
> 
> 
> On 9/20/22 04:53, Xia, Chenbo wrote:
>>> -----Original Message-----
>>> From: Liu, Changpeng <changpeng.liu@intel.com>
>>> Sent: Tuesday, September 20, 2022 10:34 AM
>>> To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org
>>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> Subject: RE: [PATCH] vhost: use try_lock in rte_vhost_vring_call
>>>
>>> Hi Bo,
>>>
>>>> -----Original Message-----
>>>> From: Xia, Chenbo <chenbo.xia@intel.com>
>>>> Sent: Tuesday, September 20, 2022 10:25 AM
>>>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
>>>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>> Subject: RE: [PATCH] vhost: use try_lock in rte_vhost_vring_call
>>>>
>>>> Hi Changpeng,
>>>>
>>>>> -----Original Message-----
>>>>> From: Liu, Changpeng <changpeng.liu@intel.com>
>>>>> Sent: Tuesday, September 6, 2022 10:22 AM
>>>>> To: dev@dpdk.org
>>>>> Cc: Liu, Changpeng <changpeng.liu@intel.com>; Maxime Coquelin
>>>>> <maxime.coquelin@redhat.com>; Xia, Chenbo <chenbo.xia@intel.com>
>>>>> Subject: [PATCH] vhost: use try_lock in rte_vhost_vring_call
>>>>>
>>>>> Note that this function is in data path, so the thread context
>>>>> may not same as socket messages processing context, by using
>>>>> try_lock here, users can have another try in case of VQ's access
>>>>> lock is held by `vhost-events` thread.
>>>>
>>>> Better to describe the issue this patch wants to fix and how does
>>>> it fix.
>>>>
>>>> I remember it's a bz issue, do you want to backport? And it has
>>>> some bz ID, we need to add it in commit message.
>>> Actually it's my intention not to add bz ID, as I think for this bz ID,
>>> It's better not to lock all VQ's access lock for KICK/CALLFD messages,
>>
>> Do you plan to add this change? I think that may be an improvement to 
>> current
>> locking implementation.
>>
>> Maxime, what do you think of this idea about only locking specific 
>> queue when
>> handling vring related message (not global config like mem table)?
> 
> I think this is not a good idea.
> For example SET_VRING_KICK can currently call
> translate_ring_addresses(), which itself can call numa_realloc().
> 
> numa_realloc() may reallocate the dev, so you don't want it to be used
> by other queues while it happens.

Hmm, actually that may be possible because numa_realloc() reallocs the 
dev only if it is not running.

So maybe you can propose something, but you will have to test it
carefully with use-cases involving NUMA reallocation.

>>> What do you think? If this is identified as a fix, I can backport it to
>>> 22.05.
>>
>> You can decide, if this is planned to be the fix, just backport. I am 
>> just
>> thinking if this is not the fix for the bz, do we still need this?
>>
>> Thanks,
>> Chenbo
>>
>>>>
>>>>>
>>>>> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
>>>>> ---
>>>>>   lib/vhost/vhost.c | 6 +++++-
>>>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
>>>>> index 60cb05a0ff..072d2acb7b 100644
>>>>> --- a/lib/vhost/vhost.c
>>>>> +++ b/lib/vhost/vhost.c
>>>>> @@ -1329,7 +1329,11 @@ rte_vhost_vring_call(int vid, uint16_t
>>> vring_idx)
>>>>>       if (!vq)
>>>>>           return -1;
>>>>>
>>>>> -    rte_spinlock_lock(&vq->access_lock);
>>>>> +    if (!rte_spinlock_trylock(&vq->access_lock)) {
>>>>> +        VHOST_LOG_CONFIG(dev->ifname, DEBUG,
>>>>
>>>> Should use VHOST_LOG_DATA
>>> OK.
>>>>
>>>> Thanks,
>>>> Chenbo
>>>>
>>>>> +            "failed to kick guest, virtqueue busy.\n");
>>>>> +        return -1;
>>>>> +    }
>>>>>
>>>>>       if (vq_is_packed(dev))
>>>>>           vhost_vring_call_packed(dev, vq);
>>>>> -- 
>>>>> 2.21.3
>>

Re: [PATCH] vhost: use try_lock in rte_vhost_vring_call

[Maxime Coquelin]

On 9/6/22 04:22, Changpeng Liu wrote:
> Note that this function is in data path, so the thread context
> may not same as socket messages processing context, by using
> try_lock here, users can have another try in case of VQ's access
> lock is held by `vhost-events` thread.
> 
> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> ---
>   lib/vhost/vhost.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> index 60cb05a0ff..072d2acb7b 100644
> --- a/lib/vhost/vhost.c
> +++ b/lib/vhost/vhost.c
> @@ -1329,7 +1329,11 @@ rte_vhost_vring_call(int vid, uint16_t vring_idx)
>   	if (!vq)
>   		return -1;
>   
> -	rte_spinlock_lock(&vq->access_lock);
> +	if (!rte_spinlock_trylock(&vq->access_lock)) {
> +		VHOST_LOG_CONFIG(dev->ifname, DEBUG,
> +			"failed to kick guest, virtqueue busy.\n");
> +		return -1;
> +	}
>   
>   	if (vq_is_packed(dev))
>   		vhost_vring_call_packed(dev, vq);

I think that's problematic, because it will break other applications
that currently rely on the API to block until the call is done.

Just some internal DPDK usage of this API:
./drivers/vdpa/ifc/ifcvf_vdpa.c:871:	rte_vhost_vring_call(internal->vid, 
qid);
./examples/vhost/virtio_net.c:236:	rte_vhost_vring_call(dev->vid, queue_id);
./examples/vhost/virtio_net.c:446:	rte_vhost_vring_call(dev->vid, queue_id);
./examples/vhost_blk/vhost_blk.c:99: 
rte_vhost_vring_call(task->ctrlr->vid, vq->id);
./examples/vhost_blk/vhost_blk.c:134: 
rte_vhost_vring_call(task->ctrlr->vid, vq->id);

This change will break all the above uses.

And that's not counting external projects.

ou should better introduce a new API that does not block.

Regards,
Maxime

RE: [PATCH] vhost: use try_lock in rte_vhost_vring_call

[Liu, Changpeng]

Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, September 20, 2022 3:19 PM
> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
> Cc: Xia, Chenbo <chenbo.xia@intel.com>
> Subject: Re: [PATCH] vhost: use try_lock in rte_vhost_vring_call
> 
> 
> 
> On 9/6/22 04:22, Changpeng Liu wrote:
> > Note that this function is in data path, so the thread context
> > may not same as socket messages processing context, by using
> > try_lock here, users can have another try in case of VQ's access
> > lock is held by `vhost-events` thread.
> >
> > Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> > ---
> >   lib/vhost/vhost.c | 6 +++++-
> >   1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> > index 60cb05a0ff..072d2acb7b 100644
> > --- a/lib/vhost/vhost.c
> > +++ b/lib/vhost/vhost.c
> > @@ -1329,7 +1329,11 @@ rte_vhost_vring_call(int vid, uint16_t vring_idx)
> >   	if (!vq)
> >   		return -1;
> >
> > -	rte_spinlock_lock(&vq->access_lock);
> > +	if (!rte_spinlock_trylock(&vq->access_lock)) {
> > +		VHOST_LOG_CONFIG(dev->ifname, DEBUG,
> > +			"failed to kick guest, virtqueue busy.\n");
> > +		return -1;
> > +	}
> >
> >   	if (vq_is_packed(dev))
> >   		vhost_vring_call_packed(dev, vq);
> 
> I think that's problematic, because it will break other applications
> that currently rely on the API to block until the call is done.
> 
> Just some internal DPDK usage of this API:
> ./drivers/vdpa/ifc/ifcvf_vdpa.c:871:	rte_vhost_vring_call(internal->vid,
> qid);
> ./examples/vhost/virtio_net.c:236:	rte_vhost_vring_call(dev->vid, queue_id);
> ./examples/vhost/virtio_net.c:446:	rte_vhost_vring_call(dev->vid, queue_id);
> ./examples/vhost_blk/vhost_blk.c:99:
> rte_vhost_vring_call(task->ctrlr->vid, vq->id);
> ./examples/vhost_blk/vhost_blk.c:134:
> rte_vhost_vring_call(task->ctrlr->vid, vq->id);
> 
> This change will break all the above uses.
> 
> And that's not counting external projects.
> 
> ou should better introduce a new API that does not block.
Could you add a new API to do this? 
I think we can use the new API in SPDK as a workaround, note that SPDK project is blocked for
a while which can't be used with DPDK 22.05 or newer.
Vhost-blk and scsi devices are not same with vhost-net, we need to cover SeaBIOS and VM
cases, so we need to start processing vrings after 1 vring is ready.
> 
> Regards,
> Maxime

Re: [PATCH] vhost: use try_lock in rte_vhost_vring_call

[Maxime Coquelin]

On 9/20/22 09:29, Liu, Changpeng wrote:
> Hi Maxime,
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Tuesday, September 20, 2022 3:19 PM
>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
>> Cc: Xia, Chenbo <chenbo.xia@intel.com>
>> Subject: Re: [PATCH] vhost: use try_lock in rte_vhost_vring_call
>>
>>
>>
>> On 9/6/22 04:22, Changpeng Liu wrote:
>>> Note that this function is in data path, so the thread context
>>> may not same as socket messages processing context, by using
>>> try_lock here, users can have another try in case of VQ's access
>>> lock is held by `vhost-events` thread.
>>>
>>> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
>>> ---
>>>    lib/vhost/vhost.c | 6 +++++-
>>>    1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
>>> index 60cb05a0ff..072d2acb7b 100644
>>> --- a/lib/vhost/vhost.c
>>> +++ b/lib/vhost/vhost.c
>>> @@ -1329,7 +1329,11 @@ rte_vhost_vring_call(int vid, uint16_t vring_idx)
>>>    	if (!vq)
>>>    		return -1;
>>>
>>> -	rte_spinlock_lock(&vq->access_lock);
>>> +	if (!rte_spinlock_trylock(&vq->access_lock)) {
>>> +		VHOST_LOG_CONFIG(dev->ifname, DEBUG,
>>> +			"failed to kick guest, virtqueue busy.\n");
>>> +		return -1;
>>> +	}
>>>
>>>    	if (vq_is_packed(dev))
>>>    		vhost_vring_call_packed(dev, vq);
>>
>> I think that's problematic, because it will break other applications
>> that currently rely on the API to block until the call is done.
>>
>> Just some internal DPDK usage of this API:
>> ./drivers/vdpa/ifc/ifcvf_vdpa.c:871:	rte_vhost_vring_call(internal->vid,
>> qid);
>> ./examples/vhost/virtio_net.c:236:	rte_vhost_vring_call(dev->vid, queue_id);
>> ./examples/vhost/virtio_net.c:446:	rte_vhost_vring_call(dev->vid, queue_id);
>> ./examples/vhost_blk/vhost_blk.c:99:
>> rte_vhost_vring_call(task->ctrlr->vid, vq->id);
>> ./examples/vhost_blk/vhost_blk.c:134:
>> rte_vhost_vring_call(task->ctrlr->vid, vq->id);
>>
>> This change will break all the above uses.
>>
>> And that's not counting external projects.
>>
>> ou should better introduce a new API that does not block.
> Could you add a new API to do this?
 >
> I think we can use the new API in SPDK as a workaround, note that SPDK project is blocked for
> a while which can't be used with DPDK 22.05 or newer.

DPDK v22.05?
What is the commit introducing the regression?

Note that if we introduce a new API, it won't be backported to stable
branches.


> Vhost-blk and scsi devices are not same with vhost-net, we need to cover SeaBIOS and VM
> cases, so we need to start processing vrings after 1 vring is ready.
>>
>> Regards,
>> Maxime
> 

RE: [PATCH] vhost: use try_lock in rte_vhost_vring_call

[Liu, Changpeng]

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, September 20, 2022 3:35 PM
> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
> Cc: Xia, Chenbo <chenbo.xia@intel.com>
> Subject: Re: [PATCH] vhost: use try_lock in rte_vhost_vring_call
> 
> 
> 
> On 9/20/22 09:29, Liu, Changpeng wrote:
> > Hi Maxime,
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Sent: Tuesday, September 20, 2022 3:19 PM
> >> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
> >> Cc: Xia, Chenbo <chenbo.xia@intel.com>
> >> Subject: Re: [PATCH] vhost: use try_lock in rte_vhost_vring_call
> >>
> >>
> >>
> >> On 9/6/22 04:22, Changpeng Liu wrote:
> >>> Note that this function is in data path, so the thread context
> >>> may not same as socket messages processing context, by using
> >>> try_lock here, users can have another try in case of VQ's access
> >>> lock is held by `vhost-events` thread.
> >>>
> >>> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> >>> ---
> >>>    lib/vhost/vhost.c | 6 +++++-
> >>>    1 file changed, 5 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> >>> index 60cb05a0ff..072d2acb7b 100644
> >>> --- a/lib/vhost/vhost.c
> >>> +++ b/lib/vhost/vhost.c
> >>> @@ -1329,7 +1329,11 @@ rte_vhost_vring_call(int vid, uint16_t vring_idx)
> >>>    	if (!vq)
> >>>    		return -1;
> >>>
> >>> -	rte_spinlock_lock(&vq->access_lock);
> >>> +	if (!rte_spinlock_trylock(&vq->access_lock)) {
> >>> +		VHOST_LOG_CONFIG(dev->ifname, DEBUG,
> >>> +			"failed to kick guest, virtqueue busy.\n");
> >>> +		return -1;
> >>> +	}
> >>>
> >>>    	if (vq_is_packed(dev))
> >>>    		vhost_vring_call_packed(dev, vq);
> >>
> >> I think that's problematic, because it will break other applications
> >> that currently rely on the API to block until the call is done.
> >>
> >> Just some internal DPDK usage of this API:
> >> ./drivers/vdpa/ifc/ifcvf_vdpa.c:871:	rte_vhost_vring_call(internal->vid,
> >> qid);
> >> ./examples/vhost/virtio_net.c:236:	rte_vhost_vring_call(dev->vid, queue_id);
> >> ./examples/vhost/virtio_net.c:446:	rte_vhost_vring_call(dev->vid, queue_id);
> >> ./examples/vhost_blk/vhost_blk.c:99:
> >> rte_vhost_vring_call(task->ctrlr->vid, vq->id);
> >> ./examples/vhost_blk/vhost_blk.c:134:
> >> rte_vhost_vring_call(task->ctrlr->vid, vq->id);
> >>
> >> This change will break all the above uses.
> >>
> >> And that's not counting external projects.
> >>
> >> ou should better introduce a new API that does not block.
> > Could you add a new API to do this?
>  >
> > I think we can use the new API in SPDK as a workaround, note that SPDK project
> is blocked for
> > a while which can't be used with DPDK 22.05 or newer.
> 
> DPDK v22.05?
> What is the commit introducing the regression?
Here is the commit introducing this issue
c5736998305d ("vhost: fix missing virtqueue lock protection")
Bugzilla ID: 1015
> 
> Note that if we introduce a new API, it won't be backported to stable
> branches.
I understand, but do we have better idea in short time? we're planning
to release SPDK 22.09 recently.
> 
> 
> > Vhost-blk and scsi devices are not same with vhost-net, we need to cover
> SeaBIOS and VM
> > cases, so we need to start processing vrings after 1 vring is ready.
> >>
> >> Regards,
> >> Maxime
> >

Re: [PATCH] vhost: use try_lock in rte_vhost_vring_call

[Maxime Coquelin]

On 9/20/22 09:45, Liu, Changpeng wrote:
> 
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Tuesday, September 20, 2022 3:35 PM
>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
>> Cc: Xia, Chenbo <chenbo.xia@intel.com>
>> Subject: Re: [PATCH] vhost: use try_lock in rte_vhost_vring_call
>>
>>
>>
>> On 9/20/22 09:29, Liu, Changpeng wrote:
>>> Hi Maxime,
>>>
>>>> -----Original Message-----
>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>> Sent: Tuesday, September 20, 2022 3:19 PM
>>>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
>>>> Cc: Xia, Chenbo <chenbo.xia@intel.com>
>>>> Subject: Re: [PATCH] vhost: use try_lock in rte_vhost_vring_call
>>>>
>>>>
>>>>
>>>> On 9/6/22 04:22, Changpeng Liu wrote:
>>>>> Note that this function is in data path, so the thread context
>>>>> may not same as socket messages processing context, by using
>>>>> try_lock here, users can have another try in case of VQ's access
>>>>> lock is held by `vhost-events` thread.
>>>>>
>>>>> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
>>>>> ---
>>>>>     lib/vhost/vhost.c | 6 +++++-
>>>>>     1 file changed, 5 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
>>>>> index 60cb05a0ff..072d2acb7b 100644
>>>>> --- a/lib/vhost/vhost.c
>>>>> +++ b/lib/vhost/vhost.c
>>>>> @@ -1329,7 +1329,11 @@ rte_vhost_vring_call(int vid, uint16_t vring_idx)
>>>>>     	if (!vq)
>>>>>     		return -1;
>>>>>
>>>>> -	rte_spinlock_lock(&vq->access_lock);
>>>>> +	if (!rte_spinlock_trylock(&vq->access_lock)) {
>>>>> +		VHOST_LOG_CONFIG(dev->ifname, DEBUG,
>>>>> +			"failed to kick guest, virtqueue busy.\n");
>>>>> +		return -1;
>>>>> +	}
>>>>>
>>>>>     	if (vq_is_packed(dev))
>>>>>     		vhost_vring_call_packed(dev, vq);
>>>>
>>>> I think that's problematic, because it will break other applications
>>>> that currently rely on the API to block until the call is done.
>>>>
>>>> Just some internal DPDK usage of this API:
>>>> ./drivers/vdpa/ifc/ifcvf_vdpa.c:871:	rte_vhost_vring_call(internal->vid,
>>>> qid);
>>>> ./examples/vhost/virtio_net.c:236:	rte_vhost_vring_call(dev->vid, queue_id);
>>>> ./examples/vhost/virtio_net.c:446:	rte_vhost_vring_call(dev->vid, queue_id);
>>>> ./examples/vhost_blk/vhost_blk.c:99:
>>>> rte_vhost_vring_call(task->ctrlr->vid, vq->id);
>>>> ./examples/vhost_blk/vhost_blk.c:134:
>>>> rte_vhost_vring_call(task->ctrlr->vid, vq->id);
>>>>
>>>> This change will break all the above uses.
>>>>
>>>> And that's not counting external projects.
>>>>
>>>> ou should better introduce a new API that does not block.
>>> Could you add a new API to do this?
>>   >
>>> I think we can use the new API in SPDK as a workaround, note that SPDK project
>> is blocked for
>>> a while which can't be used with DPDK 22.05 or newer.
>>
>> DPDK v22.05?
>> What is the commit introducing the regression?
> Here is the commit introducing this issue
> c5736998305d ("vhost: fix missing virtqueue lock protection")
> Bugzilla ID: 1015

Ok, it cannot be reverted, as it prevents some undefined
behaviors/crashes.

>>
>> Note that if we introduce a new API, it won't be backported to stable
>> branches.
> I understand, but do we have better idea in short time? we're planning
> to release SPDK 22.09 recently.

You can have another thread that sends the call?

>>
>>
>>> Vhost-blk and scsi devices are not same with vhost-net, we need to cover
>> SeaBIOS and VM
>>> cases, so we need to start processing vrings after 1 vring is ready.
>>>>
>>>> Regards,
>>>> Maxime
>>>
> 

RE: [PATCH] vhost: use try_lock in rte_vhost_vring_call

[Liu, Changpeng]

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, September 20, 2022 4:13 PM
> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
> Cc: Xia, Chenbo <chenbo.xia@intel.com>
> Subject: Re: [PATCH] vhost: use try_lock in rte_vhost_vring_call
> 
> 
> 
> On 9/20/22 09:45, Liu, Changpeng wrote:
> >
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Sent: Tuesday, September 20, 2022 3:35 PM
> >> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
> >> Cc: Xia, Chenbo <chenbo.xia@intel.com>
> >> Subject: Re: [PATCH] vhost: use try_lock in rte_vhost_vring_call
> >>
> >>
> >>
> >> On 9/20/22 09:29, Liu, Changpeng wrote:
> >>> Hi Maxime,
> >>>
> >>>> -----Original Message-----
> >>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>> Sent: Tuesday, September 20, 2022 3:19 PM
> >>>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
> >>>> Cc: Xia, Chenbo <chenbo.xia@intel.com>
> >>>> Subject: Re: [PATCH] vhost: use try_lock in rte_vhost_vring_call
> >>>>
> >>>>
> >>>>
> >>>> On 9/6/22 04:22, Changpeng Liu wrote:
> >>>>> Note that this function is in data path, so the thread context
> >>>>> may not same as socket messages processing context, by using
> >>>>> try_lock here, users can have another try in case of VQ's access
> >>>>> lock is held by `vhost-events` thread.
> >>>>>
> >>>>> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> >>>>> ---
> >>>>>     lib/vhost/vhost.c | 6 +++++-
> >>>>>     1 file changed, 5 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> >>>>> index 60cb05a0ff..072d2acb7b 100644
> >>>>> --- a/lib/vhost/vhost.c
> >>>>> +++ b/lib/vhost/vhost.c
> >>>>> @@ -1329,7 +1329,11 @@ rte_vhost_vring_call(int vid, uint16_t vring_idx)
> >>>>>     	if (!vq)
> >>>>>     		return -1;
> >>>>>
> >>>>> -	rte_spinlock_lock(&vq->access_lock);
> >>>>> +	if (!rte_spinlock_trylock(&vq->access_lock)) {
> >>>>> +		VHOST_LOG_CONFIG(dev->ifname, DEBUG,
> >>>>> +			"failed to kick guest, virtqueue busy.\n");
> >>>>> +		return -1;
> >>>>> +	}
> >>>>>
> >>>>>     	if (vq_is_packed(dev))
> >>>>>     		vhost_vring_call_packed(dev, vq);
> >>>>
> >>>> I think that's problematic, because it will break other applications
> >>>> that currently rely on the API to block until the call is done.
> >>>>
> >>>> Just some internal DPDK usage of this API:
> >>>> ./drivers/vdpa/ifc/ifcvf_vdpa.c:871:	rte_vhost_vring_call(internal->vid,
> >>>> qid);
> >>>> ./examples/vhost/virtio_net.c:236:	rte_vhost_vring_call(dev->vid,
> queue_id);
> >>>> ./examples/vhost/virtio_net.c:446:	rte_vhost_vring_call(dev->vid,
> queue_id);
> >>>> ./examples/vhost_blk/vhost_blk.c:99:
> >>>> rte_vhost_vring_call(task->ctrlr->vid, vq->id);
> >>>> ./examples/vhost_blk/vhost_blk.c:134:
> >>>> rte_vhost_vring_call(task->ctrlr->vid, vq->id);
> >>>>
> >>>> This change will break all the above uses.
> >>>>
> >>>> And that's not counting external projects.
> >>>>
> >>>> ou should better introduce a new API that does not block.
> >>> Could you add a new API to do this?
> >>   >
> >>> I think we can use the new API in SPDK as a workaround, note that SPDK
> project
> >> is blocked for
> >>> a while which can't be used with DPDK 22.05 or newer.
> >>
> >> DPDK v22.05?
> >> What is the commit introducing the regression?
> > Here is the commit introducing this issue
> > c5736998305d ("vhost: fix missing virtqueue lock protection")
> > Bugzilla ID: 1015
> 
> Ok, it cannot be reverted, as it prevents some undefined
> behaviors/crashes.
> 
> >>
> >> Note that if we introduce a new API, it won't be backported to stable
> >> branches.
> > I understand, but do we have better idea in short time? we're planning
> > to release SPDK 22.09 recently.
> 
> You can have another thread that sends the call?
We already use two threads to do this. Here is the example for existing code in SPDK:

DPDK vhost-events thread                        SPDK thread

    SET_VRING_KICK VQ1       ---->            Start polling VQ1
    Reply to DPDK                    <----              Done
    SET_VRING_KICK VQ2       ---->            thread is blocked on VQ's access lock, SPDK thread can't provide reply message                               
 
For example, we can just return for  SET_VRING_KICK VQ2 message without checking SPDK thread, but this leave
uncertain replies to VM.
> 
> >>
> >>
> >>> Vhost-blk and scsi devices are not same with vhost-net, we need to cover
> >> SeaBIOS and VM
> >>> cases, so we need to start processing vrings after 1 vring is ready.
> >>>>
> >>>> Regards,
> >>>> Maxime
> >>>
> >

Re: [PATCH] vhost: use try_lock in rte_vhost_vring_call

[Maxime Coquelin]

On 9/20/22 10:43, Liu, Changpeng wrote:
> 
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Tuesday, September 20, 2022 4:13 PM
>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
>> Cc: Xia, Chenbo <chenbo.xia@intel.com>
>> Subject: Re: [PATCH] vhost: use try_lock in rte_vhost_vring_call
>>
>>
>>
>> On 9/20/22 09:45, Liu, Changpeng wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>> Sent: Tuesday, September 20, 2022 3:35 PM
>>>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
>>>> Cc: Xia, Chenbo <chenbo.xia@intel.com>
>>>> Subject: Re: [PATCH] vhost: use try_lock in rte_vhost_vring_call
>>>>
>>>>
>>>>
>>>> On 9/20/22 09:29, Liu, Changpeng wrote:
>>>>> Hi Maxime,
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>>> Sent: Tuesday, September 20, 2022 3:19 PM
>>>>>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
>>>>>> Cc: Xia, Chenbo <chenbo.xia@intel.com>
>>>>>> Subject: Re: [PATCH] vhost: use try_lock in rte_vhost_vring_call
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 9/6/22 04:22, Changpeng Liu wrote:
>>>>>>> Note that this function is in data path, so the thread context
>>>>>>> may not same as socket messages processing context, by using
>>>>>>> try_lock here, users can have another try in case of VQ's access
>>>>>>> lock is held by `vhost-events` thread.
>>>>>>>
>>>>>>> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
>>>>>>> ---
>>>>>>>      lib/vhost/vhost.c | 6 +++++-
>>>>>>>      1 file changed, 5 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
>>>>>>> index 60cb05a0ff..072d2acb7b 100644
>>>>>>> --- a/lib/vhost/vhost.c
>>>>>>> +++ b/lib/vhost/vhost.c
>>>>>>> @@ -1329,7 +1329,11 @@ rte_vhost_vring_call(int vid, uint16_t vring_idx)
>>>>>>>      	if (!vq)
>>>>>>>      		return -1;
>>>>>>>
>>>>>>> -	rte_spinlock_lock(&vq->access_lock);
>>>>>>> +	if (!rte_spinlock_trylock(&vq->access_lock)) {
>>>>>>> +		VHOST_LOG_CONFIG(dev->ifname, DEBUG,
>>>>>>> +			"failed to kick guest, virtqueue busy.\n");
>>>>>>> +		return -1;
>>>>>>> +	}
>>>>>>>
>>>>>>>      	if (vq_is_packed(dev))
>>>>>>>      		vhost_vring_call_packed(dev, vq);
>>>>>>
>>>>>> I think that's problematic, because it will break other applications
>>>>>> that currently rely on the API to block until the call is done.
>>>>>>
>>>>>> Just some internal DPDK usage of this API:
>>>>>> ./drivers/vdpa/ifc/ifcvf_vdpa.c:871:	rte_vhost_vring_call(internal->vid,
>>>>>> qid);
>>>>>> ./examples/vhost/virtio_net.c:236:	rte_vhost_vring_call(dev->vid,
>> queue_id);
>>>>>> ./examples/vhost/virtio_net.c:446:	rte_vhost_vring_call(dev->vid,
>> queue_id);
>>>>>> ./examples/vhost_blk/vhost_blk.c:99:
>>>>>> rte_vhost_vring_call(task->ctrlr->vid, vq->id);
>>>>>> ./examples/vhost_blk/vhost_blk.c:134:
>>>>>> rte_vhost_vring_call(task->ctrlr->vid, vq->id);
>>>>>>
>>>>>> This change will break all the above uses.
>>>>>>
>>>>>> And that's not counting external projects.
>>>>>>
>>>>>> ou should better introduce a new API that does not block.
>>>>> Could you add a new API to do this?
>>>>    >
>>>>> I think we can use the new API in SPDK as a workaround, note that SPDK
>> project
>>>> is blocked for
>>>>> a while which can't be used with DPDK 22.05 or newer.
>>>>
>>>> DPDK v22.05?
>>>> What is the commit introducing the regression?
>>> Here is the commit introducing this issue
>>> c5736998305d ("vhost: fix missing virtqueue lock protection")
>>> Bugzilla ID: 1015
>>
>> Ok, it cannot be reverted, as it prevents some undefined
>> behaviors/crashes.
>>
>>>>
>>>> Note that if we introduce a new API, it won't be backported to stable
>>>> branches.
>>> I understand, but do we have better idea in short time? we're planning
>>> to release SPDK 22.09 recently.
>>
>> You can have another thread that sends the call?
> We already use two threads to do this. Here is the example for existing code in SPDK:
> 
> DPDK vhost-events thread                        SPDK thread
> 
>      SET_VRING_KICK VQ1       ---->            Start polling VQ1
>      Reply to DPDK                    <----              Done
>      SET_VRING_KICK VQ2       ---->            thread is blocked on VQ's access lock, SPDK thread can't provide reply message
>   
> For example, we can just return for  SET_VRING_KICK VQ2 message without checking SPDK thread, but this leave
> uncertain replies to VM.

I'm sorry but you will have to find a workaround while v22.11 is out and
you can consume it. We can neither backport new API nor we can break all
the other applications not handling locking failure.

Regarding the new API for v22.11, I should be named something like
rte_vhost_vring_call_nonblock(), and ideally should return some like
-EAGAIN instead of -1 o that the applications can distinguish between a
real failure and a need for retry.

Regards,
Maxime

>>
>>>>
>>>>
>>>>> Vhost-blk and scsi devices are not same with vhost-net, we need to cover
>>>> SeaBIOS and VM
>>>>> cases, so we need to start processing vrings after 1 vring is ready.
>>>>>>
>>>>>> Regards,
>>>>>> Maxime
>>>>>
>>>
> 

RE: [PATCH] vhost: use try_lock in rte_vhost_vring_call

[Liu, Changpeng]

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Wednesday, September 21, 2022 5:41 PM
> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
> Cc: Xia, Chenbo <chenbo.xia@intel.com>
> Subject: Re: [PATCH] vhost: use try_lock in rte_vhost_vring_call
> 
> 
> 
> On 9/20/22 10:43, Liu, Changpeng wrote:
> >
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Sent: Tuesday, September 20, 2022 4:13 PM
> >> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
> >> Cc: Xia, Chenbo <chenbo.xia@intel.com>
> >> Subject: Re: [PATCH] vhost: use try_lock in rte_vhost_vring_call
> >>
> >>
> >>
> >> On 9/20/22 09:45, Liu, Changpeng wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>> Sent: Tuesday, September 20, 2022 3:35 PM
> >>>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
> >>>> Cc: Xia, Chenbo <chenbo.xia@intel.com>
> >>>> Subject: Re: [PATCH] vhost: use try_lock in rte_vhost_vring_call
> >>>>
> >>>>
> >>>>
> >>>> On 9/20/22 09:29, Liu, Changpeng wrote:
> >>>>> Hi Maxime,
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>>>> Sent: Tuesday, September 20, 2022 3:19 PM
> >>>>>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
> >>>>>> Cc: Xia, Chenbo <chenbo.xia@intel.com>
> >>>>>> Subject: Re: [PATCH] vhost: use try_lock in rte_vhost_vring_call
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 9/6/22 04:22, Changpeng Liu wrote:
> >>>>>>> Note that this function is in data path, so the thread context
> >>>>>>> may not same as socket messages processing context, by using
> >>>>>>> try_lock here, users can have another try in case of VQ's access
> >>>>>>> lock is held by `vhost-events` thread.
> >>>>>>>
> >>>>>>> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> >>>>>>> ---
> >>>>>>>      lib/vhost/vhost.c | 6 +++++-
> >>>>>>>      1 file changed, 5 insertions(+), 1 deletion(-)
> >>>>>>>
> >>>>>>> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> >>>>>>> index 60cb05a0ff..072d2acb7b 100644
> >>>>>>> --- a/lib/vhost/vhost.c
> >>>>>>> +++ b/lib/vhost/vhost.c
> >>>>>>> @@ -1329,7 +1329,11 @@ rte_vhost_vring_call(int vid, uint16_t
> vring_idx)
> >>>>>>>      	if (!vq)
> >>>>>>>      		return -1;
> >>>>>>>
> >>>>>>> -	rte_spinlock_lock(&vq->access_lock);
> >>>>>>> +	if (!rte_spinlock_trylock(&vq->access_lock)) {
> >>>>>>> +		VHOST_LOG_CONFIG(dev->ifname, DEBUG,
> >>>>>>> +			"failed to kick guest, virtqueue busy.\n");
> >>>>>>> +		return -1;
> >>>>>>> +	}
> >>>>>>>
> >>>>>>>      	if (vq_is_packed(dev))
> >>>>>>>      		vhost_vring_call_packed(dev, vq);
> >>>>>>
> >>>>>> I think that's problematic, because it will break other applications
> >>>>>> that currently rely on the API to block until the call is done.
> >>>>>>
> >>>>>> Just some internal DPDK usage of this API:
> >>>>>> ./drivers/vdpa/ifc/ifcvf_vdpa.c:871:	rte_vhost_vring_call(internal->vid,
> >>>>>> qid);
> >>>>>> ./examples/vhost/virtio_net.c:236:	rte_vhost_vring_call(dev->vid,
> >> queue_id);
> >>>>>> ./examples/vhost/virtio_net.c:446:	rte_vhost_vring_call(dev->vid,
> >> queue_id);
> >>>>>> ./examples/vhost_blk/vhost_blk.c:99:
> >>>>>> rte_vhost_vring_call(task->ctrlr->vid, vq->id);
> >>>>>> ./examples/vhost_blk/vhost_blk.c:134:
> >>>>>> rte_vhost_vring_call(task->ctrlr->vid, vq->id);
> >>>>>>
> >>>>>> This change will break all the above uses.
> >>>>>>
> >>>>>> And that's not counting external projects.
> >>>>>>
> >>>>>> ou should better introduce a new API that does not block.
> >>>>> Could you add a new API to do this?
> >>>>    >
> >>>>> I think we can use the new API in SPDK as a workaround, note that SPDK
> >> project
> >>>> is blocked for
> >>>>> a while which can't be used with DPDK 22.05 or newer.
> >>>>
> >>>> DPDK v22.05?
> >>>> What is the commit introducing the regression?
> >>> Here is the commit introducing this issue
> >>> c5736998305d ("vhost: fix missing virtqueue lock protection")
> >>> Bugzilla ID: 1015
> >>
> >> Ok, it cannot be reverted, as it prevents some undefined
> >> behaviors/crashes.
> >>
> >>>>
> >>>> Note that if we introduce a new API, it won't be backported to stable
> >>>> branches.
> >>> I understand, but do we have better idea in short time? we're planning
> >>> to release SPDK 22.09 recently.
> >>
> >> You can have another thread that sends the call?
> > We already use two threads to do this. Here is the example for existing code in
> SPDK:
> >
> > DPDK vhost-events thread                        SPDK thread
> >
> >      SET_VRING_KICK VQ1       ---->            Start polling VQ1
> >      Reply to DPDK                    <----              Done
> >      SET_VRING_KICK VQ2       ---->            thread is blocked on VQ's access lock,
> SPDK thread can't provide reply message
> >
> > For example, we can just return for  SET_VRING_KICK VQ2 message without
> checking SPDK thread, but this leave
> > uncertain replies to VM.
> 
> I'm sorry but you will have to find a workaround while v22.11 is out and
> you can consume it. We can neither backport new API nor we can break all
> the other applications not handling locking failure.
By processing vhost-user message in asynchronous way in SPDK can be a
workaround now, we can backport the workaround to SPDK earlier version
so that it can work with distro DPDK releases.
> 
> Regarding the new API for v22.11, I should be named something like
> rte_vhost_vring_call_nonblock(), and ideally should return some like
> -EAGAIN instead of -1 o that the applications can distinguish between a
> real failure and a need for retry.
Agreed, then we can switch to the new API finally.
> 
> Regards,
> Maxime
> 
> >>
> >>>>
> >>>>
> >>>>> Vhost-blk and scsi devices are not same with vhost-net, we need to cover
> >>>> SeaBIOS and VM
> >>>>> cases, so we need to start processing vrings after 1 vring is ready.
> >>>>>>
> >>>>>> Regards,
> >>>>>> Maxime
> >>>>>
> >>>
> >

Re: [PATCH] vhost: use try_lock in rte_vhost_vring_call

[Maxime Coquelin]

Hi Changpeng,

On 9/21/22 11:52, Liu, Changpeng wrote:
> 
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Wednesday, September 21, 2022 5:41 PM
>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
>> Cc: Xia, Chenbo <chenbo.xia@intel.com>
>> Subject: Re: [PATCH] vhost: use try_lock in rte_vhost_vring_call
>>
>>
>>
>> On 9/20/22 10:43, Liu, Changpeng wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>> Sent: Tuesday, September 20, 2022 4:13 PM
>>>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
>>>> Cc: Xia, Chenbo <chenbo.xia@intel.com>
>>>> Subject: Re: [PATCH] vhost: use try_lock in rte_vhost_vring_call
>>>>
>>>>
>>>>
>>>> On 9/20/22 09:45, Liu, Changpeng wrote:
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>>> Sent: Tuesday, September 20, 2022 3:35 PM
>>>>>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
>>>>>> Cc: Xia, Chenbo <chenbo.xia@intel.com>
>>>>>> Subject: Re: [PATCH] vhost: use try_lock in rte_vhost_vring_call
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 9/20/22 09:29, Liu, Changpeng wrote:
>>>>>>> Hi Maxime,
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>>>>> Sent: Tuesday, September 20, 2022 3:19 PM
>>>>>>>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
>>>>>>>> Cc: Xia, Chenbo <chenbo.xia@intel.com>
>>>>>>>> Subject: Re: [PATCH] vhost: use try_lock in rte_vhost_vring_call
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 9/6/22 04:22, Changpeng Liu wrote:
>>>>>>>>> Note that this function is in data path, so the thread context
>>>>>>>>> may not same as socket messages processing context, by using
>>>>>>>>> try_lock here, users can have another try in case of VQ's access
>>>>>>>>> lock is held by `vhost-events` thread.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
>>>>>>>>> ---
>>>>>>>>>       lib/vhost/vhost.c | 6 +++++-
>>>>>>>>>       1 file changed, 5 insertions(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
>>>>>>>>> index 60cb05a0ff..072d2acb7b 100644
>>>>>>>>> --- a/lib/vhost/vhost.c
>>>>>>>>> +++ b/lib/vhost/vhost.c
>>>>>>>>> @@ -1329,7 +1329,11 @@ rte_vhost_vring_call(int vid, uint16_t
>> vring_idx)
>>>>>>>>>       	if (!vq)
>>>>>>>>>       		return -1;
>>>>>>>>>
>>>>>>>>> -	rte_spinlock_lock(&vq->access_lock);
>>>>>>>>> +	if (!rte_spinlock_trylock(&vq->access_lock)) {
>>>>>>>>> +		VHOST_LOG_CONFIG(dev->ifname, DEBUG,
>>>>>>>>> +			"failed to kick guest, virtqueue busy.\n");
>>>>>>>>> +		return -1;
>>>>>>>>> +	}
>>>>>>>>>
>>>>>>>>>       	if (vq_is_packed(dev))
>>>>>>>>>       		vhost_vring_call_packed(dev, vq);
>>>>>>>>
>>>>>>>> I think that's problematic, because it will break other applications
>>>>>>>> that currently rely on the API to block until the call is done.
>>>>>>>>
>>>>>>>> Just some internal DPDK usage of this API:
>>>>>>>> ./drivers/vdpa/ifc/ifcvf_vdpa.c:871:	rte_vhost_vring_call(internal->vid,
>>>>>>>> qid);
>>>>>>>> ./examples/vhost/virtio_net.c:236:	rte_vhost_vring_call(dev->vid,
>>>> queue_id);
>>>>>>>> ./examples/vhost/virtio_net.c:446:	rte_vhost_vring_call(dev->vid,
>>>> queue_id);
>>>>>>>> ./examples/vhost_blk/vhost_blk.c:99:
>>>>>>>> rte_vhost_vring_call(task->ctrlr->vid, vq->id);
>>>>>>>> ./examples/vhost_blk/vhost_blk.c:134:
>>>>>>>> rte_vhost_vring_call(task->ctrlr->vid, vq->id);
>>>>>>>>
>>>>>>>> This change will break all the above uses.
>>>>>>>>
>>>>>>>> And that's not counting external projects.
>>>>>>>>
>>>>>>>> ou should better introduce a new API that does not block.
>>>>>>> Could you add a new API to do this?
>>>>>>     >
>>>>>>> I think we can use the new API in SPDK as a workaround, note that SPDK
>>>> project
>>>>>> is blocked for
>>>>>>> a while which can't be used with DPDK 22.05 or newer.
>>>>>>
>>>>>> DPDK v22.05?
>>>>>> What is the commit introducing the regression?
>>>>> Here is the commit introducing this issue
>>>>> c5736998305d ("vhost: fix missing virtqueue lock protection")
>>>>> Bugzilla ID: 1015
>>>>
>>>> Ok, it cannot be reverted, as it prevents some undefined
>>>> behaviors/crashes.
>>>>
>>>>>>
>>>>>> Note that if we introduce a new API, it won't be backported to stable
>>>>>> branches.
>>>>> I understand, but do we have better idea in short time? we're planning
>>>>> to release SPDK 22.09 recently.
>>>>
>>>> You can have another thread that sends the call?
>>> We already use two threads to do this. Here is the example for existing code in
>> SPDK:
>>>
>>> DPDK vhost-events thread                        SPDK thread
>>>
>>>       SET_VRING_KICK VQ1       ---->            Start polling VQ1
>>>       Reply to DPDK                    <----              Done
>>>       SET_VRING_KICK VQ2       ---->            thread is blocked on VQ's access lock,
>> SPDK thread can't provide reply message
>>>
>>> For example, we can just return for  SET_VRING_KICK VQ2 message without
>> checking SPDK thread, but this leave
>>> uncertain replies to VM.
>>
>> I'm sorry but you will have to find a workaround while v22.11 is out and
>> you can consume it. We can neither backport new API nor we can break all
>> the other applications not handling locking failure.
> By processing vhost-user message in asynchronous way in SPDK can be a
> workaround now, we can backport the workaround to SPDK earlier version
> so that it can work with distro DPDK releases.
>>
>> Regarding the new API for v22.11, I should be named something like
>> rte_vhost_vring_call_nonblock(), and ideally should return some like
>> -EAGAIN instead of -1 o that the applications can distinguish between a
>> real failure and a need for retry.
> Agreed, then we can switch to the new API finally.

Just a reminder that -rc2 is in ~ two weeks, have you prepared the patch
adding the new API?

Regards,
Maxime

>> Regards,
>> Maxime
>>
>>>>
>>>>>>
>>>>>>
>>>>>>> Vhost-blk and scsi devices are not same with vhost-net, we need to cover
>>>>>> SeaBIOS and VM
>>>>>>> cases, so we need to start processing vrings after 1 vring is ready.
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Maxime
>>>>>>>
>>>>>
>>>
> 

[PATCH v2] vhost: add new `rte_vhost_vring_call_nonblock` API

[Changpeng Liu]

Vhost-user library locks all VQ's access lock when processing
vring based messages, such as SET_VRING_KICK and SET_VRING_CALL,
and the data processing thread may already be started, e.g: SPDK
vhost-blk and vhost-scsi will start the data processing thread
when one vring is ready, then deadlock may happen when SPDK is
posting interrupts to VM.  Here, we add a new API which allows
caller to try again later for this case.

Bugzilla ID: 1015
Fixes: c5736998305d ("vhost: fix missing virtqueue lock protection")

Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
---
 lib/vhost/rte_vhost.h | 15 +++++++++++++++
 lib/vhost/version.map |  3 +++
 lib/vhost/vhost.c     | 30 ++++++++++++++++++++++++++++++
 3 files changed, 48 insertions(+)

diff --git a/lib/vhost/rte_vhost.h b/lib/vhost/rte_vhost.h
index bb7d86a432..d22b25cd4e 100644
--- a/lib/vhost/rte_vhost.h
+++ b/lib/vhost/rte_vhost.h
@@ -909,6 +909,21 @@ rte_vhost_clr_inflight_desc_packed(int vid, uint16_t vring_idx,
  */
 int rte_vhost_vring_call(int vid, uint16_t vring_idx);
 
+/**
+ * Notify the guest that used descriptors have been added to the vring.  This
+ * function acts as a memory barrier.  This function will return -EAGAIN when
+ * vq's access lock is held by other thread, user should try again later.
+ *
+ * @param vid
+ *  vhost device ID
+ * @param vring_idx
+ *  vring index
+ * @return
+ *  0 on success, -1 on failure, -EAGAIN for another retry
+ */
+__rte_experimental
+int rte_vhost_vring_call_nonblock(int vid, uint16_t vring_idx);
+
 /**
  * Get vhost RX queue avail count.
  *
diff --git a/lib/vhost/version.map b/lib/vhost/version.map
index 7a00b65740..c8c44b8326 100644
--- a/lib/vhost/version.map
+++ b/lib/vhost/version.map
@@ -94,6 +94,9 @@ EXPERIMENTAL {
 	rte_vhost_async_try_dequeue_burst;
 	rte_vhost_driver_get_vdpa_dev_type;
 	rte_vhost_clear_queue;
+
+	# added in 22.11
+	rte_vhost_vring_call_nonblock;
 };
 
 INTERNAL {
diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
index 8740aa2788..ed6efb003f 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -1317,6 +1317,36 @@ rte_vhost_vring_call(int vid, uint16_t vring_idx)
 	return 0;
 }
 
+int
+rte_vhost_vring_call_nonblock(int vid, uint16_t vring_idx)
+{
+	struct virtio_net *dev;
+	struct vhost_virtqueue *vq;
+
+	dev = get_device(vid);
+	if (!dev)
+		return -1;
+
+	if (vring_idx >= VHOST_MAX_VRING)
+		return -1;
+
+	vq = dev->virtqueue[vring_idx];
+	if (!vq)
+		return -1;
+
+	if (!rte_spinlock_trylock(&vq->access_lock))
+		return -EAGAIN;
+
+	if (vq_is_packed(dev))
+		vhost_vring_call_packed(dev, vq);
+	else
+		vhost_vring_call_split(dev, vq);
+
+	rte_spinlock_unlock(&vq->access_lock);
+
+	return 0;
+}
+
 uint16_t
 rte_vhost_avail_entries(int vid, uint16_t queue_id)
 {
-- 
2.21.3

Re: [PATCH v2] vhost: add new `rte_vhost_vring_call_nonblock` API

[Maxime Coquelin]

Hi Changpeng,

On 10/12/22 08:40, Changpeng Liu wrote:
> Vhost-user library locks all VQ's access lock when processing
> vring based messages, such as SET_VRING_KICK and SET_VRING_CALL,
> and the data processing thread may already be started, e.g: SPDK
> vhost-blk and vhost-scsi will start the data processing thread
> when one vring is ready, then deadlock may happen when SPDK is
> posting interrupts to VM.  Here, we add a new API which allows
> caller to try again later for this case.
> 
> Bugzilla ID: 1015
> Fixes: c5736998305d ("vhost: fix missing virtqueue lock protection")
> 
> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> ---
>   lib/vhost/rte_vhost.h | 15 +++++++++++++++
>   lib/vhost/version.map |  3 +++
>   lib/vhost/vhost.c     | 30 ++++++++++++++++++++++++++++++
>   3 files changed, 48 insertions(+)
> 
> diff --git a/lib/vhost/rte_vhost.h b/lib/vhost/rte_vhost.h
> index bb7d86a432..d22b25cd4e 100644
> --- a/lib/vhost/rte_vhost.h
> +++ b/lib/vhost/rte_vhost.h
> @@ -909,6 +909,21 @@ rte_vhost_clr_inflight_desc_packed(int vid, uint16_t vring_idx,
>    */
>   int rte_vhost_vring_call(int vid, uint16_t vring_idx);
>   
> +/**
> + * Notify the guest that used descriptors have been added to the vring.  This
> + * function acts as a memory barrier.  This function will return -EAGAIN when
> + * vq's access lock is held by other thread, user should try again later.
> + *
> + * @param vid
> + *  vhost device ID
> + * @param vring_idx
> + *  vring index
> + * @return
> + *  0 on success, -1 on failure, -EAGAIN for another retry
> + */
> +__rte_experimental
> +int rte_vhost_vring_call_nonblock(int vid, uint16_t vring_idx);
> +
>   /**
>    * Get vhost RX queue avail count.
>    *
> diff --git a/lib/vhost/version.map b/lib/vhost/version.map
> index 7a00b65740..c8c44b8326 100644
> --- a/lib/vhost/version.map
> +++ b/lib/vhost/version.map
> @@ -94,6 +94,9 @@ EXPERIMENTAL {
>   	rte_vhost_async_try_dequeue_burst;
>   	rte_vhost_driver_get_vdpa_dev_type;
>   	rte_vhost_clear_queue;
> +
> +	# added in 22.11
> +	rte_vhost_vring_call_nonblock;
>   };
>   
>   INTERNAL {
> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> index 8740aa2788..ed6efb003f 100644
> --- a/lib/vhost/vhost.c
> +++ b/lib/vhost/vhost.c
> @@ -1317,6 +1317,36 @@ rte_vhost_vring_call(int vid, uint16_t vring_idx)
>   	return 0;
>   }
>   
> +int
> +rte_vhost_vring_call_nonblock(int vid, uint16_t vring_idx)
> +{
> +	struct virtio_net *dev;
> +	struct vhost_virtqueue *vq;
> +
> +	dev = get_device(vid);
> +	if (!dev)
> +		return -1;
> +
> +	if (vring_idx >= VHOST_MAX_VRING)
> +		return -1;
> +
> +	vq = dev->virtqueue[vring_idx];
> +	if (!vq)
> +		return -1;
> +
> +	if (!rte_spinlock_trylock(&vq->access_lock))
> +		return -EAGAIN;
> +
> +	if (vq_is_packed(dev))
> +		vhost_vring_call_packed(dev, vq);
> +	else
> +		vhost_vring_call_split(dev, vq);
> +
> +	rte_spinlock_unlock(&vq->access_lock);
> +
> +	return 0;
> +}
> +
>   uint16_t
>   rte_vhost_avail_entries(int vid, uint16_t queue_id)
>   {

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

Thanks,
Maxime

RE: [PATCH v2] vhost: add new `rte_vhost_vring_call_nonblock` API

[Xia, Chenbo]

Hi Changpeng,

> -----Original Message-----
> From: Liu, Changpeng <changpeng.liu@intel.com>
> Sent: Wednesday, October 12, 2022 2:40 PM
> To: dev@dpdk.org
> Cc: Liu, Changpeng <changpeng.liu@intel.com>; Maxime Coquelin
> <maxime.coquelin@redhat.com>; Xia, Chenbo <chenbo.xia@intel.com>; David
> Marchand <david.marchand@redhat.com>
> Subject: [PATCH v2] vhost: add new `rte_vhost_vring_call_nonblock` API
> 
> Vhost-user library locks all VQ's access lock when processing
> vring based messages, such as SET_VRING_KICK and SET_VRING_CALL,
> and the data processing thread may already be started, e.g: SPDK
> vhost-blk and vhost-scsi will start the data processing thread
> when one vring is ready, then deadlock may happen when SPDK is
> posting interrupts to VM.  Here, we add a new API which allows
> caller to try again later for this case.
> 
> Bugzilla ID: 1015
> Fixes: c5736998305d ("vhost: fix missing virtqueue lock protection")
> 
> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> ---
>  lib/vhost/rte_vhost.h | 15 +++++++++++++++
>  lib/vhost/version.map |  3 +++
>  lib/vhost/vhost.c     | 30 ++++++++++++++++++++++++++++++
>  3 files changed, 48 insertions(+)

For new API, we need to update release_22_11.rst and vhost_lib.rst.

You can refer to http://patchwork.dpdk.org/project/dpdk/patch/20221013092708.4922-2-xuan.ding@intel.com/

Thanks,
Chenbo

> 
> diff --git a/lib/vhost/rte_vhost.h b/lib/vhost/rte_vhost.h
> index bb7d86a432..d22b25cd4e 100644
> --- a/lib/vhost/rte_vhost.h
> +++ b/lib/vhost/rte_vhost.h
> @@ -909,6 +909,21 @@ rte_vhost_clr_inflight_desc_packed(int vid, uint16_t
> vring_idx,
>   */
>  int rte_vhost_vring_call(int vid, uint16_t vring_idx);
> 
> +/**
> + * Notify the guest that used descriptors have been added to the vring.
> This
> + * function acts as a memory barrier.  This function will return -EAGAIN
> when
> + * vq's access lock is held by other thread, user should try again later.
> + *
> + * @param vid
> + *  vhost device ID
> + * @param vring_idx
> + *  vring index
> + * @return
> + *  0 on success, -1 on failure, -EAGAIN for another retry
> + */
> +__rte_experimental
> +int rte_vhost_vring_call_nonblock(int vid, uint16_t vring_idx);
> +
>  /**
>   * Get vhost RX queue avail count.
>   *
> diff --git a/lib/vhost/version.map b/lib/vhost/version.map
> index 7a00b65740..c8c44b8326 100644
> --- a/lib/vhost/version.map
> +++ b/lib/vhost/version.map
> @@ -94,6 +94,9 @@ EXPERIMENTAL {
>  	rte_vhost_async_try_dequeue_burst;
>  	rte_vhost_driver_get_vdpa_dev_type;
>  	rte_vhost_clear_queue;
> +
> +	# added in 22.11
> +	rte_vhost_vring_call_nonblock;
>  };
> 
>  INTERNAL {
> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> index 8740aa2788..ed6efb003f 100644
> --- a/lib/vhost/vhost.c
> +++ b/lib/vhost/vhost.c
> @@ -1317,6 +1317,36 @@ rte_vhost_vring_call(int vid, uint16_t vring_idx)
>  	return 0;
>  }
> 
> +int
> +rte_vhost_vring_call_nonblock(int vid, uint16_t vring_idx)
> +{
> +	struct virtio_net *dev;
> +	struct vhost_virtqueue *vq;
> +
> +	dev = get_device(vid);
> +	if (!dev)
> +		return -1;
> +
> +	if (vring_idx >= VHOST_MAX_VRING)
> +		return -1;
> +
> +	vq = dev->virtqueue[vring_idx];
> +	if (!vq)
> +		return -1;
> +
> +	if (!rte_spinlock_trylock(&vq->access_lock))
> +		return -EAGAIN;
> +
> +	if (vq_is_packed(dev))
> +		vhost_vring_call_packed(dev, vq);
> +	else
> +		vhost_vring_call_split(dev, vq);
> +
> +	rte_spinlock_unlock(&vq->access_lock);
> +
> +	return 0;
> +}
> +
>  uint16_t
>  rte_vhost_avail_entries(int vid, uint16_t queue_id)
>  {
> --
> 2.21.3

RE: [PATCH v2] vhost: add new `rte_vhost_vring_call_nonblock` API

[Liu, Changpeng]

> -----Original Message-----
> From: Xia, Chenbo <chenbo.xia@intel.com>
> Sent: Monday, October 17, 2022 2:47 PM
> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; David Marchand
> <david.marchand@redhat.com>
> Subject: RE: [PATCH v2] vhost: add new `rte_vhost_vring_call_nonblock` API
> 
> Hi Changpeng,
> 
> > -----Original Message-----
> > From: Liu, Changpeng <changpeng.liu@intel.com>
> > Sent: Wednesday, October 12, 2022 2:40 PM
> > To: dev@dpdk.org
> > Cc: Liu, Changpeng <changpeng.liu@intel.com>; Maxime Coquelin
> > <maxime.coquelin@redhat.com>; Xia, Chenbo <chenbo.xia@intel.com>; David
> > Marchand <david.marchand@redhat.com>
> > Subject: [PATCH v2] vhost: add new `rte_vhost_vring_call_nonblock` API
> >
> > Vhost-user library locks all VQ's access lock when processing
> > vring based messages, such as SET_VRING_KICK and SET_VRING_CALL,
> > and the data processing thread may already be started, e.g: SPDK
> > vhost-blk and vhost-scsi will start the data processing thread
> > when one vring is ready, then deadlock may happen when SPDK is
> > posting interrupts to VM.  Here, we add a new API which allows
> > caller to try again later for this case.
> >
> > Bugzilla ID: 1015
> > Fixes: c5736998305d ("vhost: fix missing virtqueue lock protection")
> >
> > Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> > ---
> >  lib/vhost/rte_vhost.h | 15 +++++++++++++++
> >  lib/vhost/version.map |  3 +++
> >  lib/vhost/vhost.c     | 30 ++++++++++++++++++++++++++++++
> >  3 files changed, 48 insertions(+)
> 
> For new API, we need to update release_22_11.rst and vhost_lib.rst.
Thanks Chenbo, a new v3 is sent for review.
> 
> You can refer to
> http://patchwork.dpdk.org/project/dpdk/patch/20221013092708.4922-2-
> xuan.ding@intel.com/
> 
> Thanks,
> Chenbo
> 
> >
> > diff --git a/lib/vhost/rte_vhost.h b/lib/vhost/rte_vhost.h
> > index bb7d86a432..d22b25cd4e 100644
> > --- a/lib/vhost/rte_vhost.h
> > +++ b/lib/vhost/rte_vhost.h
> > @@ -909,6 +909,21 @@ rte_vhost_clr_inflight_desc_packed(int vid, uint16_t
> > vring_idx,
> >   */
> >  int rte_vhost_vring_call(int vid, uint16_t vring_idx);
> >
> > +/**
> > + * Notify the guest that used descriptors have been added to the vring.
> > This
> > + * function acts as a memory barrier.  This function will return -EAGAIN
> > when
> > + * vq's access lock is held by other thread, user should try again later.
> > + *
> > + * @param vid
> > + *  vhost device ID
> > + * @param vring_idx
> > + *  vring index
> > + * @return
> > + *  0 on success, -1 on failure, -EAGAIN for another retry
> > + */
> > +__rte_experimental
> > +int rte_vhost_vring_call_nonblock(int vid, uint16_t vring_idx);
> > +
> >  /**
> >   * Get vhost RX queue avail count.
> >   *
> > diff --git a/lib/vhost/version.map b/lib/vhost/version.map
> > index 7a00b65740..c8c44b8326 100644
> > --- a/lib/vhost/version.map
> > +++ b/lib/vhost/version.map
> > @@ -94,6 +94,9 @@ EXPERIMENTAL {
> >  	rte_vhost_async_try_dequeue_burst;
> >  	rte_vhost_driver_get_vdpa_dev_type;
> >  	rte_vhost_clear_queue;
> > +
> > +	# added in 22.11
> > +	rte_vhost_vring_call_nonblock;
> >  };
> >
> >  INTERNAL {
> > diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> > index 8740aa2788..ed6efb003f 100644
> > --- a/lib/vhost/vhost.c
> > +++ b/lib/vhost/vhost.c
> > @@ -1317,6 +1317,36 @@ rte_vhost_vring_call(int vid, uint16_t vring_idx)
> >  	return 0;
> >  }
> >
> > +int
> > +rte_vhost_vring_call_nonblock(int vid, uint16_t vring_idx)
> > +{
> > +	struct virtio_net *dev;
> > +	struct vhost_virtqueue *vq;
> > +
> > +	dev = get_device(vid);
> > +	if (!dev)
> > +		return -1;
> > +
> > +	if (vring_idx >= VHOST_MAX_VRING)
> > +		return -1;
> > +
> > +	vq = dev->virtqueue[vring_idx];
> > +	if (!vq)
> > +		return -1;
> > +
> > +	if (!rte_spinlock_trylock(&vq->access_lock))
> > +		return -EAGAIN;
> > +
> > +	if (vq_is_packed(dev))
> > +		vhost_vring_call_packed(dev, vq);
> > +	else
> > +		vhost_vring_call_split(dev, vq);
> > +
> > +	rte_spinlock_unlock(&vq->access_lock);
> > +
> > +	return 0;
> > +}
> > +
> >  uint16_t
> >  rte_vhost_avail_entries(int vid, uint16_t queue_id)
> >  {
> > --
> > 2.21.3

[PATCH v3] vhost: add new `rte_vhost_vring_call_nonblock` API

[Changpeng Liu]

Vhost-user library locks all VQ's access lock when processing
vring based messages, such as SET_VRING_KICK and SET_VRING_CALL,
and the data processing thread may already be started, e.g: SPDK
vhost-blk and vhost-scsi will start the data processing thread
when one vring is ready, then deadlock may happen when SPDK is
posting interrupts to VM.  Here, we add a new API which allows
caller to try again later for this case.

Bugzilla ID: 1015
Fixes: c5736998305d ("vhost: fix missing virtqueue lock protection")

Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
---
 doc/guides/prog_guide/vhost_lib.rst    |  6 ++++++
 doc/guides/rel_notes/release_22_11.rst |  6 ++++++
 lib/vhost/rte_vhost.h                  | 15 +++++++++++++
 lib/vhost/version.map                  |  3 +++
 lib/vhost/vhost.c                      | 30 ++++++++++++++++++++++++++
 5 files changed, 60 insertions(+)

diff --git a/doc/guides/prog_guide/vhost_lib.rst b/doc/guides/prog_guide/vhost_lib.rst
index bad4d819e1..52f0589f51 100644
--- a/doc/guides/prog_guide/vhost_lib.rst
+++ b/doc/guides/prog_guide/vhost_lib.rst
@@ -297,6 +297,12 @@ The following is an overview of some key Vhost API functions:
   Clear in-flight packets which are submitted to async channel in vhost async data
   path. Completed packets are returned to applications through ``pkts``.
 
+* ``rte_vhost_vring_call_nonblock(int vid, uint16_t vring_idx)``
+
+  Notify the guest that used descriptors have been added to the vring. This function
+  will return -EAGAIN when vq's access lock is held by other thread, user should try
+  again later.
+
 * ``rte_vhost_vring_stats_get_names(int vid, uint16_t queue_id, struct rte_vhost_stat_name *names, unsigned int size)``
 
   This function returns the names of the queue statistics. It requires
diff --git a/doc/guides/rel_notes/release_22_11.rst b/doc/guides/rel_notes/release_22_11.rst
index 2da8bc9661..9b3783ffd8 100644
--- a/doc/guides/rel_notes/release_22_11.rst
+++ b/doc/guides/rel_notes/release_22_11.rst
@@ -236,6 +236,12 @@ New Features
 
      strings $dpdk_binary_or_driver | sed -n 's/^PMD_INFO_STRING= //p'
 
+* **Added non-block notify API to the vhost library.**
+
+  Added API to notify the guest that used descriptors have been added
+  to the vring in non-block way, user should check the return value of
+  this API.
+
 
 Removed Items
 -------------
diff --git a/lib/vhost/rte_vhost.h b/lib/vhost/rte_vhost.h
index bb7d86a432..d22b25cd4e 100644
--- a/lib/vhost/rte_vhost.h
+++ b/lib/vhost/rte_vhost.h
@@ -909,6 +909,21 @@ rte_vhost_clr_inflight_desc_packed(int vid, uint16_t vring_idx,
  */
 int rte_vhost_vring_call(int vid, uint16_t vring_idx);
 
+/**
+ * Notify the guest that used descriptors have been added to the vring.  This
+ * function acts as a memory barrier.  This function will return -EAGAIN when
+ * vq's access lock is held by other thread, user should try again later.
+ *
+ * @param vid
+ *  vhost device ID
+ * @param vring_idx
+ *  vring index
+ * @return
+ *  0 on success, -1 on failure, -EAGAIN for another retry
+ */
+__rte_experimental
+int rte_vhost_vring_call_nonblock(int vid, uint16_t vring_idx);
+
 /**
  * Get vhost RX queue avail count.
  *
diff --git a/lib/vhost/version.map b/lib/vhost/version.map
index 7a00b65740..c8c44b8326 100644
--- a/lib/vhost/version.map
+++ b/lib/vhost/version.map
@@ -94,6 +94,9 @@ EXPERIMENTAL {
 	rte_vhost_async_try_dequeue_burst;
 	rte_vhost_driver_get_vdpa_dev_type;
 	rte_vhost_clear_queue;
+
+	# added in 22.11
+	rte_vhost_vring_call_nonblock;
 };
 
 INTERNAL {
diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
index 8740aa2788..ed6efb003f 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -1317,6 +1317,36 @@ rte_vhost_vring_call(int vid, uint16_t vring_idx)
 	return 0;
 }
 
+int
+rte_vhost_vring_call_nonblock(int vid, uint16_t vring_idx)
+{
+	struct virtio_net *dev;
+	struct vhost_virtqueue *vq;
+
+	dev = get_device(vid);
+	if (!dev)
+		return -1;
+
+	if (vring_idx >= VHOST_MAX_VRING)
+		return -1;
+
+	vq = dev->virtqueue[vring_idx];
+	if (!vq)
+		return -1;
+
+	if (!rte_spinlock_trylock(&vq->access_lock))
+		return -EAGAIN;
+
+	if (vq_is_packed(dev))
+		vhost_vring_call_packed(dev, vq);
+	else
+		vhost_vring_call_split(dev, vq);
+
+	rte_spinlock_unlock(&vq->access_lock);
+
+	return 0;
+}
+
 uint16_t
 rte_vhost_avail_entries(int vid, uint16_t queue_id)
 {
-- 
2.21.3

RE: [PATCH v3] vhost: add new `rte_vhost_vring_call_nonblock` API

[Xia, Chenbo]

Hi Changpeng,

> -----Original Message-----
> From: Liu, Changpeng <changpeng.liu@intel.com>
> Sent: Monday, October 17, 2022 3:15 PM
> To: dev@dpdk.org
> Cc: Liu, Changpeng <changpeng.liu@intel.com>; Maxime Coquelin
> <maxime.coquelin@redhat.com>; Xia, Chenbo <chenbo.xia@intel.com>; David
> Marchand <david.marchand@redhat.com>
> Subject: [PATCH v3] vhost: add new `rte_vhost_vring_call_nonblock` API
> 
> Vhost-user library locks all VQ's access lock when processing
> vring based messages, such as SET_VRING_KICK and SET_VRING_CALL,
> and the data processing thread may already be started, e.g: SPDK
> vhost-blk and vhost-scsi will start the data processing thread
> when one vring is ready, then deadlock may happen when SPDK is
> posting interrupts to VM.  Here, we add a new API which allows
> caller to try again later for this case.
> 
> Bugzilla ID: 1015
> Fixes: c5736998305d ("vhost: fix missing virtqueue lock protection")
> 
> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> ---
>  doc/guides/prog_guide/vhost_lib.rst    |  6 ++++++
>  doc/guides/rel_notes/release_22_11.rst |  6 ++++++
>  lib/vhost/rte_vhost.h                  | 15 +++++++++++++
>  lib/vhost/version.map                  |  3 +++
>  lib/vhost/vhost.c                      | 30 ++++++++++++++++++++++++++
>  5 files changed, 60 insertions(+)
> 
> diff --git a/doc/guides/prog_guide/vhost_lib.rst
> b/doc/guides/prog_guide/vhost_lib.rst
> index bad4d819e1..52f0589f51 100644
> --- a/doc/guides/prog_guide/vhost_lib.rst
> +++ b/doc/guides/prog_guide/vhost_lib.rst
> @@ -297,6 +297,12 @@ The following is an overview of some key Vhost API
> functions:
>    Clear in-flight packets which are submitted to async channel in vhost
> async data
>    path. Completed packets are returned to applications through ``pkts``.
> 
> +* ``rte_vhost_vring_call_nonblock(int vid, uint16_t vring_idx)``
> +
> +  Notify the guest that used descriptors have been added to the vring.
> This function
> +  will return -EAGAIN when vq's access lock is held by other thread, user
> should try
> +  again later.
> +
>  * ``rte_vhost_vring_stats_get_names(int vid, uint16_t queue_id, struct
> rte_vhost_stat_name *names, unsigned int size)``
> 
>    This function returns the names of the queue statistics. It requires
> diff --git a/doc/guides/rel_notes/release_22_11.rst
> b/doc/guides/rel_notes/release_22_11.rst
> index 2da8bc9661..9b3783ffd8 100644
> --- a/doc/guides/rel_notes/release_22_11.rst
> +++ b/doc/guides/rel_notes/release_22_11.rst
> @@ -236,6 +236,12 @@ New Features
> 
>       strings $dpdk_binary_or_driver | sed -n 's/^PMD_INFO_STRING= //p'
> 
> +* **Added non-block notify API to the vhost library.**
> +
> +  Added API to notify the guest that used descriptors have been added
> +  to the vring in non-block way, user should check the return value of
> +  this API.
> +

This may be better:

* **Added non-blocking notify API to the vhost library.**

  Added ``rte_vhost_vring_call_nonblock`` API to notify the guest that used
  descriptors have been added to the vring in non-blocking way. User should
  check the return value of this API and try again if needed.

And for new features, library should come before scripts, so you need to move
this before ' Rewritten pmdinfo script. ' item.

Thanks,
Chenbo

> 
>  Removed Items
>  -------------
> diff --git a/lib/vhost/rte_vhost.h b/lib/vhost/rte_vhost.h
> index bb7d86a432..d22b25cd4e 100644
> --- a/lib/vhost/rte_vhost.h
> +++ b/lib/vhost/rte_vhost.h
> @@ -909,6 +909,21 @@ rte_vhost_clr_inflight_desc_packed(int vid, uint16_t
> vring_idx,
>   */
>  int rte_vhost_vring_call(int vid, uint16_t vring_idx);
> 
> +/**
> + * Notify the guest that used descriptors have been added to the vring.
> This
> + * function acts as a memory barrier.  This function will return -EAGAIN
> when
> + * vq's access lock is held by other thread, user should try again later.
> + *
> + * @param vid
> + *  vhost device ID
> + * @param vring_idx
> + *  vring index
> + * @return
> + *  0 on success, -1 on failure, -EAGAIN for another retry
> + */
> +__rte_experimental
> +int rte_vhost_vring_call_nonblock(int vid, uint16_t vring_idx);
> +
>  /**
>   * Get vhost RX queue avail count.
>   *
> diff --git a/lib/vhost/version.map b/lib/vhost/version.map
> index 7a00b65740..c8c44b8326 100644
> --- a/lib/vhost/version.map
> +++ b/lib/vhost/version.map
> @@ -94,6 +94,9 @@ EXPERIMENTAL {
>  	rte_vhost_async_try_dequeue_burst;
>  	rte_vhost_driver_get_vdpa_dev_type;
>  	rte_vhost_clear_queue;
> +
> +	# added in 22.11
> +	rte_vhost_vring_call_nonblock;
>  };
> 
>  INTERNAL {
> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> index 8740aa2788..ed6efb003f 100644
> --- a/lib/vhost/vhost.c
> +++ b/lib/vhost/vhost.c
> @@ -1317,6 +1317,36 @@ rte_vhost_vring_call(int vid, uint16_t vring_idx)
>  	return 0;
>  }
> 
> +int
> +rte_vhost_vring_call_nonblock(int vid, uint16_t vring_idx)
> +{
> +	struct virtio_net *dev;
> +	struct vhost_virtqueue *vq;
> +
> +	dev = get_device(vid);
> +	if (!dev)
> +		return -1;
> +
> +	if (vring_idx >= VHOST_MAX_VRING)
> +		return -1;
> +
> +	vq = dev->virtqueue[vring_idx];
> +	if (!vq)
> +		return -1;
> +
> +	if (!rte_spinlock_trylock(&vq->access_lock))
> +		return -EAGAIN;
> +
> +	if (vq_is_packed(dev))
> +		vhost_vring_call_packed(dev, vq);
> +	else
> +		vhost_vring_call_split(dev, vq);
> +
> +	rte_spinlock_unlock(&vq->access_lock);
> +
> +	return 0;
> +}
> +
>  uint16_t
>  rte_vhost_avail_entries(int vid, uint16_t queue_id)
>  {
> --
> 2.21.3

RE: [PATCH v3] vhost: add new `rte_vhost_vring_call_nonblock` API

[Liu, Changpeng]

> -----Original Message-----
> From: Xia, Chenbo <chenbo.xia@intel.com>
> Sent: Monday, October 17, 2022 3:40 PM
> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; David Marchand
> <david.marchand@redhat.com>
> Subject: RE: [PATCH v3] vhost: add new `rte_vhost_vring_call_nonblock` API
> 
> Hi Changpeng,
> 
> > -----Original Message-----
> > From: Liu, Changpeng <changpeng.liu@intel.com>
> > Sent: Monday, October 17, 2022 3:15 PM
> > To: dev@dpdk.org
> > Cc: Liu, Changpeng <changpeng.liu@intel.com>; Maxime Coquelin
> > <maxime.coquelin@redhat.com>; Xia, Chenbo <chenbo.xia@intel.com>; David
> > Marchand <david.marchand@redhat.com>
> > Subject: [PATCH v3] vhost: add new `rte_vhost_vring_call_nonblock` API
> >
> > Vhost-user library locks all VQ's access lock when processing
> > vring based messages, such as SET_VRING_KICK and SET_VRING_CALL,
> > and the data processing thread may already be started, e.g: SPDK
> > vhost-blk and vhost-scsi will start the data processing thread
> > when one vring is ready, then deadlock may happen when SPDK is
> > posting interrupts to VM.  Here, we add a new API which allows
> > caller to try again later for this case.
> >
> > Bugzilla ID: 1015
> > Fixes: c5736998305d ("vhost: fix missing virtqueue lock protection")
> >
> > Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> > ---
> >  doc/guides/prog_guide/vhost_lib.rst    |  6 ++++++
> >  doc/guides/rel_notes/release_22_11.rst |  6 ++++++
> >  lib/vhost/rte_vhost.h                  | 15 +++++++++++++
> >  lib/vhost/version.map                  |  3 +++
> >  lib/vhost/vhost.c                      | 30 ++++++++++++++++++++++++++
> >  5 files changed, 60 insertions(+)
> >
> > diff --git a/doc/guides/prog_guide/vhost_lib.rst
> > b/doc/guides/prog_guide/vhost_lib.rst
> > index bad4d819e1..52f0589f51 100644
> > --- a/doc/guides/prog_guide/vhost_lib.rst
> > +++ b/doc/guides/prog_guide/vhost_lib.rst
> > @@ -297,6 +297,12 @@ The following is an overview of some key Vhost API
> > functions:
> >    Clear in-flight packets which are submitted to async channel in vhost
> > async data
> >    path. Completed packets are returned to applications through ``pkts``.
> >
> > +* ``rte_vhost_vring_call_nonblock(int vid, uint16_t vring_idx)``
> > +
> > +  Notify the guest that used descriptors have been added to the vring.
> > This function
> > +  will return -EAGAIN when vq's access lock is held by other thread, user
> > should try
> > +  again later.
> > +
> >  * ``rte_vhost_vring_stats_get_names(int vid, uint16_t queue_id, struct
> > rte_vhost_stat_name *names, unsigned int size)``
> >
> >    This function returns the names of the queue statistics. It requires
> > diff --git a/doc/guides/rel_notes/release_22_11.rst
> > b/doc/guides/rel_notes/release_22_11.rst
> > index 2da8bc9661..9b3783ffd8 100644
> > --- a/doc/guides/rel_notes/release_22_11.rst
> > +++ b/doc/guides/rel_notes/release_22_11.rst
> > @@ -236,6 +236,12 @@ New Features
> >
> >       strings $dpdk_binary_or_driver | sed -n 's/^PMD_INFO_STRING= //p'
> >
> > +* **Added non-block notify API to the vhost library.**
> > +
> > +  Added API to notify the guest that used descriptors have been added
> > +  to the vring in non-block way, user should check the return value of
> > +  this API.
> > +
> 
> This may be better:
> 
> * **Added non-blocking notify API to the vhost library.**
> 
>   Added ``rte_vhost_vring_call_nonblock`` API to notify the guest that used
>   descriptors have been added to the vring in non-blocking way. User should
>   check the return value of this API and try again if needed.
> 
> And for new features, library should come before scripts, so you need to move
> this before ' Rewritten pmdinfo script. ' item.
Thanks Chenbo, applied them with v4.
> 
> Thanks,
> Chenbo
> 
> >
> >  Removed Items
> >  -------------
> > diff --git a/lib/vhost/rte_vhost.h b/lib/vhost/rte_vhost.h
> > index bb7d86a432..d22b25cd4e 100644
> > --- a/lib/vhost/rte_vhost.h
> > +++ b/lib/vhost/rte_vhost.h
> > @@ -909,6 +909,21 @@ rte_vhost_clr_inflight_desc_packed(int vid, uint16_t
> > vring_idx,
> >   */
> >  int rte_vhost_vring_call(int vid, uint16_t vring_idx);
> >
> > +/**
> > + * Notify the guest that used descriptors have been added to the vring.
> > This
> > + * function acts as a memory barrier.  This function will return -EAGAIN
> > when
> > + * vq's access lock is held by other thread, user should try again later.
> > + *
> > + * @param vid
> > + *  vhost device ID
> > + * @param vring_idx
> > + *  vring index
> > + * @return
> > + *  0 on success, -1 on failure, -EAGAIN for another retry
> > + */
> > +__rte_experimental
> > +int rte_vhost_vring_call_nonblock(int vid, uint16_t vring_idx);
> > +
> >  /**
> >   * Get vhost RX queue avail count.
> >   *
> > diff --git a/lib/vhost/version.map b/lib/vhost/version.map
> > index 7a00b65740..c8c44b8326 100644
> > --- a/lib/vhost/version.map
> > +++ b/lib/vhost/version.map
> > @@ -94,6 +94,9 @@ EXPERIMENTAL {
> >  	rte_vhost_async_try_dequeue_burst;
> >  	rte_vhost_driver_get_vdpa_dev_type;
> >  	rte_vhost_clear_queue;
> > +
> > +	# added in 22.11
> > +	rte_vhost_vring_call_nonblock;
> >  };
> >
> >  INTERNAL {
> > diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> > index 8740aa2788..ed6efb003f 100644
> > --- a/lib/vhost/vhost.c
> > +++ b/lib/vhost/vhost.c
> > @@ -1317,6 +1317,36 @@ rte_vhost_vring_call(int vid, uint16_t vring_idx)
> >  	return 0;
> >  }
> >
> > +int
> > +rte_vhost_vring_call_nonblock(int vid, uint16_t vring_idx)
> > +{
> > +	struct virtio_net *dev;
> > +	struct vhost_virtqueue *vq;
> > +
> > +	dev = get_device(vid);
> > +	if (!dev)
> > +		return -1;
> > +
> > +	if (vring_idx >= VHOST_MAX_VRING)
> > +		return -1;
> > +
> > +	vq = dev->virtqueue[vring_idx];
> > +	if (!vq)
> > +		return -1;
> > +
> > +	if (!rte_spinlock_trylock(&vq->access_lock))
> > +		return -EAGAIN;
> > +
> > +	if (vq_is_packed(dev))
> > +		vhost_vring_call_packed(dev, vq);
> > +	else
> > +		vhost_vring_call_split(dev, vq);
> > +
> > +	rte_spinlock_unlock(&vq->access_lock);
> > +
> > +	return 0;
> > +}
> > +
> >  uint16_t
> >  rte_vhost_avail_entries(int vid, uint16_t queue_id)
> >  {
> > --
> > 2.21.3

[PATCH v4] vhost: add new `rte_vhost_vring_call_nonblock` API

[Changpeng Liu]

Vhost-user library locks all VQ's access lock when processing
vring based messages, such as SET_VRING_KICK and SET_VRING_CALL,
and the data processing thread may already be started, e.g: SPDK
vhost-blk and vhost-scsi will start the data processing thread
when one vring is ready, then deadlock may happen when SPDK is
posting interrupts to VM.  Here, we add a new API which allows
caller to try again later for this case.

Bugzilla ID: 1015
Fixes: c5736998305d ("vhost: fix missing virtqueue lock protection")

Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
---
 doc/guides/prog_guide/vhost_lib.rst    |  6 ++++++
 doc/guides/rel_notes/release_22_11.rst |  6 ++++++
 lib/vhost/rte_vhost.h                  | 15 +++++++++++++
 lib/vhost/version.map                  |  3 +++
 lib/vhost/vhost.c                      | 30 ++++++++++++++++++++++++++
 5 files changed, 60 insertions(+)

diff --git a/doc/guides/prog_guide/vhost_lib.rst b/doc/guides/prog_guide/vhost_lib.rst
index bad4d819e1..52f0589f51 100644
--- a/doc/guides/prog_guide/vhost_lib.rst
+++ b/doc/guides/prog_guide/vhost_lib.rst
@@ -297,6 +297,12 @@ The following is an overview of some key Vhost API functions:
   Clear in-flight packets which are submitted to async channel in vhost async data
   path. Completed packets are returned to applications through ``pkts``.
 
+* ``rte_vhost_vring_call_nonblock(int vid, uint16_t vring_idx)``
+
+  Notify the guest that used descriptors have been added to the vring. This function
+  will return -EAGAIN when vq's access lock is held by other thread, user should try
+  again later.
+
 * ``rte_vhost_vring_stats_get_names(int vid, uint16_t queue_id, struct rte_vhost_stat_name *names, unsigned int size)``
 
   This function returns the names of the queue statistics. It requires
diff --git a/doc/guides/rel_notes/release_22_11.rst b/doc/guides/rel_notes/release_22_11.rst
index 2da8bc9661..b3d1ba043c 100644
--- a/doc/guides/rel_notes/release_22_11.rst
+++ b/doc/guides/rel_notes/release_22_11.rst
@@ -225,6 +225,12 @@ New Features
   sysfs entries to adjust the minimum and maximum uncore frequency values,
   which works on Linux with Intel hardware only.
 
+* **Added non-blocking notify API to the vhost library.**
+
+  Added ``rte_vhost_vring_call_nonblock`` API to notify the guest that
+  used descriptors have been added to the vring in non-blocking way. User
+  should check the return value of this API and try again if needed.
+
 * **Rewritten pmdinfo script.**
 
   The ``dpdk-pmdinfo.py`` script was rewritten to produce valid JSON only.
diff --git a/lib/vhost/rte_vhost.h b/lib/vhost/rte_vhost.h
index bb7d86a432..d22b25cd4e 100644
--- a/lib/vhost/rte_vhost.h
+++ b/lib/vhost/rte_vhost.h
@@ -909,6 +909,21 @@ rte_vhost_clr_inflight_desc_packed(int vid, uint16_t vring_idx,
  */
 int rte_vhost_vring_call(int vid, uint16_t vring_idx);
 
+/**
+ * Notify the guest that used descriptors have been added to the vring.  This
+ * function acts as a memory barrier.  This function will return -EAGAIN when
+ * vq's access lock is held by other thread, user should try again later.
+ *
+ * @param vid
+ *  vhost device ID
+ * @param vring_idx
+ *  vring index
+ * @return
+ *  0 on success, -1 on failure, -EAGAIN for another retry
+ */
+__rte_experimental
+int rte_vhost_vring_call_nonblock(int vid, uint16_t vring_idx);
+
 /**
  * Get vhost RX queue avail count.
  *
diff --git a/lib/vhost/version.map b/lib/vhost/version.map
index 7a00b65740..c8c44b8326 100644
--- a/lib/vhost/version.map
+++ b/lib/vhost/version.map
@@ -94,6 +94,9 @@ EXPERIMENTAL {
 	rte_vhost_async_try_dequeue_burst;
 	rte_vhost_driver_get_vdpa_dev_type;
 	rte_vhost_clear_queue;
+
+	# added in 22.11
+	rte_vhost_vring_call_nonblock;
 };
 
 INTERNAL {
diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
index 8740aa2788..ed6efb003f 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -1317,6 +1317,36 @@ rte_vhost_vring_call(int vid, uint16_t vring_idx)
 	return 0;
 }
 
+int
+rte_vhost_vring_call_nonblock(int vid, uint16_t vring_idx)
+{
+	struct virtio_net *dev;
+	struct vhost_virtqueue *vq;
+
+	dev = get_device(vid);
+	if (!dev)
+		return -1;
+
+	if (vring_idx >= VHOST_MAX_VRING)
+		return -1;
+
+	vq = dev->virtqueue[vring_idx];
+	if (!vq)
+		return -1;
+
+	if (!rte_spinlock_trylock(&vq->access_lock))
+		return -EAGAIN;
+
+	if (vq_is_packed(dev))
+		vhost_vring_call_packed(dev, vq);
+	else
+		vhost_vring_call_split(dev, vq);
+
+	rte_spinlock_unlock(&vq->access_lock);
+
+	return 0;
+}
+
 uint16_t
 rte_vhost_avail_entries(int vid, uint16_t queue_id)
 {
-- 
2.21.3

RE: [PATCH v4] vhost: add new `rte_vhost_vring_call_nonblock` API

[Xia, Chenbo]

> -----Original Message-----
> From: Liu, Changpeng <changpeng.liu@intel.com>
> Sent: Monday, October 17, 2022 3:48 PM
> To: dev@dpdk.org
> Cc: Liu, Changpeng <changpeng.liu@intel.com>; Maxime Coquelin
> <maxime.coquelin@redhat.com>; Xia, Chenbo <chenbo.xia@intel.com>; David
> Marchand <david.marchand@redhat.com>
> Subject: [PATCH v4] vhost: add new `rte_vhost_vring_call_nonblock` API
> 
> Vhost-user library locks all VQ's access lock when processing
> vring based messages, such as SET_VRING_KICK and SET_VRING_CALL,
> and the data processing thread may already be started, e.g: SPDK
> vhost-blk and vhost-scsi will start the data processing thread
> when one vring is ready, then deadlock may happen when SPDK is
> posting interrupts to VM.  Here, we add a new API which allows
> caller to try again later for this case.
> 
> Bugzilla ID: 1015
> Fixes: c5736998305d ("vhost: fix missing virtqueue lock protection")
> 
> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> ---
>  doc/guides/prog_guide/vhost_lib.rst    |  6 ++++++
>  doc/guides/rel_notes/release_22_11.rst |  6 ++++++
>  lib/vhost/rte_vhost.h                  | 15 +++++++++++++
>  lib/vhost/version.map                  |  3 +++
>  lib/vhost/vhost.c                      | 30 ++++++++++++++++++++++++++
>  5 files changed, 60 insertions(+)
> 
> diff --git a/doc/guides/prog_guide/vhost_lib.rst
> b/doc/guides/prog_guide/vhost_lib.rst
> index bad4d819e1..52f0589f51 100644
> --- a/doc/guides/prog_guide/vhost_lib.rst
> +++ b/doc/guides/prog_guide/vhost_lib.rst
> @@ -297,6 +297,12 @@ The following is an overview of some key Vhost API
> functions:
>    Clear in-flight packets which are submitted to async channel in vhost
> async data
>    path. Completed packets are returned to applications through ``pkts``.
> 
> +* ``rte_vhost_vring_call_nonblock(int vid, uint16_t vring_idx)``
> +
> +  Notify the guest that used descriptors have been added to the vring.
> This function
> +  will return -EAGAIN when vq's access lock is held by other thread, user
> should try
> +  again later.
> +
>  * ``rte_vhost_vring_stats_get_names(int vid, uint16_t queue_id, struct
> rte_vhost_stat_name *names, unsigned int size)``
> 
>    This function returns the names of the queue statistics. It requires
> diff --git a/doc/guides/rel_notes/release_22_11.rst
> b/doc/guides/rel_notes/release_22_11.rst
> index 2da8bc9661..b3d1ba043c 100644
> --- a/doc/guides/rel_notes/release_22_11.rst
> +++ b/doc/guides/rel_notes/release_22_11.rst
> @@ -225,6 +225,12 @@ New Features
>    sysfs entries to adjust the minimum and maximum uncore frequency values,
>    which works on Linux with Intel hardware only.
> 
> +* **Added non-blocking notify API to the vhost library.**
> +
> +  Added ``rte_vhost_vring_call_nonblock`` API to notify the guest that
> +  used descriptors have been added to the vring in non-blocking way. User
> +  should check the return value of this API and try again if needed.
> +
>  * **Rewritten pmdinfo script.**
> 
>    The ``dpdk-pmdinfo.py`` script was rewritten to produce valid JSON only.
> diff --git a/lib/vhost/rte_vhost.h b/lib/vhost/rte_vhost.h
> index bb7d86a432..d22b25cd4e 100644
> --- a/lib/vhost/rte_vhost.h
> +++ b/lib/vhost/rte_vhost.h
> @@ -909,6 +909,21 @@ rte_vhost_clr_inflight_desc_packed(int vid, uint16_t
> vring_idx,
>   */
>  int rte_vhost_vring_call(int vid, uint16_t vring_idx);
> 
> +/**
> + * Notify the guest that used descriptors have been added to the vring.
> This
> + * function acts as a memory barrier.  This function will return -EAGAIN
> when
> + * vq's access lock is held by other thread, user should try again later.
> + *
> + * @param vid
> + *  vhost device ID
> + * @param vring_idx
> + *  vring index
> + * @return
> + *  0 on success, -1 on failure, -EAGAIN for another retry
> + */
> +__rte_experimental
> +int rte_vhost_vring_call_nonblock(int vid, uint16_t vring_idx);
> +
>  /**
>   * Get vhost RX queue avail count.
>   *
> diff --git a/lib/vhost/version.map b/lib/vhost/version.map
> index 7a00b65740..c8c44b8326 100644
> --- a/lib/vhost/version.map
> +++ b/lib/vhost/version.map
> @@ -94,6 +94,9 @@ EXPERIMENTAL {
>  	rte_vhost_async_try_dequeue_burst;
>  	rte_vhost_driver_get_vdpa_dev_type;
>  	rte_vhost_clear_queue;
> +
> +	# added in 22.11
> +	rte_vhost_vring_call_nonblock;
>  };
> 
>  INTERNAL {
> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> index 8740aa2788..ed6efb003f 100644
> --- a/lib/vhost/vhost.c
> +++ b/lib/vhost/vhost.c
> @@ -1317,6 +1317,36 @@ rte_vhost_vring_call(int vid, uint16_t vring_idx)
>  	return 0;
>  }
> 
> +int
> +rte_vhost_vring_call_nonblock(int vid, uint16_t vring_idx)
> +{
> +	struct virtio_net *dev;
> +	struct vhost_virtqueue *vq;
> +
> +	dev = get_device(vid);
> +	if (!dev)
> +		return -1;
> +
> +	if (vring_idx >= VHOST_MAX_VRING)
> +		return -1;
> +
> +	vq = dev->virtqueue[vring_idx];
> +	if (!vq)
> +		return -1;
> +
> +	if (!rte_spinlock_trylock(&vq->access_lock))
> +		return -EAGAIN;
> +
> +	if (vq_is_packed(dev))
> +		vhost_vring_call_packed(dev, vq);
> +	else
> +		vhost_vring_call_split(dev, vq);
> +
> +	rte_spinlock_unlock(&vq->access_lock);
> +
> +	return 0;
> +}
> +
>  uint16_t
>  rte_vhost_avail_entries(int vid, uint16_t queue_id)
>  {
> --
> 2.21.3

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

RE: [PATCH v4] vhost: add new `rte_vhost_vring_call_nonblock` API

[Xia, Chenbo]

> -----Original Message-----
> From: Liu, Changpeng <changpeng.liu@intel.com>
> Sent: Monday, October 17, 2022 3:48 PM
> To: dev@dpdk.org
> Cc: Liu, Changpeng <changpeng.liu@intel.com>; Maxime Coquelin
> <maxime.coquelin@redhat.com>; Xia, Chenbo <chenbo.xia@intel.com>; David
> Marchand <david.marchand@redhat.com>
> Subject: [PATCH v4] vhost: add new `rte_vhost_vring_call_nonblock` API
> 
> Vhost-user library locks all VQ's access lock when processing
> vring based messages, such as SET_VRING_KICK and SET_VRING_CALL,
> and the data processing thread may already be started, e.g: SPDK
> vhost-blk and vhost-scsi will start the data processing thread
> when one vring is ready, then deadlock may happen when SPDK is
> posting interrupts to VM.  Here, we add a new API which allows
> caller to try again later for this case.
> 
> Bugzilla ID: 1015
> Fixes: c5736998305d ("vhost: fix missing virtqueue lock protection")
> 
> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> ---
>  doc/guides/prog_guide/vhost_lib.rst    |  6 ++++++
>  doc/guides/rel_notes/release_22_11.rst |  6 ++++++
>  lib/vhost/rte_vhost.h                  | 15 +++++++++++++
>  lib/vhost/version.map                  |  3 +++
>  lib/vhost/vhost.c                      | 30 ++++++++++++++++++++++++++
>  5 files changed, 60 insertions(+)
> 
> diff --git a/doc/guides/prog_guide/vhost_lib.rst
> b/doc/guides/prog_guide/vhost_lib.rst
> index bad4d819e1..52f0589f51 100644
> --- a/doc/guides/prog_guide/vhost_lib.rst
> +++ b/doc/guides/prog_guide/vhost_lib.rst
> @@ -297,6 +297,12 @@ The following is an overview of some key Vhost API
> functions:
>    Clear in-flight packets which are submitted to async channel in vhost
> async data
>    path. Completed packets are returned to applications through ``pkts``.
> 
> +* ``rte_vhost_vring_call_nonblock(int vid, uint16_t vring_idx)``
> +
> +  Notify the guest that used descriptors have been added to the vring.
> This function
> +  will return -EAGAIN when vq's access lock is held by other thread, user
> should try
> +  again later.
> +
>  * ``rte_vhost_vring_stats_get_names(int vid, uint16_t queue_id, struct
> rte_vhost_stat_name *names, unsigned int size)``
> 
>    This function returns the names of the queue statistics. It requires
> diff --git a/doc/guides/rel_notes/release_22_11.rst
> b/doc/guides/rel_notes/release_22_11.rst
> index 2da8bc9661..b3d1ba043c 100644
> --- a/doc/guides/rel_notes/release_22_11.rst
> +++ b/doc/guides/rel_notes/release_22_11.rst
> @@ -225,6 +225,12 @@ New Features
>    sysfs entries to adjust the minimum and maximum uncore frequency values,
>    which works on Linux with Intel hardware only.
> 
> +* **Added non-blocking notify API to the vhost library.**
> +
> +  Added ``rte_vhost_vring_call_nonblock`` API to notify the guest that
> +  used descriptors have been added to the vring in non-blocking way. User
> +  should check the return value of this API and try again if needed.
> +
>  * **Rewritten pmdinfo script.**
> 
>    The ``dpdk-pmdinfo.py`` script was rewritten to produce valid JSON only.
> diff --git a/lib/vhost/rte_vhost.h b/lib/vhost/rte_vhost.h
> index bb7d86a432..d22b25cd4e 100644
> --- a/lib/vhost/rte_vhost.h
> +++ b/lib/vhost/rte_vhost.h
> @@ -909,6 +909,21 @@ rte_vhost_clr_inflight_desc_packed(int vid, uint16_t
> vring_idx,
>   */
>  int rte_vhost_vring_call(int vid, uint16_t vring_idx);
> 
> +/**
> + * Notify the guest that used descriptors have been added to the vring.
> This
> + * function acts as a memory barrier.  This function will return -EAGAIN
> when
> + * vq's access lock is held by other thread, user should try again later.
> + *
> + * @param vid
> + *  vhost device ID
> + * @param vring_idx
> + *  vring index
> + * @return
> + *  0 on success, -1 on failure, -EAGAIN for another retry
> + */
> +__rte_experimental
> +int rte_vhost_vring_call_nonblock(int vid, uint16_t vring_idx);
> +
>  /**
>   * Get vhost RX queue avail count.
>   *
> diff --git a/lib/vhost/version.map b/lib/vhost/version.map
> index 7a00b65740..c8c44b8326 100644
> --- a/lib/vhost/version.map
> +++ b/lib/vhost/version.map
> @@ -94,6 +94,9 @@ EXPERIMENTAL {
>  	rte_vhost_async_try_dequeue_burst;
>  	rte_vhost_driver_get_vdpa_dev_type;
>  	rte_vhost_clear_queue;
> +
> +	# added in 22.11
> +	rte_vhost_vring_call_nonblock;
>  };
> 
>  INTERNAL {
> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> index 8740aa2788..ed6efb003f 100644
> --- a/lib/vhost/vhost.c
> +++ b/lib/vhost/vhost.c
> @@ -1317,6 +1317,36 @@ rte_vhost_vring_call(int vid, uint16_t vring_idx)
>  	return 0;
>  }
> 
> +int
> +rte_vhost_vring_call_nonblock(int vid, uint16_t vring_idx)
> +{
> +	struct virtio_net *dev;
> +	struct vhost_virtqueue *vq;
> +
> +	dev = get_device(vid);
> +	if (!dev)
> +		return -1;
> +
> +	if (vring_idx >= VHOST_MAX_VRING)
> +		return -1;
> +
> +	vq = dev->virtqueue[vring_idx];
> +	if (!vq)
> +		return -1;
> +
> +	if (!rte_spinlock_trylock(&vq->access_lock))
> +		return -EAGAIN;
> +
> +	if (vq_is_packed(dev))
> +		vhost_vring_call_packed(dev, vq);
> +	else
> +		vhost_vring_call_split(dev, vq);
> +
> +	rte_spinlock_unlock(&vq->access_lock);
> +
> +	return 0;
> +}
> +
>  uint16_t
>  rte_vhost_avail_entries(int vid, uint16_t queue_id)
>  {
> --
> 2.21.3

Change title to 'vhost: add non-blocking API for posting interrupt'

Applied to next-virtio/main. thanks