DPDK patches and discussions
 help / color / mirror / Atom feed
From: Ye Xiaolong <xiaolong.ye@intel.com>
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
Subject: Re: [dpdk-dev] [PATCH v4] net/ixgbe: fix resource leak after thread exits normally
Date: Mon, 20 Apr 2020 16:49:43 +0800	[thread overview]
Message-ID: <20200420084943.GE33101@intel.com> (raw)
In-Reply-To: <20200415141850.33883-1-taox.zhu@intel.com>

On 04/15, taox.zhu@intel.com wrote:
>From: Zhu Tao <taox.zhu@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 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 <taox.zhu@intel.com>
>---
> 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

>+		}
> 	}
> }
> 

  reply	other threads:[~2020-04-20  8:54 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-10  5:18 [dpdk-dev] [PATCH] " taox.zhu
2020-04-10  6:22 ` [dpdk-dev] [PATCH v2] " taox.zhu
2020-04-10 12:09   ` Ananyev, Konstantin
     [not found]     ` <60652C6914E08D41B9AA1580751B3CA9015ED8D2@SHSMSX103.ccr.corp.intel.com>
2020-04-14 10:25       ` Ananyev, Konstantin
2020-04-14 11:26         ` Ananyev, Konstantin
2020-04-15 12:27 ` [dpdk-dev] [PATCH v3] " taox.zhu
2020-04-15 14:18 ` [dpdk-dev] [PATCH v4] " taox.zhu
2020-04-20  8:49   ` Ye Xiaolong [this message]
2020-04-21 18:53 ` [dpdk-dev] [PATCH v5] " taox.zhu
2020-04-21 22:37   ` Ananyev, Konstantin
2020-04-22  2:33   ` Ye Xiaolong
2020-04-22 12:37 ` [dpdk-dev] [PATCH v6] " taox.zhu
2020-04-22  5:18   ` Ye Xiaolong

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200420084943.GE33101@intel.com \
    --to=xiaolong.ye@intel.com \
    --cc=dev@dpdk.org \
    --cc=konstantin.ananyev@intel.com \
    --cc=martin.weiser@allegro-packets.com \
    --cc=stable@dpdk.org \
    --cc=taox.zhu@intel.com \
    --cc=wenzhuo.lu@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).