On 04/15, taox....@intel.com wrote:
>From: Zhu Tao <taox....@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: sta...@dpdk.org
>
>Signed-off-by: Zhu Tao <taox....@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
>+ }
> }
> }
>
