> Hi Tao,
>
>
> > Because ixgbe_dev_wait_setup_link_complete() doesn't guarantee that the
> > thread will always complete properly. So after the wait
> > timeout, need to call pthread_cancel() to cancel the thread.
>
> But pthread_cancel() also doesn't guarantee immediate thread termination.
> You'll have to wait for link_thread_running again to make sure.
> Another thing - in theory if link_thread was already terminated,
> then its TID can be reused by other thread and we will terminate different
> thread
> (though chances are quite small).
>
> > Definition of function ixgbe_dev_wait_setup_link_complete() as below:
> >
> > /* return 1: setup complete, return 0: setup not complete, and wait
> > timeout*/ static int ixgbe_dev_wait_setup_link_complete(struct
> > rte_eth_dev *dev) { #define DELAY_INTERVAL 100 /* 100ms */
As another thought - instead of unconditionally defining timeout
inside the function itself, can we have it a s function parameter?
Can caller can decide for how long he is ok to wait.
> > #define MAX_TIMEOUT 90 /* 9s (90 * 100ms) in total */
> > struct ixgbe_adapter *ad = dev->data->dev_private;
> > int timeout = MAX_TIMEOUT;
> >
> > while (rte_atomic32_read(&ad->link_thread_running) && timeout) {
> > msec_delay(DELAY_INTERVAL);
> > timeout--;
> > }
> >
> >
> > return !!timeout;
> > }
> >
> > BR,
> > Zhu, Tao
> >
> > > -----Original Message-----
> > > From: Ananyev, Konstantin
> > > Sent: Friday, April 10, 2020 8:10 PM
> > > To: Zhu, TaoX <taox....@intel.com>; Lu, Wenzhuo
> > > <wenzhuo...@intel.com>; Ye, Xiaolong <xiaolong...@intel.com>
> > > Cc: dev@dpdk.org; martin.wei...@allegro-packets.com; sta...@dpdk.org
> > > Subject: RE: [PATCH v2] net/ixgbe: fix resource leak after thread exits
> > > normally
> > >
> > > Hi Tao,
> > >
> > > > 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.
> > > >
> > > > 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 | 3 +--
> > > > 1 file changed, 1 insertion(+), 2 deletions(-)
> > > >
> > > > v2 changes:
> > > > commit log.
> > > >
> > > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > > b/drivers/net/ixgbe/ixgbe_ethdev.c
> > > > index 2c57976..f141ae4 100644
> > > > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > > > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > > > @@ -4165,11 +4165,9 @@ static int
> > > ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
> > > > 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);
> > >
> > > As you already waiting for link thread termination in
> > > ixgbe_dev_wait_setup_link_complete(), do you still need
> > > pthread_cancel() here?
> > >
> > > > rte_atomic32_clear(&ad->link_thread_running);
> > > > PMD_DRV_LOG(ERR, "Link thread not complete, cancel it!");
> > > > }
> > > > @@ -4186,6 +4184,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);
> > > > --
> > > > 1.8.3.1
> > >
> >
