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 BDD99A00C2; Wed, 22 Apr 2020 07:22:41 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 0C2C21C437; Wed, 22 Apr 2020 07:22:41 +0200 (CEST) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 3212B1C2EC; Wed, 22 Apr 2020 07:22:39 +0200 (CEST) IronPort-SDR: 6d7wOSrHvz5k8LwVx0It5wFmvhk5612DL31Y/Xi+t/oNQM6ERjsJowPZn/5phDFN/Ynz0hXwQL T3WVyI5zdY8w== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 21 Apr 2020 22:22:38 -0700 IronPort-SDR: o6ckTOYL98tnxeYHt7+UIEQMQju6XvdZ+pkvVErSIozfCvrho5t4dBIf+gumemxiQyjJbsic2p RijfdJKwZxcg== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.72,412,1580803200"; d="scan'208";a="255523354" Received: from yexl-server.sh.intel.com (HELO localhost) ([10.67.117.17]) by orsmga003.jf.intel.com with ESMTP; 21 Apr 2020 22:22:36 -0700 Date: Wed, 22 Apr 2020 13:18:14 +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: <20200422051814.GD4538@intel.com> References: <1586495895-9610-1-git-send-email-taox.zhu@intel.com> <20200422123736.18234-1-taox.zhu@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200422123736.18234-1-taox.zhu@intel.com> User-Agent: Mutt/1.9.4 (2018-02-28) Subject: Re: [dpdk-dev] [PATCH v6] 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/22, 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 >Acked-by: Konstantin Ananyev >--- > 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 Applied to dpdk-next-net-intel, Thanks.