| Message ID | 20200410062227.24201-1-taox.zhu@intel.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | xiaolong ye |
| Headers | show |
| Series |
|
| Related | show |
| Context | Check | Description |
|---|---|---|
| ci/Intel-compilation | success | Compilation OK |
| ci/travis-robot | success | Travis build: passed |
| ci/iol-testing | success | Testing PASS |
| ci/iol-mellanox-Performance | success | Performance Testing PASS |
| ci/iol-intel-Performance | success | Performance Testing PASS |
| ci/checkpatch | warning | coding style issues |
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 > > > > >
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);