[v2,2/3] net/vhost: fix queue update
Checks
Commit Message
Now that the vhost library saves the guest notifications
enablement value in its virtqueues metadata, it is not
necessary to do it in the vring_state_changed callback.
One effect of the patch is also to prevent possible
deadlock happening in vhost library.
Fixes: 604052ae5395 ("net/vhost: support queue update")
Reported-by: Yinan Wang <yinan.wang@intel.com>
Tested-by: Yinan Wang <yinan.wang@intel.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
drivers/net/vhost/rte_eth_vhost.c | 28 +++++++++-------------------
1 file changed, 9 insertions(+), 19 deletions(-)
Comments
> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Wednesday, July 29, 2020 12:50 AM
> To: dev@dpdk.org; matan@mellanox.com; Xia, Chenbo
> <chenbo.xia@intel.com>; Liu, Yong <yong.liu@intel.com>; Wang, Yinan
> <yinan.wang@intel.com>
> Cc: thomas@monjalon.net; Yigit, Ferruh <ferruh.yigit@intel.com>;
> david.marchand@redhat.com; Maxime Coquelin
> <maxime.coquelin@redhat.com>
> Subject: [PATCH v2 2/3] net/vhost: fix queue update
>
> Now that the vhost library saves the guest notifications enablement value in its
> virtqueues metadata, it is not necessary to do it in the vring_state_changed
> callback.
>
> One effect of the patch is also to prevent possible deadlock happening in vhost
> library.
>
> Fixes: 604052ae5395 ("net/vhost: support queue update")
>
> Reported-by: Yinan Wang <yinan.wang@intel.com>
> Tested-by: Yinan Wang <yinan.wang@intel.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
> drivers/net/vhost/rte_eth_vhost.c | 28 +++++++++-------------------
> 1 file changed, 9 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/net/vhost/rte_eth_vhost.c
> b/drivers/net/vhost/rte_eth_vhost.c
> index bbf79b2c0e..951929c663 100644
> --- a/drivers/net/vhost/rte_eth_vhost.c
> +++ b/drivers/net/vhost/rte_eth_vhost.c
> @@ -94,7 +94,6 @@ struct vhost_queue {
> struct rte_mempool *mb_pool;
> uint16_t port;
> uint16_t virtqueue_id;
> - bool intr_en;
> struct vhost_stats stats;
> };
>
> @@ -547,8 +546,6 @@ eth_rxq_intr_enable(struct rte_eth_dev *dev, uint16_t
> qid)
> rte_vhost_enable_guest_notification(vq->vid, (qid << 1) + 1, 1);
> rte_wmb();
>
> - vq->intr_en = true;
> -
> return ret;
> }
>
> @@ -574,8 +571,6 @@ eth_rxq_intr_disable(struct rte_eth_dev *dev, uint16_t
> qid)
> rte_vhost_enable_guest_notification(vq->vid, (qid << 1) + 1, 0);
> rte_wmb();
>
> - vq->intr_en = false;
> -
> return 0;
> }
>
> @@ -841,7 +836,6 @@ vring_conf_update(int vid, struct rte_eth_dev *eth_dev,
> uint16_t vring_id)
> struct rte_eth_conf *dev_conf = ð_dev->data->dev_conf;
> struct pmd_internal *internal = eth_dev->data->dev_private;
> struct rte_vhost_vring vring;
> - struct vhost_queue *vq;
> int rx_idx = vring_id % 2 ? (vring_id - 1) >> 1 : -1;
> int ret = 0;
>
> @@ -853,21 +847,17 @@ vring_conf_update(int vid, struct rte_eth_dev
> *eth_dev, uint16_t vring_id)
> rte_atomic32_read(&internal->dev_attached) &&
> rte_atomic32_read(&internal->started) &&
> dev_conf->intr_conf.rxq) {
> - vq = eth_dev->data->rx_queues[rx_idx];
> ret = rte_vhost_get_vhost_vring(vid, vring_id, &vring);
> - if (!ret) {
> - if (vring.kickfd !=
> - eth_dev->intr_handle->efds[rx_idx]) {
> - VHOST_LOG(INFO,
> - "kickfd for rxq-%d was changed.\n",
> - rx_idx);
> - eth_dev->intr_handle->efds[rx_idx] =
> - vring.kickfd;
> - }
> + if (ret) {
> + VHOST_LOG(ERR, "Failed to get vring %d
> information.\n",
> + vring_id);
> + return ret;
> + }
>
> - rte_vhost_enable_guest_notification(vid, vring_id,
> - vq->intr_en);
> - rte_wmb();
> + if (vring.kickfd != eth_dev->intr_handle->efds[rx_idx]) {
> + VHOST_LOG(INFO, "kickfd for rxq-%d was changed.\n",
> + rx_idx);
> + eth_dev->intr_handle->efds[rx_idx] = vring.kickfd;
> }
> }
>
> --
> 2.26.2
Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
> -----Original Message-----
> From: Xia, Chenbo <chenbo.xia@intel.com>
> Sent: 2020?7?29? 10:53
> To: Maxime Coquelin <maxime.coquelin@redhat.com>; dev@dpdk.org;
> matan@mellanox.com; Liu, Yong <yong.liu@intel.com>; Wang, Yinan
> <yinan.wang@intel.com>
> Cc: thomas@monjalon.net; Yigit, Ferruh <ferruh.yigit@intel.com>;
> david.marchand@redhat.com
> Subject: RE: [PATCH v2 2/3] net/vhost: fix queue update
>
>
> > -----Original Message-----
> > From: Maxime Coquelin <maxime.coquelin@redhat.com>
> > Sent: Wednesday, July 29, 2020 12:50 AM
> > To: dev@dpdk.org; matan@mellanox.com; Xia, Chenbo
> > <chenbo.xia@intel.com>; Liu, Yong <yong.liu@intel.com>; Wang, Yinan
> > <yinan.wang@intel.com>
> > Cc: thomas@monjalon.net; Yigit, Ferruh <ferruh.yigit@intel.com>;
> > david.marchand@redhat.com; Maxime Coquelin
> > <maxime.coquelin@redhat.com>
> > Subject: [PATCH v2 2/3] net/vhost: fix queue update
> >
> > Now that the vhost library saves the guest notifications enablement value
> in its
> > virtqueues metadata, it is not necessary to do it in the vring_state_changed
> > callback.
> >
> > One effect of the patch is also to prevent possible deadlock happening in
> vhost
> > library.
> >
> > Fixes: 604052ae5395 ("net/vhost: support queue update")
> >
> > Reported-by: Yinan Wang <yinan.wang@intel.com>
> > Tested-by: Yinan Wang <yinan.wang@intel.com>
> > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> > ---
> > drivers/net/vhost/rte_eth_vhost.c | 28 +++++++++-------------------
> > 1 file changed, 9 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/net/vhost/rte_eth_vhost.c
> > b/drivers/net/vhost/rte_eth_vhost.c
> > index bbf79b2c0e..951929c663 100644
> > --- a/drivers/net/vhost/rte_eth_vhost.c
> > +++ b/drivers/net/vhost/rte_eth_vhost.c
> > @@ -94,7 +94,6 @@ struct vhost_queue {
> > struct rte_mempool *mb_pool;
> > uint16_t port;
> > uint16_t virtqueue_id;
> > - bool intr_en;
> > struct vhost_stats stats;
> > };
> >
> > @@ -547,8 +546,6 @@ eth_rxq_intr_enable(struct rte_eth_dev *dev,
> uint16_t
> > qid)
> > rte_vhost_enable_guest_notification(vq->vid, (qid << 1) + 1, 1);
> > rte_wmb();
> >
> > - vq->intr_en = true;
> > -
> > return ret;
> > }
> >
> > @@ -574,8 +571,6 @@ eth_rxq_intr_disable(struct rte_eth_dev *dev,
> uint16_t
> > qid)
> > rte_vhost_enable_guest_notification(vq->vid, (qid << 1) + 1, 0);
> > rte_wmb();
> >
> > - vq->intr_en = false;
> > -
> > return 0;
> > }
> >
> > @@ -841,7 +836,6 @@ vring_conf_update(int vid, struct rte_eth_dev
> *eth_dev,
> > uint16_t vring_id)
> > struct rte_eth_conf *dev_conf = ð_dev->data->dev_conf;
> > struct pmd_internal *internal = eth_dev->data->dev_private;
> > struct rte_vhost_vring vring;
> > - struct vhost_queue *vq;
> > int rx_idx = vring_id % 2 ? (vring_id - 1) >> 1 : -1;
> > int ret = 0;
> >
> > @@ -853,21 +847,17 @@ vring_conf_update(int vid, struct rte_eth_dev
> > *eth_dev, uint16_t vring_id)
> > rte_atomic32_read(&internal->dev_attached) &&
> > rte_atomic32_read(&internal->started) &&
> > dev_conf->intr_conf.rxq) {
> > - vq = eth_dev->data->rx_queues[rx_idx];
> > ret = rte_vhost_get_vhost_vring(vid, vring_id, &vring);
> > - if (!ret) {
> > - if (vring.kickfd !=
> > - eth_dev->intr_handle->efds[rx_idx]) {
> > - VHOST_LOG(INFO,
> > - "kickfd for rxq-%d was changed.\n",
> > - rx_idx);
> > - eth_dev->intr_handle->efds[rx_idx] =
> > - vring.kickfd;
> > - }
> > + if (ret) {
> > + VHOST_LOG(ERR, "Failed to get vring %d
> > information.\n",
> > + vring_id);
> > + return ret;
> > + }
> >
> > - rte_vhost_enable_guest_notification(vid, vring_id,
> > - vq->intr_en);
> > - rte_wmb();
> > + if (vring.kickfd != eth_dev->intr_handle->efds[rx_idx]) {
> > + VHOST_LOG(INFO, "kickfd for rxq-%d was
> changed.\n",
> > + rx_idx);
> > + eth_dev->intr_handle->efds[rx_idx] = vring.kickfd;
> > }
> > }
> >
> > --
> > 2.26.2
>
> Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
Tested-by: Yinan Wang <yinan.wang@intel.com>
@@ -94,7 +94,6 @@ struct vhost_queue {
struct rte_mempool *mb_pool;
uint16_t port;
uint16_t virtqueue_id;
- bool intr_en;
struct vhost_stats stats;
};
@@ -547,8 +546,6 @@ eth_rxq_intr_enable(struct rte_eth_dev *dev, uint16_t qid)
rte_vhost_enable_guest_notification(vq->vid, (qid << 1) + 1, 1);
rte_wmb();
- vq->intr_en = true;
-
return ret;
}
@@ -574,8 +571,6 @@ eth_rxq_intr_disable(struct rte_eth_dev *dev, uint16_t qid)
rte_vhost_enable_guest_notification(vq->vid, (qid << 1) + 1, 0);
rte_wmb();
- vq->intr_en = false;
-
return 0;
}
@@ -841,7 +836,6 @@ vring_conf_update(int vid, struct rte_eth_dev *eth_dev, uint16_t vring_id)
struct rte_eth_conf *dev_conf = ð_dev->data->dev_conf;
struct pmd_internal *internal = eth_dev->data->dev_private;
struct rte_vhost_vring vring;
- struct vhost_queue *vq;
int rx_idx = vring_id % 2 ? (vring_id - 1) >> 1 : -1;
int ret = 0;
@@ -853,21 +847,17 @@ vring_conf_update(int vid, struct rte_eth_dev *eth_dev, uint16_t vring_id)
rte_atomic32_read(&internal->dev_attached) &&
rte_atomic32_read(&internal->started) &&
dev_conf->intr_conf.rxq) {
- vq = eth_dev->data->rx_queues[rx_idx];
ret = rte_vhost_get_vhost_vring(vid, vring_id, &vring);
- if (!ret) {
- if (vring.kickfd !=
- eth_dev->intr_handle->efds[rx_idx]) {
- VHOST_LOG(INFO,
- "kickfd for rxq-%d was changed.\n",
- rx_idx);
- eth_dev->intr_handle->efds[rx_idx] =
- vring.kickfd;
- }
+ if (ret) {
+ VHOST_LOG(ERR, "Failed to get vring %d information.\n",
+ vring_id);
+ return ret;
+ }
- rte_vhost_enable_guest_notification(vid, vring_id,
- vq->intr_en);
- rte_wmb();
+ if (vring.kickfd != eth_dev->intr_handle->efds[rx_idx]) {
+ VHOST_LOG(INFO, "kickfd for rxq-%d was changed.\n",
+ rx_idx);
+ eth_dev->intr_handle->efds[rx_idx] = vring.kickfd;
}
}