[v2] net/ixgbe: fix resource leak after thread exits normally
Checks
Commit Message
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
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
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
> >
>
> 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
> > >
> >
@@ -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);