[PATCH] net/txgbe: fix blocking system events

[PATCH] net/txgbe: fix blocking system events

[Jiawen Wu]

Refer to commit 819d0d1d57f1 ("net/ixgbe: fix blocking system events").
Fix the same issue as ixgbe.

Replace rte alarm thread with an independent thread.

Fixes: 0c061eadec59 ("net/txgbe: add link status change")
Cc: stable@dpdk.org

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 drivers/net/txgbe/txgbe_ethdev.c    | 80 ++++++++++++++++++++++++++---
 drivers/net/txgbe/txgbe_ethdev.h    |  6 +++
 drivers/net/txgbe/txgbe_ethdev_vf.c |  6 ++-
 3 files changed, 84 insertions(+), 8 deletions(-)

diff --git a/drivers/net/txgbe/txgbe_ethdev.c b/drivers/net/txgbe/txgbe_ethdev.c
index 74765a469d..43970a288e 100644
--- a/drivers/net/txgbe/txgbe_ethdev.c
+++ b/drivers/net/txgbe/txgbe_ethdev.c
@@ -546,6 +546,7 @@ txgbe_parse_devargs(struct txgbe_hw *hw, struct rte_devargs *devargs)
 static int
 eth_txgbe_dev_init(struct rte_eth_dev *eth_dev, void *init_params __rte_unused)
 {
+	struct txgbe_adapter *ad = eth_dev->data->dev_private;
 	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev);
 	struct txgbe_hw *hw = TXGBE_DEV_HW(eth_dev);
 	struct txgbe_vfta *shadow_vfta = TXGBE_DEV_VFTA(eth_dev);
@@ -594,6 +595,7 @@ eth_txgbe_dev_init(struct rte_eth_dev *eth_dev, void *init_params __rte_unused)
 		return 0;
 	}
 
+	__atomic_clear(&ad->link_thread_running, __ATOMIC_SEQ_CST);
 	rte_eth_copy_pci_info(eth_dev, pci_dev);
 
 	hw->hw_addr = (void *)pci_dev->mem_resource[0].addr;
@@ -1680,7 +1682,7 @@ txgbe_dev_start(struct rte_eth_dev *dev)
 
 	/* Stop the link setup handler before resetting the HW. */
 	rte_eal_alarm_cancel(txgbe_dev_detect_sfp, dev);
-	rte_eal_alarm_cancel(txgbe_dev_setup_link_alarm_handler, dev);
+	txgbe_dev_wait_setup_link_complete(dev, 0);
 
 	/* disable uio/vfio intr/eventfd mapping */
 	rte_intr_disable(intr_handle);
@@ -1919,7 +1921,7 @@ txgbe_dev_stop(struct rte_eth_dev *dev)
 	PMD_INIT_FUNC_TRACE();
 
 	rte_eal_alarm_cancel(txgbe_dev_detect_sfp, dev);
-	rte_eal_alarm_cancel(txgbe_dev_setup_link_alarm_handler, dev);
+	txgbe_dev_wait_setup_link_complete(dev, 0);
 
 	/* disable interrupts */
 	txgbe_disable_intr(hw);
@@ -2803,11 +2805,62 @@ txgbe_dev_setup_link_alarm_handler(void *param)
 	intr->flags &= ~TXGBE_FLAG_NEED_LINK_CONFIG;
 }
 
+/*
+ * If @timeout_ms was 0, it means that it will not return until link complete.
+ * It returns 1 on complete, return 0 on timeout.
+ */
+int
+txgbe_dev_wait_setup_link_complete(struct rte_eth_dev *dev, uint32_t timeout_ms)
+{
+#define WARNING_TIMEOUT    9000 /* 9s in total */
+	struct txgbe_adapter *ad = TXGBE_DEV_ADAPTER(dev);
+	uint32_t timeout = timeout_ms ? timeout_ms : WARNING_TIMEOUT;
+
+	while (__atomic_load_n(&ad->link_thread_running, __ATOMIC_SEQ_CST)) {
+		msec_delay(1);
+		timeout--;
+
+		if (timeout_ms) {
+			if (!timeout)
+				return 0;
+		} else if (!timeout) {
+			/* It will not return until link complete */
+			timeout = WARNING_TIMEOUT;
+			PMD_DRV_LOG(ERR, "TXGBE link thread not complete too long time!");
+		}
+	}
+
+	return 1;
+}
+
+static uint32_t
+txgbe_dev_setup_link_thread_handler(void *param)
+{
+	struct rte_eth_dev *dev = (struct rte_eth_dev *)param;
+	struct txgbe_hw *hw = TXGBE_DEV_HW(dev);
+	struct txgbe_interrupt *intr = TXGBE_DEV_INTR(dev);
+	struct txgbe_adapter *ad = TXGBE_DEV_ADAPTER(dev);
+	u32 speed;
+	bool autoneg = false;
+
+	rte_thread_detach(rte_thread_self());
+	speed = hw->phy.autoneg_advertised;
+	if (!speed)
+		hw->mac.get_link_capabilities(hw, &speed, &autoneg);
+
+	hw->mac.setup_link(hw, speed, true);
+
+	intr->flags &= ~TXGBE_FLAG_NEED_LINK_CONFIG;
+	__atomic_clear(&ad->link_thread_running, __ATOMIC_SEQ_CST);
+	return 0;
+}
+
 /* return 0 means link status changed, -1 means not changed */
 int
 txgbe_dev_link_update_share(struct rte_eth_dev *dev,
 			    int wait_to_complete)
 {
+	struct txgbe_adapter *ad = TXGBE_DEV_ADAPTER(dev);
 	struct txgbe_hw *hw = TXGBE_DEV_HW(dev);
 	struct rte_eth_link link;
 	u32 link_speed = TXGBE_LINK_SPEED_UNKNOWN;
@@ -2844,10 +2897,25 @@ txgbe_dev_link_update_share(struct rte_eth_dev *dev,
 		if ((hw->subsystem_device_id & 0xFF) ==
 				TXGBE_DEV_ID_KR_KX_KX4) {
 			hw->mac.bp_down_event(hw);
-		} else if (hw->phy.media_type == txgbe_media_type_fiber) {
-			intr->flags |= TXGBE_FLAG_NEED_LINK_CONFIG;
-			rte_eal_alarm_set(10,
-				txgbe_dev_setup_link_alarm_handler, dev);
+		} else if (hw->phy.media_type == txgbe_media_type_fiber &&
+				dev->data->dev_conf.intr_conf.lsc != 0) {
+			txgbe_dev_wait_setup_link_complete(dev, 0);
+			if (!__atomic_test_and_set(&ad->link_thread_running, __ATOMIC_SEQ_CST)) {
+				/* To avoid race condition between threads, set
+				 * the TXGBE_FLAG_NEED_LINK_CONFIG flag only
+				 * when there is no link thread running.
+				 */
+				intr->flags |= TXGBE_FLAG_NEED_LINK_CONFIG;
+				if (rte_thread_create_control(&ad->link_thread_tid,
+					"txgbe-link-thread", NULL,
+					txgbe_dev_setup_link_thread_handler, dev) < 0) {
+					PMD_DRV_LOG(ERR, "Create link thread failed!");
+					__atomic_clear(&ad->link_thread_running, __ATOMIC_SEQ_CST);
+				}
+			} else {
+				PMD_DRV_LOG(ERR,
+					"Other link thread is running now!");
+			}
 		}
 		return rte_eth_linkstatus_set(dev, &link);
 	} else if (!hw->dev_start) {
diff --git a/drivers/net/txgbe/txgbe_ethdev.h b/drivers/net/txgbe/txgbe_ethdev.h
index c59b6370bb..6b296d6fd1 100644
--- a/drivers/net/txgbe/txgbe_ethdev.h
+++ b/drivers/net/txgbe/txgbe_ethdev.h
@@ -370,6 +370,9 @@ struct txgbe_adapter {
 
 	/* For RSS reta table update */
 	uint8_t rss_reta_updated;
+
+	uint32_t link_thread_running;
+	rte_thread_t link_thread_tid;
 };
 
 #define TXGBE_DEV_ADAPTER(dev) \
@@ -561,6 +564,9 @@ void txgbe_configure_dcb(struct rte_eth_dev *dev);
 int
 txgbe_dev_link_update_share(struct rte_eth_dev *dev,
 		int wait_to_complete);
+int
+txgbe_dev_wait_setup_link_complete(struct rte_eth_dev *dev,
+		uint32_t timeout_ms);
 int txgbe_pf_host_init(struct rte_eth_dev *eth_dev);
 
 void txgbe_pf_host_uninit(struct rte_eth_dev *eth_dev);
diff --git a/drivers/net/txgbe/txgbe_ethdev_vf.c b/drivers/net/txgbe/txgbe_ethdev_vf.c
index 3b1f7c913b..f1341fbf7e 100644
--- a/drivers/net/txgbe/txgbe_ethdev_vf.c
+++ b/drivers/net/txgbe/txgbe_ethdev_vf.c
@@ -165,6 +165,7 @@ eth_txgbevf_dev_init(struct rte_eth_dev *eth_dev)
 {
 	int err;
 	uint32_t tc, tcs;
+	struct txgbe_adapter *ad = eth_dev->data->dev_private;
 	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev);
 	struct rte_intr_handle *intr_handle = pci_dev->intr_handle;
 	struct txgbe_hw *hw = TXGBE_DEV_HW(eth_dev);
@@ -205,6 +206,7 @@ eth_txgbevf_dev_init(struct rte_eth_dev *eth_dev)
 		return 0;
 	}
 
+	__atomic_clear(&ad->link_thread_running, __ATOMIC_SEQ_CST);
 	rte_eth_copy_pci_info(eth_dev, pci_dev);
 
 	hw->device_id = pci_dev->id.device_id;
@@ -618,7 +620,7 @@ txgbevf_dev_start(struct rte_eth_dev *dev)
 	PMD_INIT_FUNC_TRACE();
 
 	/* Stop the link setup handler before resetting the HW. */
-	rte_eal_alarm_cancel(txgbe_dev_setup_link_alarm_handler, dev);
+	txgbe_dev_wait_setup_link_complete(dev, 0);
 
 	err = hw->mac.reset_hw(hw);
 	if (err) {
@@ -720,7 +722,7 @@ txgbevf_dev_stop(struct rte_eth_dev *dev)
 
 	PMD_INIT_FUNC_TRACE();
 
-	rte_eal_alarm_cancel(txgbe_dev_setup_link_alarm_handler, dev);
+	txgbe_dev_wait_setup_link_complete(dev, 0);
 
 	txgbevf_intr_disable(dev);
 
-- 
2.27.0

Re: [PATCH] net/txgbe: fix blocking system events

[Ferruh Yigit]

On 6/29/2023 11:07 AM, Jiawen Wu wrote:
> Refer to commit 819d0d1d57f1 ("net/ixgbe: fix blocking system events").
> Fix the same issue as ixgbe.
> 

Can you please describe the problem in this commit log, it is OK to keep
reference to the ixgbe too, but better to describe problem here.

> Replace rte alarm thread with an independent thread.
> 
> Fixes: 0c061eadec59 ("net/txgbe: add link status change")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>

<...>

> +static uint32_t
> +txgbe_dev_setup_link_thread_handler(void *param)
> +{
> +	struct rte_eth_dev *dev = (struct rte_eth_dev *)param;
> +	struct txgbe_hw *hw = TXGBE_DEV_HW(dev);
> +	struct txgbe_interrupt *intr = TXGBE_DEV_INTR(dev);
> +	struct txgbe_adapter *ad = TXGBE_DEV_ADAPTER(dev);
> +	u32 speed;
> +	bool autoneg = false;
> +
> +	rte_thread_detach(rte_thread_self());
> +	speed = hw->phy.autoneg_advertised;
> +	if (!speed)
> +		hw->mac.get_link_capabilities(hw, &speed, &autoneg);
> +
> +	hw->mac.setup_link(hw, speed, true);
> +
> +	intr->flags &= ~TXGBE_FLAG_NEED_LINK_CONFIG;
> +	__atomic_clear(&ad->link_thread_running, __ATOMIC_SEQ_CST);
> +	return 0;
> +}
> +

'txgbe_dev_setup_link_alarm_handler()' and above new
'txgbe_dev_setup_link_thread_handler()' are smilar and share common
code, what do you think to extract common code to a function?

[PATCH v2] net/txgbe: fix blocking system events

[Jiawen Wu]

Refer to commit 819d0d1d57f1 ("net/ixgbe: fix blocking system events").
Fix the same issue as ixgbe.

TXGBE link status task uses rte alarm thread in old implementation.
Sometime txgbe link status task takes up to 9 seconds. This will
severely affect the rte-alarm-thread dependent tasks in the
system, like interrupt or hotplug event. So replace with an
independent thread which has the same thread affinity settings
as rte interrupt.

Fixes: 0c061eadec59 ("net/txgbe: add link status change")
Cc: stable@dpdk.org

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 drivers/net/txgbe/txgbe_ethdev.c    | 70 ++++++++++++++++++++++++++---
 drivers/net/txgbe/txgbe_ethdev.h    |  6 +++
 drivers/net/txgbe/txgbe_ethdev_vf.c |  6 ++-
 3 files changed, 74 insertions(+), 8 deletions(-)

diff --git a/drivers/net/txgbe/txgbe_ethdev.c b/drivers/net/txgbe/txgbe_ethdev.c
index 74765a469d..d942b542ea 100644
--- a/drivers/net/txgbe/txgbe_ethdev.c
+++ b/drivers/net/txgbe/txgbe_ethdev.c
@@ -546,6 +546,7 @@ txgbe_parse_devargs(struct txgbe_hw *hw, struct rte_devargs *devargs)
 static int
 eth_txgbe_dev_init(struct rte_eth_dev *eth_dev, void *init_params __rte_unused)
 {
+	struct txgbe_adapter *ad = eth_dev->data->dev_private;
 	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev);
 	struct txgbe_hw *hw = TXGBE_DEV_HW(eth_dev);
 	struct txgbe_vfta *shadow_vfta = TXGBE_DEV_VFTA(eth_dev);
@@ -594,6 +595,7 @@ eth_txgbe_dev_init(struct rte_eth_dev *eth_dev, void *init_params __rte_unused)
 		return 0;
 	}
 
+	__atomic_clear(&ad->link_thread_running, __ATOMIC_SEQ_CST);
 	rte_eth_copy_pci_info(eth_dev, pci_dev);
 
 	hw->hw_addr = (void *)pci_dev->mem_resource[0].addr;
@@ -1680,7 +1682,7 @@ txgbe_dev_start(struct rte_eth_dev *dev)
 
 	/* Stop the link setup handler before resetting the HW. */
 	rte_eal_alarm_cancel(txgbe_dev_detect_sfp, dev);
-	rte_eal_alarm_cancel(txgbe_dev_setup_link_alarm_handler, dev);
+	txgbe_dev_wait_setup_link_complete(dev, 0);
 
 	/* disable uio/vfio intr/eventfd mapping */
 	rte_intr_disable(intr_handle);
@@ -1919,7 +1921,7 @@ txgbe_dev_stop(struct rte_eth_dev *dev)
 	PMD_INIT_FUNC_TRACE();
 
 	rte_eal_alarm_cancel(txgbe_dev_detect_sfp, dev);
-	rte_eal_alarm_cancel(txgbe_dev_setup_link_alarm_handler, dev);
+	txgbe_dev_wait_setup_link_complete(dev, 0);
 
 	/* disable interrupts */
 	txgbe_disable_intr(hw);
@@ -2803,11 +2805,52 @@ txgbe_dev_setup_link_alarm_handler(void *param)
 	intr->flags &= ~TXGBE_FLAG_NEED_LINK_CONFIG;
 }
 
+/*
+ * If @timeout_ms was 0, it means that it will not return until link complete.
+ * It returns 1 on complete, return 0 on timeout.
+ */
+int
+txgbe_dev_wait_setup_link_complete(struct rte_eth_dev *dev, uint32_t timeout_ms)
+{
+#define WARNING_TIMEOUT    9000 /* 9s in total */
+	struct txgbe_adapter *ad = TXGBE_DEV_ADAPTER(dev);
+	uint32_t timeout = timeout_ms ? timeout_ms : WARNING_TIMEOUT;
+
+	while (__atomic_load_n(&ad->link_thread_running, __ATOMIC_SEQ_CST)) {
+		msec_delay(1);
+		timeout--;
+
+		if (timeout_ms) {
+			if (!timeout)
+				return 0;
+		} else if (!timeout) {
+			/* It will not return until link complete */
+			timeout = WARNING_TIMEOUT;
+			PMD_DRV_LOG(ERR, "TXGBE link thread not complete too long time!");
+		}
+	}
+
+	return 1;
+}
+
+static uint32_t
+txgbe_dev_setup_link_thread_handler(void *param)
+{
+	struct rte_eth_dev *dev = (struct rte_eth_dev *)param;
+	struct txgbe_adapter *ad = TXGBE_DEV_ADAPTER(dev);
+
+	rte_thread_detach(rte_thread_self());
+	txgbe_dev_setup_link_alarm_handler(dev);
+	__atomic_clear(&ad->link_thread_running, __ATOMIC_SEQ_CST);
+	return 0;
+}
+
 /* return 0 means link status changed, -1 means not changed */
 int
 txgbe_dev_link_update_share(struct rte_eth_dev *dev,
 			    int wait_to_complete)
 {
+	struct txgbe_adapter *ad = TXGBE_DEV_ADAPTER(dev);
 	struct txgbe_hw *hw = TXGBE_DEV_HW(dev);
 	struct rte_eth_link link;
 	u32 link_speed = TXGBE_LINK_SPEED_UNKNOWN;
@@ -2844,10 +2887,25 @@ txgbe_dev_link_update_share(struct rte_eth_dev *dev,
 		if ((hw->subsystem_device_id & 0xFF) ==
 				TXGBE_DEV_ID_KR_KX_KX4) {
 			hw->mac.bp_down_event(hw);
-		} else if (hw->phy.media_type == txgbe_media_type_fiber) {
-			intr->flags |= TXGBE_FLAG_NEED_LINK_CONFIG;
-			rte_eal_alarm_set(10,
-				txgbe_dev_setup_link_alarm_handler, dev);
+		} else if (hw->phy.media_type == txgbe_media_type_fiber &&
+				dev->data->dev_conf.intr_conf.lsc != 0) {
+			txgbe_dev_wait_setup_link_complete(dev, 0);
+			if (!__atomic_test_and_set(&ad->link_thread_running, __ATOMIC_SEQ_CST)) {
+				/* To avoid race condition between threads, set
+				 * the TXGBE_FLAG_NEED_LINK_CONFIG flag only
+				 * when there is no link thread running.
+				 */
+				intr->flags |= TXGBE_FLAG_NEED_LINK_CONFIG;
+				if (rte_thread_create_control(&ad->link_thread_tid,
+					"txgbe-link-thread", NULL,
+					txgbe_dev_setup_link_thread_handler, dev) < 0) {
+					PMD_DRV_LOG(ERR, "Create link thread failed!");
+					__atomic_clear(&ad->link_thread_running, __ATOMIC_SEQ_CST);
+				}
+			} else {
+				PMD_DRV_LOG(ERR,
+					"Other link thread is running now!");
+			}
 		}
 		return rte_eth_linkstatus_set(dev, &link);
 	} else if (!hw->dev_start) {
diff --git a/drivers/net/txgbe/txgbe_ethdev.h b/drivers/net/txgbe/txgbe_ethdev.h
index c59b6370bb..6b296d6fd1 100644
--- a/drivers/net/txgbe/txgbe_ethdev.h
+++ b/drivers/net/txgbe/txgbe_ethdev.h
@@ -370,6 +370,9 @@ struct txgbe_adapter {
 
 	/* For RSS reta table update */
 	uint8_t rss_reta_updated;
+
+	uint32_t link_thread_running;
+	rte_thread_t link_thread_tid;
 };
 
 #define TXGBE_DEV_ADAPTER(dev) \
@@ -561,6 +564,9 @@ void txgbe_configure_dcb(struct rte_eth_dev *dev);
 int
 txgbe_dev_link_update_share(struct rte_eth_dev *dev,
 		int wait_to_complete);
+int
+txgbe_dev_wait_setup_link_complete(struct rte_eth_dev *dev,
+		uint32_t timeout_ms);
 int txgbe_pf_host_init(struct rte_eth_dev *eth_dev);
 
 void txgbe_pf_host_uninit(struct rte_eth_dev *eth_dev);
diff --git a/drivers/net/txgbe/txgbe_ethdev_vf.c b/drivers/net/txgbe/txgbe_ethdev_vf.c
index 3b1f7c913b..f1341fbf7e 100644
--- a/drivers/net/txgbe/txgbe_ethdev_vf.c
+++ b/drivers/net/txgbe/txgbe_ethdev_vf.c
@@ -165,6 +165,7 @@ eth_txgbevf_dev_init(struct rte_eth_dev *eth_dev)
 {
 	int err;
 	uint32_t tc, tcs;
+	struct txgbe_adapter *ad = eth_dev->data->dev_private;
 	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev);
 	struct rte_intr_handle *intr_handle = pci_dev->intr_handle;
 	struct txgbe_hw *hw = TXGBE_DEV_HW(eth_dev);
@@ -205,6 +206,7 @@ eth_txgbevf_dev_init(struct rte_eth_dev *eth_dev)
 		return 0;
 	}
 
+	__atomic_clear(&ad->link_thread_running, __ATOMIC_SEQ_CST);
 	rte_eth_copy_pci_info(eth_dev, pci_dev);
 
 	hw->device_id = pci_dev->id.device_id;
@@ -618,7 +620,7 @@ txgbevf_dev_start(struct rte_eth_dev *dev)
 	PMD_INIT_FUNC_TRACE();
 
 	/* Stop the link setup handler before resetting the HW. */
-	rte_eal_alarm_cancel(txgbe_dev_setup_link_alarm_handler, dev);
+	txgbe_dev_wait_setup_link_complete(dev, 0);
 
 	err = hw->mac.reset_hw(hw);
 	if (err) {
@@ -720,7 +722,7 @@ txgbevf_dev_stop(struct rte_eth_dev *dev)
 
 	PMD_INIT_FUNC_TRACE();
 
-	rte_eal_alarm_cancel(txgbe_dev_setup_link_alarm_handler, dev);
+	txgbe_dev_wait_setup_link_complete(dev, 0);
 
 	txgbevf_intr_disable(dev);
 
-- 
2.27.0

Re: [PATCH v2] net/txgbe: fix blocking system events

[Ferruh Yigit]

On 6/30/2023 10:35 AM, Jiawen Wu wrote:
> Refer to commit 819d0d1d57f1 ("net/ixgbe: fix blocking system events").
> Fix the same issue as ixgbe.
> 
> TXGBE link status task uses rte alarm thread in old implementation.
> Sometime txgbe link status task takes up to 9 seconds. This will
> severely affect the rte-alarm-thread dependent tasks in the
> system, like interrupt or hotplug event. So replace with an
> independent thread which has the same thread affinity settings
> as rte interrupt.
> 
> Fixes: 0c061eadec59 ("net/txgbe: add link status change")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
>

Applied to dpdk-next-net/main, thanks.