From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 31404A05A1; Wed, 22 Apr 2020 04:37:36 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 3C6A81D40E; Wed, 22 Apr 2020 04:37:35 +0200 (CEST) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 69B9E1D155; Wed, 22 Apr 2020 04:37:33 +0200 (CEST) IronPort-SDR: +CFOch0XAkeHVqcEkYHPwfT2LK5yVk6giyRILiP8AY0ccNKeVQXQuMmscBqFj9YoE996Vk1+z7 Pu4PCn9LsxaA== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 21 Apr 2020 19:37:32 -0700 IronPort-SDR: G6roIEfMa15YY0/hjGBZRZHEkp15L2rRfwwg3aJDVoDse5dZTPsZfR4dZcs4HPtK8GFAFWVNJv GKylLm6qqZbQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.72,412,1580803200"; d="scan'208";a="273729746" Received: from yexl-server.sh.intel.com (HELO localhost) ([10.67.117.17]) by orsmga002.jf.intel.com with ESMTP; 21 Apr 2020 19:37:30 -0700 Date: Wed, 22 Apr 2020 10:33:08 +0800 From: Ye Xiaolong To: taox.zhu@intel.com Cc: konstantin.ananyev@intel.com, wenzhuo.lu@intel.com, dev@dpdk.org, martin.weiser@allegro-packets.com, stable@dpdk.org Message-ID: <20200422023308.GC4538@intel.com> References: <1586495895-9610-1-git-send-email-taox.zhu@intel.com> <20200421185320.16946-1-taox.zhu@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200421185320.16946-1-taox.zhu@intel.com> User-Agent: Mutt/1.9.4 (2018-02-28) Subject: Re: [dpdk-dev] [PATCH v5] net/ixgbe: fix resource leak after thread exits normally X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 04/21, taox.zhu@intel.com wrote: >From: Zhu Tao > >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: stable@dpdk.org > >Signed-off-by: Zhu Tao >--- > drivers/net/ixgbe/ixgbe_ethdev.c | 57 +++++++++++++++++++--------------------- > 1 file changed, 27 insertions(+), 30 deletions(-) > >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..92788ee 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; >+ int timeout = timeout_ms ? WARNING_TIMEOUT : timeout_ms; Sees timeout_ms is 0, set timeout as 0? I guess what you intended to do is: int timeout = timeout_ms ? timeout_ms : WARNING_TIMEOUT; Thanks, Xiaolong > >- 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 >