diff mbox series

[v1] net/vhost: add queue status check

Message ID 20211116164446.149453-1-miao.li@intel.com (mailing list archive)
State Changes Requested, archived
Delegated to: Maxime Coquelin
Headers
Series [v1] net/vhost: add queue status check |

Checks

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

Commit Message

Li, Miao Nov. 16, 2021, 4:44 p.m. UTC 
  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(+)

Comments

Maxime Coquelin Nov. 16, 2021, 9:34 a.m. UTC | #1 
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
Maxime Coquelin Nov. 16, 2021, 9:36 a.m. UTC | #2 
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
Li, Miao Nov. 19, 2021, 6:30 a.m. UTC | #3 
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
Maxime Coquelin Jan. 31, 2022, 8:36 a.m. UTC | #4 
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
>
diff mbox series

Patch

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)