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

["Li, Miao" <miao.li@intel.com>]

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