[2/2] 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>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
drivers/net/vhost/rte_eth_vhost.c | 25 ++++++-------------------
1 file changed, 6 insertions(+), 19 deletions(-)
Comments
Hi Maxime,
> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Thursday, July 23, 2020 9:09 PM
> 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 2/2] 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>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
> drivers/net/vhost/rte_eth_vhost.c | 25 ++++++-------------------
> 1 file changed, 6 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/net/vhost/rte_eth_vhost.c
> b/drivers/net/vhost/rte_eth_vhost.c
> index bbf79b2c0e..14b7b59f67 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,14 @@ 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)
> + return ret;
Do you think it'll be better to add a VHOST_LOG here to show the get_vring failure
like it's done in other place? But since it's the only place that will fail, it's also easy
for user to find out. 😊
Thanks,
Chenbo
>
> - 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
@@ -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,14 @@ 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)
+ 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;
}
}