patch 'net/vhost: fix Rx interrupt' has been queued to stable release 21.11.4
Kevin Traynor
ktraynor at redhat.com
Wed Mar 22 11:31:58 CET 2023
Hi,
FYI, your patch has been queued to stable release 21.11.4
Note it hasn't been pushed to http://dpdk.org/browse/dpdk-stable yet.
It will be pushed if I get no objections before 03/24/23. So please
shout if anyone has objections.
Also note that after the patch there's a diff of the upstream commit vs the
patch applied to the branch. This will indicate if there was any rebasing
needed to apply to the stable branch. If there were code changes for rebasing
(ie: not only metadata diffs), please double check that the rebase was
correctly done.
Queued patches are on a temporary branch at:
https://github.com/kevintraynor/dpdk-stable
This queued commit can be viewed at:
https://github.com/kevintraynor/dpdk-stable/commit/1585556d2cb3b4541747992b256d1b0672938bc6
Thanks.
Kevin
---
>From 1585556d2cb3b4541747992b256d1b0672938bc6 Mon Sep 17 00:00:00 2001
From: David Marchand <david.marchand at redhat.com>
Date: Thu, 9 Mar 2023 13:37:52 +0100
Subject: [PATCH] net/vhost: fix Rx interrupt
[ upstream commit 7995bf4c295464c3a4c3f4ac500f890ab55567e7 ]
In the situation when a port was started while no virtio driver was
connected, Rx interrupts were broken.
They were also broken after a virtio driver reconnects.
There were several issues mixed in:
- this driver was not exposing a fixed file descriptor per Rx queue,
If a virtio driver was not connected yet, each Rx queue vector was
pointing at a -1 fd, and an application could interpret this as a lack
of Rx interrupt support,
- when a virtio driver later (re)connected, this net/vhost driver was
hacking into the EAL layer epoll fd to remove a old vring kickfd and
insert the new vring kickfd. This hack constitutes a layer violation
plus users of rte_eth_dev_rx_intr_ctl_q_get_fd() were not notified of
this change,
- in the case of reconnection, because the interrupt handle was
reallocated, a 0 fd was failing to be removed from the EAL layer
epoll fd, which resulted in never fixing the EAL epoll fd,
To fix Rx interrupts:
- allocating (eth_vhost_install_intr) / releasing
(eth_vhost_uninstall_intr) the interrupt handle is moved when
starting / closing the port, while setting / resetting per rxq fd is
triggered by vhost events via some new helpers (see
eth_vhost_configure_intr and eth_vhost_unconfigure_intr),
- a "proxy" epoll fd is created per Rx queue at the time the interrupt
handle is allocated, so applications can start waiting for events on
those fds, even before a virtio driver initialises,
- when available, vring kickd are populated in the "proxy" epoll fd,
Bugzilla ID: 1135
Fixes: 3f8ff12821e4 ("vhost: support interrupt mode")
Fixes: 3d4cd4be577c ("net/vhost: fix interrupt mode")
Fixes: d61138d4f0e2 ("drivers: remove direct access to interrupt handle")
Signed-off-by: David Marchand <david.marchand at redhat.com>
Reviewed-by: Chenbo Xia <chenbo.xia at intel.com>
Tested-by: Dukai Yuan <dukaix.yuan at intel.com>
---
drivers/net/vhost/rte_eth_vhost.c | 328 ++++++++++++------------------
1 file changed, 132 insertions(+), 196 deletions(-)
diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index 31ff871bbc..4e44e123eb 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -98,6 +98,7 @@ struct vhost_queue {
uint16_t virtqueue_id;
struct vhost_stats stats;
- int intr_enable;
rte_spinlock_t intr_lock;
+ struct epoll_event ev;
+ int kickfd;
};
@@ -520,88 +521,57 @@ find_internal_resource(char *ifname)
}
-static int
+static void
eth_vhost_update_intr(struct rte_eth_dev *eth_dev, uint16_t rxq_idx)
-{
- struct rte_intr_handle *handle = eth_dev->intr_handle;
- struct rte_epoll_event rev, *elist;
- int epfd, ret;
-
- if (handle == NULL)
- return 0;
-
- elist = rte_intr_elist_index_get(handle, rxq_idx);
- if (rte_intr_efds_index_get(handle, rxq_idx) == elist->fd)
- return 0;
-
- VHOST_LOG(INFO, "kickfd for rxq-%d was changed, updating handler.\n",
- rxq_idx);
-
- if (elist->fd != -1)
- VHOST_LOG(ERR, "Unexpected previous kickfd value (Got %d, expected -1).\n",
- elist->fd);
-
- /*
- * First remove invalid epoll event, and then install
- * the new one. May be solved with a proper API in the
- * future.
- */
- epfd = elist->epfd;
- rev = *elist;
- ret = rte_epoll_ctl(epfd, EPOLL_CTL_DEL, rev.fd,
- elist);
- if (ret) {
- VHOST_LOG(ERR, "Delete epoll event failed.\n");
- return ret;
- }
-
- rev.fd = rte_intr_efds_index_get(handle, rxq_idx);
- if (rte_intr_elist_index_set(handle, rxq_idx, rev))
- return -rte_errno;
-
- elist = rte_intr_elist_index_get(handle, rxq_idx);
- ret = rte_epoll_ctl(epfd, EPOLL_CTL_ADD, rev.fd, elist);
- if (ret) {
- VHOST_LOG(ERR, "Add epoll event failed.\n");
- return ret;
- }
-
- return 0;
-}
-
-static int
-eth_rxq_intr_enable(struct rte_eth_dev *dev, uint16_t qid)
{
struct rte_vhost_vring vring;
struct vhost_queue *vq;
- int old_intr_enable, ret = 0;
- vq = dev->data->rx_queues[qid];
- if (!vq) {
- VHOST_LOG(ERR, "rxq%d is not setup yet\n", qid);
- return -1;
+ vq = eth_dev->data->rx_queues[rxq_idx];
+ if (vq == NULL || vq->vid < 0)
+ return;
+
+ if (rte_vhost_get_vhost_vring(vq->vid, (rxq_idx << 1) + 1, &vring) < 0) {
+ VHOST_LOG(DEBUG, "Failed to get rxq-%d's vring, skip!\n", rxq_idx);
+ return;
}
rte_spinlock_lock(&vq->intr_lock);
- old_intr_enable = vq->intr_enable;
- vq->intr_enable = 1;
- ret = eth_vhost_update_intr(dev, qid);
+
+ /* Remove previous kickfd from proxy epoll */
+ if (vq->kickfd >= 0 && vq->kickfd != vring.kickfd) {
+ if (epoll_ctl(vq->ev.data.fd, EPOLL_CTL_DEL, vq->kickfd, &vq->ev) < 0) {
+ VHOST_LOG(DEBUG, "Failed to unregister %d from rxq-%d epoll: %s\n",
+ vq->kickfd, rxq_idx, strerror(errno));
+ } else {
+ VHOST_LOG(DEBUG, "Unregistered %d from rxq-%d epoll\n",
+ vq->kickfd, rxq_idx);
+ }
+ vq->kickfd = -1;
+ }
+
+ /* Add new one, if valid */
+ if (vq->kickfd != vring.kickfd && vring.kickfd >= 0) {
+ if (epoll_ctl(vq->ev.data.fd, EPOLL_CTL_ADD, vring.kickfd, &vq->ev) < 0) {
+ VHOST_LOG(ERR, "Failed to register %d in rxq-%d epoll: %s\n",
+ vring.kickfd, rxq_idx, strerror(errno));
+ } else {
+ vq->kickfd = vring.kickfd;
+ VHOST_LOG(DEBUG, "Registered %d in rxq-%d epoll\n",
+ vq->kickfd, rxq_idx);
+ }
+ }
+
rte_spinlock_unlock(&vq->intr_lock);
+}
- if (ret < 0) {
- VHOST_LOG(ERR, "Failed to update rxq%d's intr\n", qid);
- vq->intr_enable = old_intr_enable;
- return ret;
- }
+static int
+eth_rxq_intr_enable(struct rte_eth_dev *dev, uint16_t qid)
+{
+ struct vhost_queue *vq = dev->data->rx_queues[qid];
- ret = rte_vhost_get_vhost_vring(vq->vid, (qid << 1) + 1, &vring);
- if (ret < 0) {
- VHOST_LOG(ERR, "Failed to get rxq%d's vring\n", qid);
- return ret;
- }
- VHOST_LOG(INFO, "Enable interrupt for rxq%d\n", qid);
- rte_vhost_enable_guest_notification(vq->vid, (qid << 1) + 1, 1);
- rte_wmb();
+ if (vq->vid >= 0)
+ rte_vhost_enable_guest_notification(vq->vid, (qid << 1) + 1, 1);
- return ret;
+ return 0;
}
@@ -609,24 +579,8 @@ static int
eth_rxq_intr_disable(struct rte_eth_dev *dev, uint16_t qid)
{
- struct rte_vhost_vring vring;
- struct vhost_queue *vq;
- int ret = 0;
+ struct vhost_queue *vq = dev->data->rx_queues[qid];
- vq = dev->data->rx_queues[qid];
- if (!vq) {
- VHOST_LOG(ERR, "rxq%d is not setup yet\n", qid);
- return -1;
- }
-
- ret = rte_vhost_get_vhost_vring(vq->vid, (qid << 1) + 1, &vring);
- if (ret < 0) {
- VHOST_LOG(ERR, "Failed to get rxq%d's vring\n", qid);
- return ret;
- }
- VHOST_LOG(INFO, "Disable interrupt for rxq%d\n", qid);
- rte_vhost_enable_guest_notification(vq->vid, (qid << 1) + 1, 0);
- rte_wmb();
-
- vq->intr_enable = 0;
+ if (vq->vid >= 0)
+ rte_vhost_enable_guest_notification(vq->vid, (qid << 1) + 1, 0);
return 0;
@@ -639,4 +593,12 @@ eth_vhost_uninstall_intr(struct rte_eth_dev *dev)
if (intr_handle != NULL) {
+ int i;
+
+ for (i = 0; i < dev->data->nb_rx_queues; i++) {
+ int epoll_fd = rte_intr_efds_index_get(dev->intr_handle, i);
+
+ if (epoll_fd >= 0)
+ close(epoll_fd);
+ }
rte_intr_vec_list_free(intr_handle);
rte_intr_instance_free(intr_handle);
@@ -648,13 +610,9 @@ static int
eth_vhost_install_intr(struct rte_eth_dev *dev)
{
- struct rte_vhost_vring vring;
- struct vhost_queue *vq;
int nb_rxq = dev->data->nb_rx_queues;
- int i;
- int ret;
+ struct vhost_queue *vq;
- /* uninstall firstly if we are reconnecting */
- if (dev->intr_handle != NULL)
- eth_vhost_uninstall_intr(dev);
+ int ret;
+ int i;
dev->intr_handle = rte_intr_instance_alloc(RTE_INTR_INSTANCE_F_PRIVATE);
@@ -664,5 +622,5 @@ eth_vhost_install_intr(struct rte_eth_dev *dev)
goto error;
}
- if (rte_intr_efd_counter_size_set(dev->intr_handle, sizeof(uint64_t))) {
+ if (rte_intr_efd_counter_size_set(dev->intr_handle, 0)) {
ret = -rte_errno;
goto error;
@@ -675,38 +633,26 @@ eth_vhost_install_intr(struct rte_eth_dev *dev)
}
-
- VHOST_LOG(INFO, "Prepare intr vec\n");
+ VHOST_LOG(DEBUG, "Prepare intr vec\n");
for (i = 0; i < nb_rxq; i++) {
+ int epoll_fd = epoll_create1(0);
+
+ if (epoll_fd < 0) {
+ VHOST_LOG(ERR, "Failed to create proxy epoll fd for rxq-%d\n", i);
+ ret = -errno;
+ goto error;
+ }
+
if (rte_intr_vec_list_index_set(dev->intr_handle, i,
- RTE_INTR_VEC_RXTX_OFFSET + i)) {
- ret = -rte_errno;
- goto error;
- }
- if (rte_intr_efds_index_set(dev->intr_handle, i, -1)) {
+ RTE_INTR_VEC_RXTX_OFFSET + i) ||
+ rte_intr_efds_index_set(dev->intr_handle, i, epoll_fd)) {
ret = -rte_errno;
+ close(epoll_fd);
goto error;
}
+
vq = dev->data->rx_queues[i];
- if (!vq) {
- VHOST_LOG(INFO, "rxq-%d not setup yet, skip!\n", i);
- continue;
- }
-
- ret = rte_vhost_get_vhost_vring(vq->vid, (i << 1) + 1, &vring);
- if (ret < 0) {
- VHOST_LOG(INFO,
- "Failed to get rxq-%d's vring, skip!\n", i);
- continue;
- }
-
- if (vring.kickfd < 0) {
- VHOST_LOG(INFO,
- "rxq-%d's kickfd is invalid, skip!\n", i);
- continue;
- }
-
- if (rte_intr_efds_index_set(dev->intr_handle, i, vring.kickfd))
- continue;
- VHOST_LOG(INFO, "Installed intr vec for rxq-%d\n", i);
+ memset(&vq->ev, 0, sizeof(vq->ev));
+ vq->ev.events = EPOLLIN;
+ vq->ev.data.fd = epoll_fd;
}
@@ -731,4 +677,44 @@ error:
}
+static void
+eth_vhost_configure_intr(struct rte_eth_dev *dev)
+{
+ int i;
+
+ VHOST_LOG(DEBUG, "Configure intr vec\n");
+ for (i = 0; i < dev->data->nb_rx_queues; i++)
+ eth_vhost_update_intr(dev, i);
+}
+
+static void
+eth_vhost_unconfigure_intr(struct rte_eth_dev *eth_dev)
+{
+ struct vhost_queue *vq;
+ int i;
+
+ VHOST_LOG(DEBUG, "Unconfigure intr vec\n");
+ for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
+ vq = eth_dev->data->rx_queues[i];
+ if (vq == NULL || vq->vid < 0)
+ continue;
+
+ rte_spinlock_lock(&vq->intr_lock);
+
+ /* Remove previous kickfd from proxy epoll */
+ if (vq->kickfd >= 0) {
+ if (epoll_ctl(vq->ev.data.fd, EPOLL_CTL_DEL, vq->kickfd, &vq->ev) < 0) {
+ VHOST_LOG(DEBUG, "Failed to unregister %d from rxq-%d epoll: %s\n",
+ vq->kickfd, i, strerror(errno));
+ } else {
+ VHOST_LOG(DEBUG, "Unregistered %d from rxq-%d epoll\n",
+ vq->kickfd, i);
+ }
+ vq->kickfd = -1;
+ }
+
+ rte_spinlock_unlock(&vq->intr_lock);
+ }
+}
+
static void
update_queuing_status(struct rte_eth_dev *dev, bool wait_queuing)
@@ -832,14 +818,6 @@ new_device(int vid)
if (rte_atomic32_read(&internal->started) == 1) {
queue_setup(eth_dev, internal);
-
- if (dev_conf->intr_conf.rxq) {
- if (eth_vhost_install_intr(eth_dev) < 0) {
- VHOST_LOG(INFO,
- "Failed to install interrupt handler.\n");
- return -1;
- }
- }
- } else {
- VHOST_LOG(INFO, "RX/TX queues not exist yet\n");
+ if (dev_conf->intr_conf.rxq)
+ eth_vhost_configure_intr(eth_dev);
}
@@ -883,4 +861,5 @@ destroy_device(int vid)
rte_atomic32_set(&internal->dev_attached, 0);
update_queuing_status(eth_dev, true);
+ eth_vhost_unconfigure_intr(eth_dev);
eth_dev->data->dev_link.link_status = RTE_ETH_LINK_DOWN;
@@ -911,53 +890,8 @@ destroy_device(int vid)
VHOST_LOG(INFO, "Vhost device %d destroyed\n", vid);
- eth_vhost_uninstall_intr(eth_dev);
rte_eth_dev_callback_process(eth_dev, RTE_ETH_EVENT_INTR_LSC, NULL);
}
-static int
-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 vhost_queue *vq;
- struct rte_vhost_vring vring;
- int rx_idx = vring_id % 2 ? (vring_id - 1) >> 1 : -1;
- int ret = 0;
-
- /*
- * The vring kickfd may be changed after the new device notification.
- * Update it when the vring state is updated.
- */
- if (rx_idx >= 0 && rx_idx < eth_dev->data->nb_rx_queues &&
- rte_atomic32_read(&internal->dev_attached) &&
- rte_atomic32_read(&internal->started) &&
- dev_conf->intr_conf.rxq) {
- ret = rte_vhost_get_vhost_vring(vid, vring_id, &vring);
- if (ret) {
- VHOST_LOG(ERR, "Failed to get vring %d information.\n",
- vring_id);
- return ret;
- }
-
- if (rte_intr_efds_index_set(eth_dev->intr_handle, rx_idx,
- vring.kickfd))
- return -rte_errno;
-
- vq = eth_dev->data->rx_queues[rx_idx];
- if (!vq) {
- VHOST_LOG(ERR, "rxq%d is not setup yet\n", rx_idx);
- return -1;
- }
-
- rte_spinlock_lock(&vq->intr_lock);
- if (vq->intr_enable)
- ret = eth_vhost_update_intr(eth_dev, rx_idx);
- rte_spinlock_unlock(&vq->intr_lock);
- }
-
- return ret;
-}
-
static int
vring_state_changed(int vid, uint16_t vring, int enable)
@@ -979,7 +913,6 @@ vring_state_changed(int vid, uint16_t vring, int enable)
state = vring_states[eth_dev->data->port_id];
- if (enable && vring_conf_update(vid, eth_dev, vring))
- VHOST_LOG(INFO, "Failed to update vring-%d configuration.\n",
- (int)vring);
+ if (eth_dev->data->dev_conf.intr_conf.rxq && vring % 2)
+ eth_vhost_update_intr(eth_dev, (vring - 1) >> 1);
rte_spinlock_lock(&state->lock);
@@ -1166,15 +1099,14 @@ eth_dev_start(struct rte_eth_dev *eth_dev)
struct rte_eth_conf *dev_conf = ð_dev->data->dev_conf;
+ eth_vhost_uninstall_intr(eth_dev);
+ if (dev_conf->intr_conf.rxq && eth_vhost_install_intr(eth_dev) < 0) {
+ VHOST_LOG(ERR, "Failed to install interrupt handler.\n");
+ return -1;
+ }
+
queue_setup(eth_dev, internal);
-
- if (rte_atomic32_read(&internal->dev_attached) == 1) {
- if (dev_conf->intr_conf.rxq) {
- if (eth_vhost_install_intr(eth_dev) < 0) {
- VHOST_LOG(INFO,
- "Failed to install interrupt handler.\n");
- return -1;
- }
- }
- }
+ if (rte_atomic32_read(&internal->dev_attached) == 1 &&
+ dev_conf->intr_conf.rxq)
+ eth_vhost_configure_intr(eth_dev);
rte_atomic32_set(&internal->started, 1);
@@ -1232,4 +1164,6 @@ eth_dev_close(struct rte_eth_dev *dev)
rte_free(internal);
+ eth_vhost_uninstall_intr(dev);
+
dev->data->dev_private = NULL;
@@ -1259,4 +1193,5 @@ eth_rx_queue_setup(struct rte_eth_dev *dev, uint16_t rx_queue_id,
vq->virtqueue_id = rx_queue_id * VIRTIO_QNUM + VIRTIO_TXQ;
rte_spinlock_init(&vq->intr_lock);
+ vq->kickfd = -1;
dev->data->rx_queues[rx_queue_id] = vq;
@@ -1281,4 +1216,5 @@ eth_tx_queue_setup(struct rte_eth_dev *dev, uint16_t tx_queue_id,
vq->virtqueue_id = tx_queue_id * VIRTIO_QNUM + VIRTIO_RXQ;
rte_spinlock_init(&vq->intr_lock);
+ vq->kickfd = -1;
dev->data->tx_queues[tx_queue_id] = vq;
--
2.39.2
---
Diff of the applied patch vs upstream commit (please double-check if non-empty:
---
--- - 2023-03-22 10:30:08.202826988 +0000
+++ 0011-net-vhost-fix-Rx-interrupt.patch 2023-03-22 10:30:07.913866562 +0000
@@ -1 +1 @@
-From 7995bf4c295464c3a4c3f4ac500f890ab55567e7 Mon Sep 17 00:00:00 2001
+From 1585556d2cb3b4541747992b256d1b0672938bc6 Mon Sep 17 00:00:00 2001
@@ -5,0 +6,2 @@
+[ upstream commit 7995bf4c295464c3a4c3f4ac500f890ab55567e7 ]
+
@@ -39 +40,0 @@
-Cc: stable at dpdk.org
@@ -49 +50 @@
-index 96deb18d91..62ef955ebc 100644
+index 31ff871bbc..4e44e123eb 100644
@@ -52 +53 @@
-@@ -79,6 +79,7 @@ struct vhost_queue {
+@@ -98,6 +98,7 @@ struct vhost_queue {
@@ -61 +62 @@
-@@ -546,88 +547,57 @@ find_internal_resource(char *ifname)
+@@ -520,88 +521,57 @@ find_internal_resource(char *ifname)
@@ -191 +192 @@
-@@ -635,24 +605,8 @@ static int
+@@ -609,24 +579,8 @@ static int
@@ -219 +220 @@
-@@ -665,4 +619,12 @@ eth_vhost_uninstall_intr(struct rte_eth_dev *dev)
+@@ -639,4 +593,12 @@ eth_vhost_uninstall_intr(struct rte_eth_dev *dev)
@@ -232 +233 @@
-@@ -674,13 +636,9 @@ static int
+@@ -648,13 +610,9 @@ static int
@@ -249 +250 @@
-@@ -690,5 +648,5 @@ eth_vhost_install_intr(struct rte_eth_dev *dev)
+@@ -664,5 +622,5 @@ eth_vhost_install_intr(struct rte_eth_dev *dev)
@@ -256 +257 @@
-@@ -701,38 +659,26 @@ eth_vhost_install_intr(struct rte_eth_dev *dev)
+@@ -675,38 +633,26 @@ eth_vhost_install_intr(struct rte_eth_dev *dev)
@@ -311 +312 @@
-@@ -757,4 +703,44 @@ error:
+@@ -731,4 +677,44 @@ error:
@@ -356 +357 @@
-@@ -863,14 +849,6 @@ new_device(int vid)
+@@ -832,14 +818,6 @@ new_device(int vid)
@@ -373 +374 @@
-@@ -916,4 +894,5 @@ destroy_device(int vid)
+@@ -883,4 +861,5 @@ destroy_device(int vid)
@@ -379 +380 @@
-@@ -944,53 +923,8 @@ destroy_device(int vid)
+@@ -911,53 +890,8 @@ destroy_device(int vid)
@@ -433 +434 @@
-@@ -1012,7 +946,6 @@ vring_state_changed(int vid, uint16_t vring, int enable)
+@@ -979,7 +913,6 @@ vring_state_changed(int vid, uint16_t vring, int enable)
@@ -443 +444 @@
-@@ -1201,15 +1134,14 @@ eth_dev_start(struct rte_eth_dev *eth_dev)
+@@ -1166,15 +1099,14 @@ eth_dev_start(struct rte_eth_dev *eth_dev)
@@ -468 +469 @@
-@@ -1267,4 +1199,6 @@ eth_dev_close(struct rte_eth_dev *dev)
+@@ -1232,4 +1164,6 @@ eth_dev_close(struct rte_eth_dev *dev)
@@ -475 +476 @@
-@@ -1294,4 +1228,5 @@ eth_rx_queue_setup(struct rte_eth_dev *dev, uint16_t rx_queue_id,
+@@ -1259,4 +1193,5 @@ eth_rx_queue_setup(struct rte_eth_dev *dev, uint16_t rx_queue_id,
@@ -481 +482 @@
-@@ -1316,4 +1251,5 @@ eth_tx_queue_setup(struct rte_eth_dev *dev, uint16_t tx_queue_id,
+@@ -1281,4 +1216,5 @@ eth_tx_queue_setup(struct rte_eth_dev *dev, uint16_t tx_queue_id,
More information about the stable
mailing list