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 v6] net/ixgbe: fix resource leak after thread exits normally
Date: Wed, 22 Apr 2020 13:18:14 +0800	[thread overview]
Message-ID: <20200422051814.GD4538@intel.com> (raw)
In-Reply-To: <20200422123736.18234-1-taox.zhu@intel.com>

On 04/22, 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 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 <taox.zhu@intel.com>
>Acked-by: Konstantin Ananyev <konstantin.ananyev@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.ye@intel.com>

Applied to dpdk-next-net-intel, Thanks.

      reply	other threads:[~2020-04-22  5:22 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
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 [this message]

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=20200422051814.GD4538@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).