Hi > -----Original Message----- > From: Maxime Coquelin <maxime.coque...@redhat.com> > Sent: Tuesday, November 16, 2021 5:36 PM > To: Li, Miao <miao...@intel.com>; dev@dpdk.org > Cc: Xia, Chenbo <chenbo....@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...@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
