[v1] net/vhost: add queue status check
Checks
Commit Message
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
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
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
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
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
>
@@ -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)