On 04/22, taox....@intel.com wrote:
>From: Zhu Tao <taox....@intel.com>
>
>When the thread exits normally, pthread_join() is not called, which can
>result in a resource leak. Therefore, the thread is set to separation
>mode using function pthread_detach(), so that no program call
>pthread_join() is required to recycle, and when the thread exits,
>the system automatically reclaims resources.
>
>Wait for the thread to finish with timeout argument(0 means that it will
>not return until link complete), wait until the thread finishes before
>returning. Normally, the thread will finish in a shorter time, and give
>a warning message if it hasn't finished in a longer time.
>
>Fixes: 819d0d1d57f1 ("net/ixgbe: fix blocking system events")
>Cc: sta...@dpdk.org
>
>Signed-off-by: Zhu Tao <taox....@intel.com>
>Acked-by: Konstantin Ananyev <konstantin.anan...@intel.com>
>---
> drivers/net/ixgbe/ixgbe_ethdev.c | 57 +++++++++++++++++++---------------------
> 1 file changed, 27 insertions(+), 30 deletions(-)
>
>V6 changes:
> Change code segment
> 'int timeout = timeout_ms ? WARNING_TIMEOUT : timeout_ms;'
> to
> 'uint32_t timeout = timeout_ms ? timeout_ms :
> WARNING_TIMEOUT;'.
>
>V5 changes:
> Modify the definition of function ixgbe_dev_wait_setup_link_complete(),
> so that the caller can choose whether to block, etc.
>
>v4 changes:
> Format codes.
>
>v3 changes:
> 1. Wait for the thread to finish without setting the timeout, and the
> corresponding function name has also been modified.
> 2. Commit log.
>
>v2 changes:
> Commit log.
>
>diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
>b/drivers/net/ixgbe/ixgbe_ethdev.c
>index 2c57976..a3ae4d7 100644
>--- a/drivers/net/ixgbe/ixgbe_ethdev.c
>+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>@@ -230,7 +230,7 @@ static int ixgbe_dev_rss_reta_query(struct rte_eth_dev
>*dev,
> static void ixgbe_dev_interrupt_handler(void *param);
> static void ixgbe_dev_interrupt_delayed_handler(void *param);
> static void *ixgbe_dev_setup_link_thread_handler(void *param);
>-static void ixgbe_dev_cancel_link_thread(struct rte_eth_dev *dev);
>+static int ixgbe_dev_wait_setup_link_complete(struct rte_eth_dev *dev,
>uint32_t timeout_ms);
>
> static int ixgbe_add_rar(struct rte_eth_dev *dev,
> struct rte_ether_addr *mac_addr,
>@@ -2601,7 +2601,7 @@ static int eth_ixgbevf_pci_remove(struct rte_pci_device
>*pci_dev)
> PMD_INIT_FUNC_TRACE();
>
> /* Stop the link setup handler before resetting the HW. */
>- ixgbe_dev_cancel_link_thread(dev);
>+ ixgbe_dev_wait_setup_link_complete(dev, 0);
>
> /* disable uio/vfio intr/eventfd mapping */
> rte_intr_disable(intr_handle);
>@@ -2888,7 +2888,7 @@ static int eth_ixgbevf_pci_remove(struct rte_pci_device
>*pci_dev)
>
> PMD_INIT_FUNC_TRACE();
>
>- ixgbe_dev_cancel_link_thread(dev);
>+ ixgbe_dev_wait_setup_link_complete(dev, 0);
>
> /* disable interrupts */
> ixgbe_disable_intr(hw);
>@@ -4143,36 +4143,32 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused
>struct rte_eth_dev *dev,
> return ret_val;
> }
>
>-/* return 1: setup complete, return 0: setup not complete, and wait timeout*/
>+/*
>+ * If @timeout_ms was 0, it means that it will not return until link complete.
>+ * It returns 1 on complete, return 0 on timeout.
>+ */
> static int
>-ixgbe_dev_wait_setup_link_complete(struct rte_eth_dev *dev)
>+ixgbe_dev_wait_setup_link_complete(struct rte_eth_dev *dev, uint32_t
>timeout_ms)
> {
>-#define DELAY_INTERVAL 100 /* 100ms */
>-#define MAX_TIMEOUT 90 /* 9s (90 * 100ms) in total */
>+#define WARNING_TIMEOUT 9000 /* 9s in total */
> struct ixgbe_adapter *ad = dev->data->dev_private;
>- int timeout = MAX_TIMEOUT;
>+ uint32_t timeout = timeout_ms ? timeout_ms : WARNING_TIMEOUT;
>
>- while (rte_atomic32_read(&ad->link_thread_running) && timeout) {
>- msec_delay(DELAY_INTERVAL);
>+ while (rte_atomic32_read(&ad->link_thread_running)) {
>+ msec_delay(1);
> timeout--;
>- }
>-
>-
>- return !!timeout;
>-}
>
>-static void
>-ixgbe_dev_cancel_link_thread(struct rte_eth_dev *dev)
>-{
>- struct ixgbe_adapter *ad = dev->data->dev_private;
>- void *retval;
>-
>- if (!ixgbe_dev_wait_setup_link_complete(dev)) {
>- pthread_cancel(ad->link_thread_tid);
>- pthread_join(ad->link_thread_tid, &retval);
>- rte_atomic32_clear(&ad->link_thread_running);
>- PMD_DRV_LOG(ERR, "Link thread not complete, cancel it!");
>+ if (timeout_ms) {
>+ if (!timeout)
>+ return 0;
>+ } else if (!timeout) {
>+ /* It will not return until link complete */
>+ timeout = WARNING_TIMEOUT;
>+ PMD_DRV_LOG(ERR, "IXGBE link thread not complete too
>long time!");
>+ }
> }
>+
>+ return 1;
> }
>
> static void *
>@@ -4186,6 +4182,7 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused
>struct rte_eth_dev *dev,
> u32 speed;
> bool autoneg = false;
>
>+ pthread_detach(pthread_self());
> speed = hw->phy.autoneg_advertised;
> if (!speed)
> ixgbe_get_link_capabilities(hw, &speed, &autoneg);
>@@ -4282,8 +4279,8 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused
>struct rte_eth_dev *dev,
> if (link_up == 0) {
> if (ixgbe_get_media_type(hw) == ixgbe_media_type_fiber) {
> intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG;
>- if (ixgbe_dev_wait_setup_link_complete(dev) &&
>-
>rte_atomic32_test_and_set(&ad->link_thread_running)) {
>+ ixgbe_dev_wait_setup_link_complete(dev, 0);
>+ if
>(rte_atomic32_test_and_set(&ad->link_thread_running)) {
> if (rte_ctrl_thread_create(&ad->link_thread_tid,
> "ixgbe-link-handler",
> NULL,
>@@ -5323,7 +5320,7 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused
>struct rte_eth_dev *dev,
> PMD_INIT_FUNC_TRACE();
>
> /* Stop the link setup handler before resetting the HW. */
>- ixgbe_dev_cancel_link_thread(dev);
>+ ixgbe_dev_wait_setup_link_complete(dev, 0);
>
> err = hw->mac.ops.reset_hw(hw);
> if (err) {
>@@ -5421,7 +5418,7 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused
>struct rte_eth_dev *dev,
>
> PMD_INIT_FUNC_TRACE();
>
>- ixgbe_dev_cancel_link_thread(dev);
>+ ixgbe_dev_wait_setup_link_complete(dev, 0);
>
> ixgbevf_intr_disable(dev);
>
>--
>1.8.3.1
>
Reviewed-by: Xiaolong Ye <xiaolong...@intel.com>
Applied to dpdk-next-net-intel, Thanks.
