DPDK patches and discussions
 help / color / mirror / Atom feed
* Re: [PATCH v1] net/vhost: add queue status check
  2021-11-16 16:44 [PATCH v1] net/vhost: add queue status check Miao Li
@ 2021-11-16  9:34 ` Maxime Coquelin
  2021-11-16  9:36   ` Maxime Coquelin
  0 siblings, 1 reply; 5+ messages in thread
From: Maxime Coquelin @ 2021-11-16  9:34 UTC (permalink / raw)
  To: Miao Li, dev; +Cc: chenbo.xia



On 11/16/21 17:44, Miao Li wrote:
> This patch adds queue status check to make sure that vhost monitor
> address will not be got until the link between backend and frontend
s/got/gone/?
> up and the packets are allowed to be queued.

It needs a fixes tag.

> Signed-off-by: Miao Li <miao.li@intel.com>
> ---
>   drivers/net/vhost/rte_eth_vhost.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
> index 070f0e6dfd..9d600054d8 100644
> --- a/drivers/net/vhost/rte_eth_vhost.c
> +++ b/drivers/net/vhost/rte_eth_vhost.c
> @@ -1415,6 +1415,8 @@ vhost_get_monitor_addr(void *rx_queue, struct rte_power_monitor_cond *pmc)
>   	int ret;
>   	if (vq == NULL)
>   		return -EINVAL;
> +	if (unlikely(rte_atomic32_read(&vq->allow_queuing) == 0))
> +		return -EINVAL;

How does it help?
What does prevent allow_queuing to become zero between the check and the
call to rte_vhost_get_monitor_addr?

I think you need to implement the same logic as in eth_vhost_rx(), i.e.
check allow_queueing, set while_queueing, check allow_queueing, do your
stuff and clear while_queuing.

>   	ret = rte_vhost_get_monitor_addr(vq->vid, vq->virtqueue_id,
>   			&vhost_pmc);
>   	if (ret < 0)
> 

Maxime


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v1] net/vhost: add queue status check
  2021-11-16  9:34 ` Maxime Coquelin
@ 2021-11-16  9:36   ` Maxime Coquelin
  2021-11-19  6:30     ` Li, Miao
  0 siblings, 1 reply; 5+ messages in thread
From: Maxime Coquelin @ 2021-11-16  9:36 UTC (permalink / raw)
  To: Miao Li, dev; +Cc: chenbo.xia



On 11/16/21 10:34, Maxime Coquelin wrote:
> 
> 
> On 11/16/21 17:44, Miao Li wrote:
>> This patch adds queue status check to make sure that vhost monitor
>> address will not be got until the link between backend and frontend
> s/got/gone/?
>> up and the packets are allowed to be queued.
> 
> It needs a fixes tag.
> 
>> Signed-off-by: Miao Li <miao.li@intel.com>
>> ---
>>   drivers/net/vhost/rte_eth_vhost.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/net/vhost/rte_eth_vhost.c 
>> b/drivers/net/vhost/rte_eth_vhost.c
>> index 070f0e6dfd..9d600054d8 100644
>> --- a/drivers/net/vhost/rte_eth_vhost.c
>> +++ b/drivers/net/vhost/rte_eth_vhost.c
>> @@ -1415,6 +1415,8 @@ vhost_get_monitor_addr(void *rx_queue, struct 
>> rte_power_monitor_cond *pmc)
>>       int ret;
>>       if (vq == NULL)
>>           return -EINVAL;
>> +    if (unlikely(rte_atomic32_read(&vq->allow_queuing) == 0))
>> +        return -EINVAL;

Also, EINVAL might not be the right return value here.

> How does it help?
> What does prevent allow_queuing to become zero between the check and the
> call to rte_vhost_get_monitor_addr?
> 
> I think you need to implement the same logic as in eth_vhost_rx(), i.e.
> check allow_queueing, set while_queueing, check allow_queueing, do your
> stuff and clear while_queuing.
> 
>>       ret = rte_vhost_get_monitor_addr(vq->vid, vq->virtqueue_id,
>>               &vhost_pmc);
>>       if (ret < 0)
>>
> 
> Maxime


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v1] net/vhost: add queue status check
@ 2021-11-16 16:44 Miao Li
  2021-11-16  9:34 ` Maxime Coquelin
  0 siblings, 1 reply; 5+ messages in thread
From: Miao Li @ 2021-11-16 16:44 UTC (permalink / raw)
  To: dev; +Cc: chenbo.xia, maxime.coquelin

This patch adds queue status check to make sure that vhost monitor
address will not be got until the link between backend and frontend
up and the packets are allowed to be queued.

Signed-off-by: Miao Li <miao.li@intel.com>
---
 drivers/net/vhost/rte_eth_vhost.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index 070f0e6dfd..9d600054d8 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -1415,6 +1415,8 @@ vhost_get_monitor_addr(void *rx_queue, struct rte_power_monitor_cond *pmc)
 	int ret;
 	if (vq == NULL)
 		return -EINVAL;
+	if (unlikely(rte_atomic32_read(&vq->allow_queuing) == 0))
+		return -EINVAL;
 	ret = rte_vhost_get_monitor_addr(vq->vid, vq->virtqueue_id,
 			&vhost_pmc);
 	if (ret < 0)
-- 
2.25.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH v1] net/vhost: add queue status check
  2021-11-16  9:36   ` Maxime Coquelin
@ 2021-11-19  6:30     ` Li, Miao
  2022-01-31  8:36       ` Maxime Coquelin
  0 siblings, 1 reply; 5+ messages in thread
From: Li, Miao @ 2021-11-19  6:30 UTC (permalink / raw)
  To: Maxime Coquelin, dev; +Cc: Xia, Chenbo

Hi

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, November 16, 2021 5:36 PM
> To: Li, Miao <miao.li@intel.com>; dev@dpdk.org
> Cc: Xia, Chenbo <chenbo.xia@intel.com>
> Subject: Re: [PATCH v1] net/vhost: add queue status check
> 
> 
> 
> On 11/16/21 10:34, Maxime Coquelin wrote:
> >
> >
> > On 11/16/21 17:44, Miao Li wrote:
> >> This patch adds queue status check to make sure that vhost monitor
> >> address will not be got until the link between backend and frontend
> > s/got/gone/?
> >> up and the packets are allowed to be queued.
> >
> > It needs a fixes tag.

If we don't add this check, rte_vhost_get_monitor_addr will return -EINVAL when check if dev is null. But before return, get_device() will be called and print error log "device not found". So we want to add this check and return -EINVAL before call rte_vhost_get_monitor_addr. If we don't add this check, the vhost monitor address will also not be got but vhost will print error log continuously. It have no function impact, so I think it is not a fix. 

> >
> >> Signed-off-by: Miao Li <miao.li@intel.com>
> >> ---
> >>   drivers/net/vhost/rte_eth_vhost.c | 2 ++
> >>   1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/net/vhost/rte_eth_vhost.c
> >> b/drivers/net/vhost/rte_eth_vhost.c
> >> index 070f0e6dfd..9d600054d8 100644
> >> --- a/drivers/net/vhost/rte_eth_vhost.c
> >> +++ b/drivers/net/vhost/rte_eth_vhost.c
> >> @@ -1415,6 +1415,8 @@ vhost_get_monitor_addr(void *rx_queue, struct
> >> rte_power_monitor_cond *pmc)
> >>       int ret;
> >>       if (vq == NULL)
> >>           return -EINVAL;
> >> +    if (unlikely(rte_atomic32_read(&vq->allow_queuing) == 0))
> >> +        return -EINVAL;
> 
> Also, EINVAL might not be the right return value here.

I don't know which return value will be better. Do you have any suggestions? Thanks!

> 
> > How does it help?
> > What does prevent allow_queuing to become zero between the check and the
> > call to rte_vhost_get_monitor_addr?

This check will prevent vhost to print error log continuously.

> >
> > I think you need to implement the same logic as in eth_vhost_rx(), i.e.
> > check allow_queueing, set while_queueing, check allow_queueing, do your
> > stuff and clear while_queuing.

I think the while_queuing is unnecessary because we only read the value in vq and this API will only be called as a callback of RX.

Thanks,
Miao

> >
> >>       ret = rte_vhost_get_monitor_addr(vq->vid, vq->virtqueue_id,
> >>               &vhost_pmc);
> >>       if (ret < 0)
> >>
> >
> > Maxime


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v1] net/vhost: add queue status check
  2021-11-19  6:30     ` Li, Miao
@ 2022-01-31  8:36       ` Maxime Coquelin
  0 siblings, 0 replies; 5+ messages in thread
From: Maxime Coquelin @ 2022-01-31  8:36 UTC (permalink / raw)
  To: Li, Miao, dev; +Cc: Xia, Chenbo



On 11/19/21 07:30, Li, Miao wrote:
> Hi
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Tuesday, November 16, 2021 5:36 PM
>> To: Li, Miao <miao.li@intel.com>; dev@dpdk.org
>> Cc: Xia, Chenbo <chenbo.xia@intel.com>
>> Subject: Re: [PATCH v1] net/vhost: add queue status check
>>
>>
>>
>> On 11/16/21 10:34, Maxime Coquelin wrote:
>>>
>>>
>>> On 11/16/21 17:44, Miao Li wrote:
>>>> This patch adds queue status check to make sure that vhost monitor
>>>> address will not be got until the link between backend and frontend
>>> s/got/gone/?
>>>> up and the packets are allowed to be queued.
>>>
>>> It needs a fixes tag.
> 
> If we don't add this check, rte_vhost_get_monitor_addr will return -EINVAL when check if dev is null. But before return, get_device() will be called and print error log "device not found". So we want to add this check and return -EINVAL before call rte_vhost_get_monitor_addr. If we don't add this check, the vhost monitor address will also not be got but vhost will print error log continuously. It have no function impact, so I think it is not a fix.
> 
>>>
>>>> Signed-off-by: Miao Li <miao.li@intel.com>
>>>> ---
>>>>    drivers/net/vhost/rte_eth_vhost.c | 2 ++
>>>>    1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/net/vhost/rte_eth_vhost.c
>>>> b/drivers/net/vhost/rte_eth_vhost.c
>>>> index 070f0e6dfd..9d600054d8 100644
>>>> --- a/drivers/net/vhost/rte_eth_vhost.c
>>>> +++ b/drivers/net/vhost/rte_eth_vhost.c
>>>> @@ -1415,6 +1415,8 @@ vhost_get_monitor_addr(void *rx_queue, struct
>>>> rte_power_monitor_cond *pmc)
>>>>        int ret;
>>>>        if (vq == NULL)
>>>>            return -EINVAL;
>>>> +    if (unlikely(rte_atomic32_read(&vq->allow_queuing) == 0))
>>>> +        return -EINVAL;
>>
>> Also, EINVAL might not be the right return value here.
> 
> I don't know which return value will be better. Do you have any suggestions? Thanks!
> 
>>
>>> How does it help?
>>> What does prevent allow_queuing to become zero between the check and the
>>> call to rte_vhost_get_monitor_addr?
> 
> This check will prevent vhost to print error log continuously.

You mean, it will prevent it most of the time, as there is still a
window where it can happen, if allow_queueing is set between is check
and the call to rte_vhost_get_monitor_addr.

>>>
>>> I think you need to implement the same logic as in eth_vhost_rx(), i.e.
>>> check allow_queueing, set while_queueing, check allow_queueing, do your
>>> stuff and clear while_queuing.
> 
> I think the while_queuing is unnecessary because we only read the value in vq and this API will only be called as a callback of RX.
> 
> Thanks,
> Miao
> 
>>>
>>>>        ret = rte_vhost_get_monitor_addr(vq->vid, vq->virtqueue_id,
>>>>                &vhost_pmc);
>>>>        if (ret < 0)
>>>>
>>>
>>> Maxime
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2022-01-31  8:36 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-16 16:44 [PATCH v1] net/vhost: add queue status check Miao Li
2021-11-16  9:34 ` Maxime Coquelin
2021-11-16  9:36   ` Maxime Coquelin
2021-11-19  6:30     ` Li, Miao
2022-01-31  8:36       ` Maxime Coquelin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).