Re: [dpdk-dev] [PATCH v4] net/ixgbe: fix resource leak after thread exits normally

[Ye Xiaolong <xiaolong.ye@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

>+		}
> 	}
> }
>