Re: [dpdk-dev] [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 */
> #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
> >
>