From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
To: "Zhu, TaoX" <taox.zhu@intel.com>,
"Lu, Wenzhuo" <wenzhuo.lu@intel.com>,
"Ye, Xiaolong" <xiaolong.ye@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
"martin.weiser@allegro-packets.com"
<martin.weiser@allegro-packets.com>,
"stable@dpdk.org" <stable@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v2] net/ixgbe: fix resource leak after thread exits normally
Date: Tue, 14 Apr 2020 11:26:09 +0000 [thread overview]
Message-ID: <BYAPR11MB3301BABDA3BD93CB0B290FF59ADA0@BYAPR11MB3301.namprd11.prod.outlook.com> (raw)
In-Reply-To: <BYAPR11MB3301D9764220C642EA4B35BD9ADA0@BYAPR11MB3301.namprd11.prod.outlook.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
> > >
> >
next prev parent reply other threads:[~2020-04-14 11:26 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-10 5:18 [dpdk-dev] [PATCH] net/ixgbe: fix resource leak after thread exits normally taox.zhu
2020-04-10 6:22 ` [dpdk-dev] [PATCH v2] " taox.zhu
2020-04-10 12:09 ` Ananyev, Konstantin
[not found] ` <60652C6914E08D41B9AA1580751B3CA9015ED8D2@SHSMSX103.ccr.corp.intel.com>
2020-04-14 10:25 ` Ananyev, Konstantin
2020-04-14 11:26 ` Ananyev, Konstantin [this message]
2020-04-15 12:27 ` [dpdk-dev] [PATCH v3] " taox.zhu
2020-04-15 14:18 ` [dpdk-dev] [PATCH v4] " taox.zhu
2020-04-20 8:49 ` Ye Xiaolong
2020-04-21 18:53 ` [dpdk-dev] [PATCH v5] " taox.zhu
2020-04-21 22:37 ` Ananyev, Konstantin
2020-04-22 2:33 ` Ye Xiaolong
2020-04-22 12:37 ` [dpdk-dev] [PATCH v6] " taox.zhu
2020-04-22 5:18 ` Ye Xiaolong
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=BYAPR11MB3301BABDA3BD93CB0B290FF59ADA0@BYAPR11MB3301.namprd11.prod.outlook.com \
--to=konstantin.ananyev@intel.com \
--cc=dev@dpdk.org \
--cc=martin.weiser@allegro-packets.com \
--cc=stable@dpdk.org \
--cc=taox.zhu@intel.com \
--cc=wenzhuo.lu@intel.com \
--cc=xiaolong.ye@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.