Re: [PATCH v1] net/vhost: add queue status check

[Maxime Coquelin <maxime.coquelin@redhat.com>]

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
>