diff mbox series

[v2] net/ixgbe: fix resource leak after thread exits normally

Message ID 20200410062227.24201-1-taox.zhu@intel.com (mailing list archive)
State Superseded, archived
Delegated to: xiaolong ye
Headers
Series [v2] net/ixgbe: fix resource leak after thread exits normally |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/travis-robot success Travis build: passed
ci/Intel-compilation success Compilation OK

Commit Message

Zhu, TaoX April 10, 2020, 6:22 a.m. UTC 
  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.

Comments

Ananyev, Konstantin April 10, 2020, 12:09 p.m. UTC | #1 
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
Ananyev, Konstantin April 14, 2020, 10:25 a.m. UTC | #2 
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
> >
>
Ananyev, Konstantin April 14, 2020, 11:26 a.m. UTC | #3 
> 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
> > >
> >
diff mbox series

Patch

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);
 		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);