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

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

> 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.zhu@intel.com>; Lu, Wenzhuo
> > > <wenzhuo.lu@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>
> > > Cc: dev@dpdk.org; martin.weiser@allegro-packets.com; stable@dpdk.org
> > > Subject: RE: [PATCH v2] net/ixgbe: fix resource leak after thread exits
> > > normally
> > >
> > > Hi Tao,
> > >
> > > > 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.
> > > >
> > > > 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 | 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
> > >
> >