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 8BB21A0561; Mon, 20 Apr 2020 10:54:05 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id F22FE1D509; Mon, 20 Apr 2020 10:54:04 +0200 (CEST) Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id D27231C236; Mon, 20 Apr 2020 10:54:03 +0200 (CEST) IronPort-SDR: OdAf3RVhTpDhvdXLDMhjfcLla2cpXQkLgeuqvhVmLGnyDnzpIEOdBWtlkur1QlCn/aSBFGpCv8 HClutBATdPGA== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 20 Apr 2020 01:54:02 -0700 IronPort-SDR: og6hvF3QNQxYDr+zZ0dABcNbUlBCIi3lrGqEWukTk+tYsdkFGVOGzVmNRm/38SbOiJr9AEH+50 Fbtn/qfT/ihA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.72,406,1580803200"; d="scan'208";a="401762089" Received: from yexl-server.sh.intel.com (HELO localhost) ([10.67.117.17]) by orsmga004.jf.intel.com with ESMTP; 20 Apr 2020 01:54:01 -0700 Date: Mon, 20 Apr 2020 16:49:43 +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: <20200420084943.GE33101@intel.com> References: <1586495895-9610-1-git-send-email-taox.zhu@intel.com> <20200415141850.33883-1-taox.zhu@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200415141850.33883-1-taox.zhu@intel.com> User-Agent: Mutt/1.9.4 (2018-02-28) Subject: Re: [dpdk-dev] [PATCH v4] 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/15, 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 without setting the timeout, 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 | 46 +++++++++++++++------------------------- > 1 file changed, 17 insertions(+), 29 deletions(-) >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..9ad93c5 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 void ixgbe_dev_wait_setup_link_complete(struct rte_eth_dev *dev); > > 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); > > /* 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); > > /* disable interrupts */ > ixgbe_disable_intr(hw); >@@ -4143,35 +4143,22 @@ 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*/ >-static int >+static void > ixgbe_dev_wait_setup_link_complete(struct rte_eth_dev *dev) > { >-#define DELAY_INTERVAL 100 /* 100ms */ >-#define MAX_TIMEOUT 90 /* 9s (90 * 100ms) in total */ >+#define DELAY_INTERVAL 10 /* 10ms */ >+#define WARNING_TIMEOUT 900 /* 9s (900 * 10ms) in total */ > struct ixgbe_adapter *ad = dev->data->dev_private; >- int timeout = MAX_TIMEOUT; >+ int timeout = WARNING_TIMEOUT; > >- while (rte_atomic32_read(&ad->link_thread_running) && timeout) { >+ while (rte_atomic32_read(&ad->link_thread_running)) { > msec_delay(DELAY_INTERVAL); > 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) { >+ timeout = WARNING_TIMEOUT; >+ PMD_DRV_LOG(ERR, "IXGBE link thread not complete too long time!"); In theory, is there any possibility that the while loop will never end? And as Konstantin suggested, I think it makes sense to provide a parameter for caller to decide how long they want to wait. Thanks, Xiaolong >+ } > } > } >