From: Joshua Washington <[email protected]> The DPDK alarm and interrupts infrastructure make use of the same global thread for processing. This causes problems in GVE related to the device status poller, as the reset path invoked by that poller sends mailbox messages and blocks waiting for their completion.
When the device status poller is on the alarm thread, that blocking call leads to a temporary lockup, as the mailbox interrupt cannot come in while the status poller holds the global alarm thread, causing message timeouts. Because the interrupts infrastructure is managed by DPDK itself, that leaves moving the device status poller off of the shared global alarm/interrupt thread as the only viable option. Spin off a device status thread on initialization that checks for resets. The single thread will stay alive until the driver is torn down in gve_dev_close. Update the polling frequency to every 100ms for quicker error recovery, since the poller is no longer on the shared thread. Signed-off-by: Joshua Washington <[email protected]> Signed-off-by: Jasper Tran O'Leary <[email protected]> --- drivers/net/gve/base/gve.h | 1 + drivers/net/gve/gve_ethdev.c | 81 ++++++++++++++++--------------- drivers/net/gve/gve_ethdev.h | 30 +++++++++++- 3 files changed, 72 insertions(+), 40 deletions(-) diff --git a/drivers/net/gve/base/gve.h b/drivers/net/gve/base/gve.h index 18363fae3..4d9890625 100644 --- a/drivers/net/gve/base/gve.h +++ b/drivers/net/gve/base/gve.h @@ -52,6 +52,7 @@ enum gve_state_flags_bit { GVE_PRIV_FLAGS_DEVICE_RINGS_OK = 3, GVE_PRIV_FLAGS_FLOW_SUBSYSTEM_OK = 4, GVE_PRIV_FLAGS_NAPI_ENABLED = 5, + GVE_PRIV_FLAGS_STATUS_POLLER_OK = 7, }; enum gve_rss_hash_algorithm { diff --git a/drivers/net/gve/gve_ethdev.c b/drivers/net/gve/gve_ethdev.c index c2cfd18dc..7c335a72d 100644 --- a/drivers/net/gve/gve_ethdev.c +++ b/drivers/net/gve/gve_ethdev.c @@ -590,7 +590,6 @@ gve_dev_start(struct rte_eth_dev *dev) } } - gve_start_dev_status_polling(dev); ret = 0; unlock_and_return: @@ -703,14 +702,6 @@ gve_dev_stop(struct rte_eth_dev *dev) return -EPERM; } - /* - * Block until all polling callbacks have concluded before tearing down - * any device resources. Do so before trying to acquire the reset lock - * because a polling callback could be invoking a reset which will - * itself try to acquire the lock. - */ - gve_stop_dev_status_polling(dev); - pthread_mutex_lock(&priv->reset_lock); dev->data->dev_started = 0; @@ -869,6 +860,16 @@ gve_dev_close(struct rte_eth_dev *dev) return 0; } + /* + * Block until all polling callbacks have concluded before tearing down + * any device resources. Do so before trying to acquire the reset lock + * in gve_dev_stop() because a polling callback could be invoking a + * reset which will itself try to acquire the lock, leading to a + * deadlock. + */ + if (gve_get_dev_status_poller_ok(priv)) + gve_stop_dev_status_polling(dev); + if (dev->data->dev_started) { err = gve_dev_stop(dev); if (err != 0) @@ -1691,8 +1692,6 @@ gve_internal_recover_device(struct rte_eth_dev *dev) } } - gve_stop_dev_status_polling(dev); - if (gve_is_gqi(priv)) gve_free_stats_report(dev); @@ -1772,63 +1771,59 @@ recover_fail: return ret; } -static void +static unsigned int gve_check_device_status(void *arg) { struct gve_process_private *process_priv; struct rte_eth_dev *dev = arg; struct gve_priv *priv = dev->data->dev_private; - int ret; process_priv = dev->process_private; - gve_clean_zombie_queues(priv, false); + while (gve_get_dev_status_poller_ok(priv)) { + gve_clean_zombie_queues(priv, false); + + if (!process_priv->ctrl_ops->check_device_needs_reset(priv)) + goto sleep; - if (process_priv->ctrl_ops->check_device_needs_reset(priv)) { pthread_mutex_lock(&priv->reset_lock); /* * Defend against TOCTOU by checking if a reset is still needed * after acquiring the lock. */ - if (process_priv->ctrl_ops->check_device_needs_reset(priv)) { - PMD_DRV_LOG(NOTICE, - "Port %u: Device requested reset. Executing proactive recovery.", - dev->data->port_id); - - ret = gve_internal_recover_device(dev); - if (ret == 0) { - pthread_mutex_unlock(&priv->reset_lock); - return; - } - } + if (!process_priv->ctrl_ops->check_device_needs_reset(priv)) + goto unlock; + + PMD_DRV_LOG(NOTICE, + "Port %u: Device requested reset. Executing proactive recovery.", + dev->data->port_id); + gve_internal_recover_device(dev); +unlock: pthread_mutex_unlock(&priv->reset_lock); +sleep: + rte_delay_us_sleep(GVE_DEV_POLL_INTERVAL_US); } - if (dev->data->dev_started) { - ret = rte_eal_alarm_set(GVE_DEV_POLL_INTERVAL_US, - gve_check_device_status, dev); - if (ret != 0) - PMD_DRV_LOG(ERR, - "Port %u: Failed to re-arm alarm poller!", - dev->data->port_id); - } + return 0; } static void gve_start_dev_status_polling(struct rte_eth_dev *dev) { + struct gve_priv *priv = dev->data->dev_private; int ret; if (rte_eal_process_type() != RTE_PROC_PRIMARY) return; - ret = rte_eal_alarm_set(GVE_DEV_POLL_INTERVAL_US, - gve_check_device_status, - dev); - + gve_set_dev_status_poller_ok(priv); + ret = rte_thread_create_internal_control(&priv->dev_status_thread, + "gve-dev-status", + gve_check_device_status, dev); if (ret != 0) { + gve_clear_dev_status_poller_ok(priv); PMD_DRV_LOG(ERR, "Port %u: Failed to arm device reset polling alarm! Err=%d", dev->data->port_id, ret); @@ -1842,8 +1837,12 @@ gve_start_dev_status_polling(struct rte_eth_dev *dev) static void gve_stop_dev_status_polling(struct rte_eth_dev *dev) { + struct gve_priv *priv = dev->data->dev_private; + /* Blocks until all in-progress callbacks have completed. */ - rte_eal_alarm_cancel(gve_check_device_status, dev); + gve_clear_dev_status_poller_ok(priv); + + rte_thread_join(priv->dev_status_thread, NULL); } static int @@ -1859,9 +1858,11 @@ gve_dev_reset(struct rte_eth_dev *dev) return -EPERM; } + gve_stop_dev_status_polling(dev); pthread_mutex_lock(&priv->reset_lock); ret = gve_internal_recover_device(dev); pthread_mutex_unlock(&priv->reset_lock); + gve_start_dev_status_polling(dev); return ret; } @@ -2178,6 +2179,8 @@ gve_dev_init(struct rte_eth_dev *eth_dev) eth_dev->data->mac_addrs = &priv->dev_addr; + gve_start_dev_status_polling(eth_dev); + return 0; free_process_priv: diff --git a/drivers/net/gve/gve_ethdev.h b/drivers/net/gve/gve_ethdev.h index 495b60c11..560a9501b 100644 --- a/drivers/net/gve/gve_ethdev.h +++ b/drivers/net/gve/gve_ethdev.h @@ -66,7 +66,7 @@ RTE_ETH_RSS_NONFRAG_IPV6_UDP | \ RTE_ETH_RSS_IPV6_UDP_EX) -#define GVE_DEV_POLL_INTERVAL_US (1 * 1000 * 1000) /* 1 second in microseconds */ +#define GVE_DEV_POLL_INTERVAL_US (100 * 1000) /* 100ms in microseconds */ /* A list of pages registered with the device during setup and used by a queue * as buffers @@ -386,6 +386,7 @@ struct gve_priv { struct gve_tx_queue **zombie_tx_queues; rte_spinlock_t zombie_lock; + rte_thread_t dev_status_thread; pthread_mutex_t reset_lock; struct gve_registers __iomem *reg_bar0; /* see gve_register.h */ @@ -581,6 +582,33 @@ gve_clear_flow_subsystem_ok(struct gve_priv *priv) &priv->state_flags); } +static inline bool +gve_get_dev_status_poller_ok(struct gve_priv *priv) +{ + bool ret; + + ret = !!rte_bit_relaxed_get32(GVE_PRIV_FLAGS_STATUS_POLLER_OK, + &priv->state_flags); + rte_atomic_thread_fence(rte_memory_order_acquire); + return ret; +} + +static inline void +gve_set_dev_status_poller_ok(struct gve_priv *priv) +{ + rte_atomic_thread_fence(rte_memory_order_release); + rte_bit_relaxed_set32(GVE_PRIV_FLAGS_STATUS_POLLER_OK, + &priv->state_flags); +} + +static inline void +gve_clear_dev_status_poller_ok(struct gve_priv *priv) +{ + rte_atomic_thread_fence(rte_memory_order_release); + rte_bit_relaxed_clear32(GVE_PRIV_FLAGS_STATUS_POLLER_OK, + &priv->state_flags); +} + int gve_rx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_id, uint16_t nb_desc, unsigned int socket_id, const struct rte_eth_rxconf *conf, -- 2.56.0.307.g5fe7bd680c-goog
