* [PATCH v1] net/vhost: add queue status check @ 2021-11-16 16:44 Miao Li 2021-11-16 9:34 ` Maxime Coquelin 0 siblings, 1 reply; 5+ messages in thread From: Miao Li @ 2021-11-16 16:44 UTC (permalink / raw) To: dev; +Cc: chenbo.xia, maxime.coquelin 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(+) 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) -- 2.25.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1] net/vhost: add queue status check 2021-11-16 16:44 [PATCH v1] net/vhost: add queue status check Miao Li @ 2021-11-16 9:34 ` Maxime Coquelin 2021-11-16 9:36 ` Maxime Coquelin 0 siblings, 1 reply; 5+ messages in thread From: Maxime Coquelin @ 2021-11-16 9:34 UTC (permalink / raw) To: Miao Li, dev; +Cc: chenbo.xia 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1] net/vhost: add queue status check 2021-11-16 9:34 ` Maxime Coquelin @ 2021-11-16 9:36 ` Maxime Coquelin 2021-11-19 6:30 ` Li, Miao 0 siblings, 1 reply; 5+ messages in thread From: Maxime Coquelin @ 2021-11-16 9:36 UTC (permalink / raw) To: Miao Li, dev; +Cc: chenbo.xia 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH v1] net/vhost: add queue status check 2021-11-16 9:36 ` Maxime Coquelin @ 2021-11-19 6:30 ` Li, Miao 2022-01-31 8:36 ` Maxime Coquelin 0 siblings, 1 reply; 5+ messages in thread From: Li, Miao @ 2021-11-19 6:30 UTC (permalink / raw) To: Maxime Coquelin, dev; +Cc: Xia, Chenbo 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1] net/vhost: add queue status check 2021-11-19 6:30 ` Li, Miao @ 2022-01-31 8:36 ` Maxime Coquelin 0 siblings, 0 replies; 5+ messages in thread From: Maxime Coquelin @ 2022-01-31 8:36 UTC (permalink / raw) To: Li, Miao, dev; +Cc: Xia, Chenbo 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-01-31 8:36 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-16 16:44 [PATCH v1] net/vhost: add queue status check Miao Li 2021-11-16 9:34 ` Maxime Coquelin 2021-11-16 9:36 ` Maxime Coquelin 2021-11-19 6:30 ` Li, Miao 2022-01-31 8:36 ` Maxime Coquelin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).