Message ID | 1435307390-20140-2-git-send-email-michael.qiu@intel.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id D32B8C86A; Fri, 26 Jun 2015 10:30:04 +0200 (CEST) Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id BCD03C84C for <dev@dpdk.org>; Fri, 26 Jun 2015 10:30:00 +0200 (CEST) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga101.jf.intel.com with ESMTP; 26 Jun 2015 01:30:01 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,683,1427785200"; d="scan'208";a="750834580" Received: from shvmail01.sh.intel.com ([10.239.29.42]) by fmsmga002.fm.intel.com with ESMTP; 26 Jun 2015 01:29:59 -0700 Received: from shecgisg004.sh.intel.com (shecgisg004.sh.intel.com [10.239.29.89]) by shvmail01.sh.intel.com with ESMTP id t5Q8TvXs023295; Fri, 26 Jun 2015 16:29:57 +0800 Received: from shecgisg004.sh.intel.com (localhost [127.0.0.1]) by shecgisg004.sh.intel.com (8.13.6/8.13.6/SuSE Linux 0.8) with ESMTP id t5Q8TtUs020182; Fri, 26 Jun 2015 16:29:57 +0800 Received: (from dayuqiu@localhost) by shecgisg004.sh.intel.com (8.13.6/8.13.6/Submit) id t5Q8TtOM020178; Fri, 26 Jun 2015 16:29:55 +0800 From: Michael Qiu <michael.qiu@intel.com> To: dev@dpdk.org Date: Fri, 26 Jun 2015 16:29:49 +0800 Message-Id: <1435307390-20140-2-git-send-email-michael.qiu@intel.com> X-Mailer: git-send-email 1.7.4.1 In-Reply-To: <1435307390-20140-1-git-send-email-michael.qiu@intel.com> References: <1434702717-11877-1-git-send-email-michael.qiu@intel.com> <1435307390-20140-1-git-send-email-michael.qiu@intel.com> Cc: shaopeng.he@intel.com Subject: [dpdk-dev] [PATCH 1/2 v4] fm10k: Free queues when close port X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Michael Qiu
June 26, 2015, 8:29 a.m. UTC
When close a port, lots of memory should be released,
such as software rings, queues, etc.
Signed-off-by: Michael Qiu <michael.qiu@intel.com>
---
drivers/net/fm10k/fm10k_ethdev.c | 37 +++++++++++++++++++++++++++++++++----
1 file changed, 33 insertions(+), 4 deletions(-)
Comments
> -----Original Message----- > From: Qiu, Michael > Sent: Friday, June 26, 2015 9:30 AM > To: dev@dpdk.org > Cc: Chen, Jing D; He, Shaopeng; Iremonger, Bernard; Qiu, Michael > Subject: [PATCH 1/2 v4] fm10k: Free queues when close port > > When close a port, lots of memory should be released, such as software > rings, queues, etc. > > Signed-off-by: Michael Qiu <michael.qiu@intel.com> > --- Hi Michael, There are 2 comments inline > drivers/net/fm10k/fm10k_ethdev.c | 37 > +++++++++++++++++++++++++++++++++---- > 1 file changed, 33 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/fm10k/fm10k_ethdev.c > b/drivers/net/fm10k/fm10k_ethdev.c > index 406c350..eba7228 100644 > --- a/drivers/net/fm10k/fm10k_ethdev.c > +++ b/drivers/net/fm10k/fm10k_ethdev.c > @@ -65,6 +65,8 @@ static void > fm10k_MAC_filter_set(struct rte_eth_dev *dev, const u8 *mac, bool add); > static void fm10k_MACVLAN_remove_all(struct rte_eth_dev *dev); > +static void fm10k_tx_queue_release(void *queue); static void > +fm10k_rx_queue_release(void *queue); > > static void > fm10k_mbx_initlock(struct fm10k_hw *hw) @@ -809,11 +811,37 @@ > fm10k_dev_stop(struct rte_eth_dev *dev) > > PMD_INIT_FUNC_TRACE(); > > - for (i = 0; i < dev->data->nb_tx_queues; i++) > - fm10k_dev_tx_queue_stop(dev, i); > + if (dev->data->tx_queues) > + for (i = 0; i < dev->data->nb_tx_queues; i++) > + fm10k_dev_tx_queue_stop(dev, i); > > - for (i = 0; i < dev->data->nb_rx_queues; i++) > - fm10k_dev_rx_queue_stop(dev, i); > + if (dev->data->rx_queues) > + for (i = 0; i < dev->data->nb_rx_queues; i++) > + fm10k_dev_rx_queue_stop(dev, i); > +} > + > +static void > +fm10k_dev_queue_release(struct rte_eth_dev *dev) { > + int i; > + > + PMD_INIT_FUNC_TRACE(); > + > + if (dev->data->tx_queues) { > + for (i = 0; i < dev->data->nb_tx_queues; i++) > + fm10k_tx_queue_release(dev->data- > >tx_queues[i]); > + rte_free(dev->data->tx_queues); > + dev->data->tx_queues = NULL; The memory for dev->data->tx_queues is not allocated in the fm10k PMD, so it should probably not be released here. I have submitted a patch today for rte_eth_dev.c to do this. /dev/patchwork/patch/5829/ > + dev->data->nb_tx_queues = 0; > + } > + > + if (dev->data->rx_queues) { > + for (i = 0; i < dev->data->nb_rx_queues; i++) > + fm10k_rx_queue_release(dev->data- > >rx_queues[i]); > + rte_free(dev->data->rx_queues); > + dev->data->rx_queues = NULL; The memory for dev->data->rx_queues is not allocated in the fm10k PMD, so it should probably not be released here. I have submitted a patch today for rte_eth_dev.c to do this. /dev/patchwork/patch/5829/ Regards, Bernard. > + dev->data->nb_rx_queues = 0; > + } > } > > static void > @@ -828,6 +856,7 @@ fm10k_dev_close(struct rte_eth_dev *dev) > /* Stop mailbox service first */ > fm10k_close_mbx_service(hw); > fm10k_dev_stop(dev); > + fm10k_dev_queue_release(dev); > fm10k_stop_hw(hw); > } > > -- > 1.9.3
On 6/26/2015 7:02 PM, Iremonger, Bernard wrote: >> -----Original Message----- >> From: Qiu, Michael >> Sent: Friday, June 26, 2015 9:30 AM >> To: dev@dpdk.org >> Cc: Chen, Jing D; He, Shaopeng; Iremonger, Bernard; Qiu, Michael >> Subject: [PATCH 1/2 v4] fm10k: Free queues when close port >> >> When close a port, lots of memory should be released, such as software >> rings, queues, etc. >> >> Signed-off-by: Michael Qiu <michael.qiu@intel.com> >> --- > Hi Michael, > > There are 2 comments inline > >> drivers/net/fm10k/fm10k_ethdev.c | 37 >> +++++++++++++++++++++++++++++++++---- >> 1 file changed, 33 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/net/fm10k/fm10k_ethdev.c >> b/drivers/net/fm10k/fm10k_ethdev.c >> index 406c350..eba7228 100644 >> --- a/drivers/net/fm10k/fm10k_ethdev.c >> +++ b/drivers/net/fm10k/fm10k_ethdev.c >> @@ -65,6 +65,8 @@ static void >> fm10k_MAC_filter_set(struct rte_eth_dev *dev, const u8 *mac, bool add); >> static void fm10k_MACVLAN_remove_all(struct rte_eth_dev *dev); >> +static void fm10k_tx_queue_release(void *queue); static void >> +fm10k_rx_queue_release(void *queue); >> >> static void >> fm10k_mbx_initlock(struct fm10k_hw *hw) @@ -809,11 +811,37 @@ >> fm10k_dev_stop(struct rte_eth_dev *dev) >> >> PMD_INIT_FUNC_TRACE(); >> >> - for (i = 0; i < dev->data->nb_tx_queues; i++) >> - fm10k_dev_tx_queue_stop(dev, i); >> + if (dev->data->tx_queues) >> + for (i = 0; i < dev->data->nb_tx_queues; i++) >> + fm10k_dev_tx_queue_stop(dev, i); >> >> - for (i = 0; i < dev->data->nb_rx_queues; i++) >> - fm10k_dev_rx_queue_stop(dev, i); >> + if (dev->data->rx_queues) >> + for (i = 0; i < dev->data->nb_rx_queues; i++) >> + fm10k_dev_rx_queue_stop(dev, i); >> +} >> + >> +static void >> +fm10k_dev_queue_release(struct rte_eth_dev *dev) { >> + int i; >> + >> + PMD_INIT_FUNC_TRACE(); >> + >> + if (dev->data->tx_queues) { >> + for (i = 0; i < dev->data->nb_tx_queues; i++) >> + fm10k_tx_queue_release(dev->data- >>> tx_queues[i]); >> + rte_free(dev->data->tx_queues); >> + dev->data->tx_queues = NULL; > The memory for dev->data->tx_queues is not allocated in the fm10k PMD, > so it should probably not be released here. > I have submitted a patch today for rte_eth_dev.c to do this. > /dev/patchwork/patch/5829/ > >> + dev->data->nb_tx_queues = 0; >> + } >> + >> + if (dev->data->rx_queues) { >> + for (i = 0; i < dev->data->nb_rx_queues; i++) >> + fm10k_rx_queue_release(dev->data- >>> rx_queues[i]); >> + rte_free(dev->data->rx_queues); >> + dev->data->rx_queues = NULL; > The memory for dev->data->rx_queues is not allocated in the fm10k PMD, > so it should probably not be released here. > I have submitted a patch today for rte_eth_dev.c to do this. > /dev/patchwork/patch/5829/ Is it a good idea? What about to close the port for twice at a time? I think it is better to do it in rte_eth_dev_close(), I will give the comments to you. Thanks, Michael > Regards, > > Bernard. > > >> + dev->data->nb_rx_queues = 0; >> + } >> } >> >> static void >> @@ -828,6 +856,7 @@ fm10k_dev_close(struct rte_eth_dev *dev) >> /* Stop mailbox service first */ >> fm10k_close_mbx_service(hw); >> fm10k_dev_stop(dev); >> + fm10k_dev_queue_release(dev); >> fm10k_stop_hw(hw); >> } >> >> -- >> 1.9.3 >
> -----Original Message----- > From: Qiu, Michael > Sent: Monday, June 29, 2015 9:17 AM > To: Iremonger, Bernard; dev@dpdk.org > Cc: Chen, Jing D; He, Shaopeng > Subject: Re: [PATCH 1/2 v4] fm10k: Free queues when close port > > On 6/26/2015 7:02 PM, Iremonger, Bernard wrote: > >> -----Original Message----- > >> From: Qiu, Michael > >> Sent: Friday, June 26, 2015 9:30 AM > >> To: dev@dpdk.org > >> Cc: Chen, Jing D; He, Shaopeng; Iremonger, Bernard; Qiu, Michael > >> Subject: [PATCH 1/2 v4] fm10k: Free queues when close port > >> > >> When close a port, lots of memory should be released, such as > >> software rings, queues, etc. > >> > >> Signed-off-by: Michael Qiu <michael.qiu@intel.com> > >> --- > > Hi Michael, > > > > There are 2 comments inline > > > >> drivers/net/fm10k/fm10k_ethdev.c | 37 > >> +++++++++++++++++++++++++++++++++---- > >> 1 file changed, 33 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/net/fm10k/fm10k_ethdev.c > >> b/drivers/net/fm10k/fm10k_ethdev.c > >> index 406c350..eba7228 100644 > >> --- a/drivers/net/fm10k/fm10k_ethdev.c > >> +++ b/drivers/net/fm10k/fm10k_ethdev.c > >> @@ -65,6 +65,8 @@ static void > >> fm10k_MAC_filter_set(struct rte_eth_dev *dev, const u8 *mac, bool > >> add); static void fm10k_MACVLAN_remove_all(struct rte_eth_dev > *dev); > >> +static void fm10k_tx_queue_release(void *queue); static void > >> +fm10k_rx_queue_release(void *queue); > >> > >> static void > >> fm10k_mbx_initlock(struct fm10k_hw *hw) @@ -809,11 +811,37 @@ > >> fm10k_dev_stop(struct rte_eth_dev *dev) > >> > >> PMD_INIT_FUNC_TRACE(); > >> > >> - for (i = 0; i < dev->data->nb_tx_queues; i++) > >> - fm10k_dev_tx_queue_stop(dev, i); > >> + if (dev->data->tx_queues) > >> + for (i = 0; i < dev->data->nb_tx_queues; i++) > >> + fm10k_dev_tx_queue_stop(dev, i); > >> > >> - for (i = 0; i < dev->data->nb_rx_queues; i++) > >> - fm10k_dev_rx_queue_stop(dev, i); > >> + if (dev->data->rx_queues) > >> + for (i = 0; i < dev->data->nb_rx_queues; i++) > >> + fm10k_dev_rx_queue_stop(dev, i); > >> +} > >> + > >> +static void > >> +fm10k_dev_queue_release(struct rte_eth_dev *dev) { > >> + int i; > >> + > >> + PMD_INIT_FUNC_TRACE(); > >> + > >> + if (dev->data->tx_queues) { > >> + for (i = 0; i < dev->data->nb_tx_queues; i++) > >> + fm10k_tx_queue_release(dev->data- > >>> tx_queues[i]); > >> + rte_free(dev->data->tx_queues); > >> + dev->data->tx_queues = NULL; > > The memory for dev->data->tx_queues is not allocated in the fm10k > > PMD, so it should probably not be released here. > > I have submitted a patch today for rte_eth_dev.c to do this. > > /dev/patchwork/patch/5829/ > > > >> + dev->data->nb_tx_queues = 0; > >> + } > >> + > >> + if (dev->data->rx_queues) { > >> + for (i = 0; i < dev->data->nb_rx_queues; i++) > >> + fm10k_rx_queue_release(dev->data- > >>> rx_queues[i]); > >> + rte_free(dev->data->rx_queues); > >> + dev->data->rx_queues = NULL; > > The memory for dev->data->rx_queues is not allocated in the fm10k > > PMD, so it should probably not be released here. > > I have submitted a patch today for rte_eth_dev.c to do this. > > /dev/patchwork/patch/5829/ > > Is it a good idea? What about to close the port for twice at a time? > I think it is better to do it in rte_eth_dev_close(), I will give the comments to > you. > > Thanks, > Michael Hi Michael, Could you take a look at the comments on http://dpdk.org/dev/patchwork/patch/5829/ The consensus is that memory should be freed in the component that allocated it. In my pmd hotplug patches I have used a flag to ensure that dev_close is not called twice. In the e1000 patch I have added a stopped flag to struct e1000_adapter. http://dpdk.org/dev/patchwork/patch/5655/ Regards, Bernard. <snip>
On 6/29/2015 4:57 PM, Iremonger, Bernard wrote: >> -----Original Message----- >> From: Qiu, Michael >> Sent: Monday, June 29, 2015 9:17 AM >> To: Iremonger, Bernard; dev@dpdk.org >> Cc: Chen, Jing D; He, Shaopeng >> Subject: Re: [PATCH 1/2 v4] fm10k: Free queues when close port >> >> On 6/26/2015 7:02 PM, Iremonger, Bernard wrote: >>>> -----Original Message----- >>>> From: Qiu, Michael >>>> Sent: Friday, June 26, 2015 9:30 AM >>>> To: dev@dpdk.org >>>> Cc: Chen, Jing D; He, Shaopeng; Iremonger, Bernard; Qiu, Michael >>>> Subject: [PATCH 1/2 v4] fm10k: Free queues when close port >>>> >>>> When close a port, lots of memory should be released, such as >>>> software rings, queues, etc. >>>> >>>> Signed-off-by: Michael Qiu <michael.qiu@intel.com> >>>> --- >>> Hi Michael, >>> >>> There are 2 comments inline >>> >>>> drivers/net/fm10k/fm10k_ethdev.c | 37 >>>> +++++++++++++++++++++++++++++++++---- >>>> 1 file changed, 33 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/net/fm10k/fm10k_ethdev.c >>>> b/drivers/net/fm10k/fm10k_ethdev.c >>>> index 406c350..eba7228 100644 >>>> --- a/drivers/net/fm10k/fm10k_ethdev.c >>>> +++ b/drivers/net/fm10k/fm10k_ethdev.c >>>> @@ -65,6 +65,8 @@ static void >>>> fm10k_MAC_filter_set(struct rte_eth_dev *dev, const u8 *mac, bool >>>> add); static void fm10k_MACVLAN_remove_all(struct rte_eth_dev >> *dev); >>>> +static void fm10k_tx_queue_release(void *queue); static void >>>> +fm10k_rx_queue_release(void *queue); >>>> >>>> static void >>>> fm10k_mbx_initlock(struct fm10k_hw *hw) @@ -809,11 +811,37 @@ >>>> fm10k_dev_stop(struct rte_eth_dev *dev) >>>> >>>> PMD_INIT_FUNC_TRACE(); >>>> >>>> - for (i = 0; i < dev->data->nb_tx_queues; i++) >>>> - fm10k_dev_tx_queue_stop(dev, i); >>>> + if (dev->data->tx_queues) >>>> + for (i = 0; i < dev->data->nb_tx_queues; i++) >>>> + fm10k_dev_tx_queue_stop(dev, i); >>>> >>>> - for (i = 0; i < dev->data->nb_rx_queues; i++) >>>> - fm10k_dev_rx_queue_stop(dev, i); >>>> + if (dev->data->rx_queues) >>>> + for (i = 0; i < dev->data->nb_rx_queues; i++) >>>> + fm10k_dev_rx_queue_stop(dev, i); >>>> +} >>>> + >>>> +static void >>>> +fm10k_dev_queue_release(struct rte_eth_dev *dev) { >>>> + int i; >>>> + >>>> + PMD_INIT_FUNC_TRACE(); >>>> + >>>> + if (dev->data->tx_queues) { >>>> + for (i = 0; i < dev->data->nb_tx_queues; i++) >>>> + fm10k_tx_queue_release(dev->data- >>>>> tx_queues[i]); >>>> + rte_free(dev->data->tx_queues); >>>> + dev->data->tx_queues = NULL; >>> The memory for dev->data->tx_queues is not allocated in the fm10k >>> PMD, so it should probably not be released here. >>> I have submitted a patch today for rte_eth_dev.c to do this. >>> /dev/patchwork/patch/5829/ >>> >>>> + dev->data->nb_tx_queues = 0; >>>> + } >>>> + >>>> + if (dev->data->rx_queues) { >>>> + for (i = 0; i < dev->data->nb_rx_queues; i++) >>>> + fm10k_rx_queue_release(dev->data- >>>>> rx_queues[i]); >>>> + rte_free(dev->data->rx_queues); >>>> + dev->data->rx_queues = NULL; >>> The memory for dev->data->rx_queues is not allocated in the fm10k >>> PMD, so it should probably not be released here. >>> I have submitted a patch today for rte_eth_dev.c to do this. >>> /dev/patchwork/patch/5829/ >> Is it a good idea? What about to close the port for twice at a time? >> I think it is better to do it in rte_eth_dev_close(), I will give the comments to >> you. >> >> Thanks, >> Michael > Hi Michael, > Could you take a look at the comments on http://dpdk.org/dev/patchwork/patch/5829/ Hi, Bernard I have give comments on it. > The consensus is that memory should be freed in the component that allocated it. > In my pmd hotplug patches I have used a flag to ensure that dev_close is not called twice. > In the e1000 patch I have added a stopped flag to struct e1000_adapter. > http://dpdk.org/dev/patchwork/patch/5655/ I reviewed your patch about ixgbe and fvl before. But forget e1000. In my logic, when dev->data->rx_queues is NULL, that means this device has been closed before. What else, we even do not care about whether it has been closed or not, when close() function be called, all memory should be freed if exist am I right? So, check dev->data->rx_queues whether it is NULL will be recommend in close function, only this could avoid unsafe situations for pointer. Thanks, Michael > Regards, > > Bernard. > > <snip> > > >
> -----Original Message----- > From: Qiu, Michael > Sent: Monday, June 29, 2015 10:21 AM > To: Iremonger, Bernard; dev@dpdk.org > Cc: Chen, Jing D; He, Shaopeng > Subject: Re: [PATCH 1/2 v4] fm10k: Free queues when close port > > On 6/29/2015 4:57 PM, Iremonger, Bernard wrote: > >> -----Original Message----- > >> From: Qiu, Michael > >> Sent: Monday, June 29, 2015 9:17 AM > >> To: Iremonger, Bernard; dev@dpdk.org > >> Cc: Chen, Jing D; He, Shaopeng > >> Subject: Re: [PATCH 1/2 v4] fm10k: Free queues when close port > >> > >> On 6/26/2015 7:02 PM, Iremonger, Bernard wrote: > >>>> -----Original Message----- > >>>> From: Qiu, Michael > >>>> Sent: Friday, June 26, 2015 9:30 AM > >>>> To: dev@dpdk.org > >>>> Cc: Chen, Jing D; He, Shaopeng; Iremonger, Bernard; Qiu, Michael > >>>> Subject: [PATCH 1/2 v4] fm10k: Free queues when close port > >>>> > >>>> When close a port, lots of memory should be released, such as > >>>> software rings, queues, etc. > >>>> > >>>> Signed-off-by: Michael Qiu <michael.qiu@intel.com> > >>>> --- > >>> Hi Michael, > >>> > >>> There are 2 comments inline > >>> > >>>> drivers/net/fm10k/fm10k_ethdev.c | 37 > >>>> +++++++++++++++++++++++++++++++++---- > >>>> 1 file changed, 33 insertions(+), 4 deletions(-) > >>>> > >>>> diff --git a/drivers/net/fm10k/fm10k_ethdev.c > >>>> b/drivers/net/fm10k/fm10k_ethdev.c > >>>> index 406c350..eba7228 100644 > >>>> --- a/drivers/net/fm10k/fm10k_ethdev.c > >>>> +++ b/drivers/net/fm10k/fm10k_ethdev.c > >>>> @@ -65,6 +65,8 @@ static void > >>>> fm10k_MAC_filter_set(struct rte_eth_dev *dev, const u8 *mac, bool > >>>> add); static void fm10k_MACVLAN_remove_all(struct rte_eth_dev > >> *dev); > >>>> +static void fm10k_tx_queue_release(void *queue); static void > >>>> +fm10k_rx_queue_release(void *queue); > >>>> > >>>> static void > >>>> fm10k_mbx_initlock(struct fm10k_hw *hw) @@ -809,11 +811,37 @@ > >>>> fm10k_dev_stop(struct rte_eth_dev *dev) > >>>> > >>>> PMD_INIT_FUNC_TRACE(); > >>>> > >>>> - for (i = 0; i < dev->data->nb_tx_queues; i++) > >>>> - fm10k_dev_tx_queue_stop(dev, i); > >>>> + if (dev->data->tx_queues) > >>>> + for (i = 0; i < dev->data->nb_tx_queues; i++) > >>>> + fm10k_dev_tx_queue_stop(dev, i); > >>>> > >>>> - for (i = 0; i < dev->data->nb_rx_queues; i++) > >>>> - fm10k_dev_rx_queue_stop(dev, i); > >>>> + if (dev->data->rx_queues) > >>>> + for (i = 0; i < dev->data->nb_rx_queues; i++) > >>>> + fm10k_dev_rx_queue_stop(dev, i); } > >>>> + > >>>> +static void > >>>> +fm10k_dev_queue_release(struct rte_eth_dev *dev) { > >>>> + int i; > >>>> + > >>>> + PMD_INIT_FUNC_TRACE(); > >>>> + > >>>> + if (dev->data->tx_queues) { > >>>> + for (i = 0; i < dev->data->nb_tx_queues; i++) > >>>> + fm10k_tx_queue_release(dev->data- > >>>>> tx_queues[i]); > >>>> + rte_free(dev->data->tx_queues); > >>>> + dev->data->tx_queues = NULL; > >>> The memory for dev->data->tx_queues is not allocated in the fm10k > >>> PMD, so it should probably not be released here. > >>> I have submitted a patch today for rte_eth_dev.c to do this. > >>> /dev/patchwork/patch/5829/ > >>> > >>>> + dev->data->nb_tx_queues = 0; > >>>> + } > >>>> + > >>>> + if (dev->data->rx_queues) { > >>>> + for (i = 0; i < dev->data->nb_rx_queues; i++) > >>>> + fm10k_rx_queue_release(dev->data- > >>>>> rx_queues[i]); > >>>> + rte_free(dev->data->rx_queues); > >>>> + dev->data->rx_queues = NULL; > >>> The memory for dev->data->rx_queues is not allocated in the fm10k > >>> PMD, so it should probably not be released here. > >>> I have submitted a patch today for rte_eth_dev.c to do this. > >>> /dev/patchwork/patch/5829/ > >> Is it a good idea? What about to close the port for twice at a time? > >> I think it is better to do it in rte_eth_dev_close(), I will give the > >> comments to you. > >> > >> Thanks, > >> Michael > > Hi Michael, > > Could you take a look at the comments on > > http://dpdk.org/dev/patchwork/patch/5829/ > > Hi, Bernard > > I have give comments on it. > > > The consensus is that memory should be freed in the component that > allocated it. > > In my pmd hotplug patches I have used a flag to ensure that dev_close is > not called twice. > > In the e1000 patch I have added a stopped flag to struct e1000_adapter. > > http://dpdk.org/dev/patchwork/patch/5655/ > > > > I reviewed your patch about ixgbe and fvl before. But forget e1000. > > In my logic, when dev->data->rx_queues is NULL, that means this device has > been closed before. What else, we even do not care about whether it has > been closed or not, when close() function be called, all memory should be > freed if exist am I right? > > So, check dev->data->rx_queues whether it is NULL will be recommend in > close function, only this could avoid unsafe situations for pointer. > > Thanks, > Michael Hi Michael, In most of the pmd's memory is allocated in the dev_init()functions and released in the dev_uninit() functions. The dev_uninit() functions call dev_close(), so either way the memory is released. The point I am trying to make is that the rx_queue and tx_queue memory is not allocated by the pmd and so it should not be freed by the pmd (please see comments on http://dpdk.org/dev/patchwork/patch/5790/) The memory is allocated in rte_eth_dev_rx_queue_config() and rte_eth_dev_tx_queue_config(), which are both called from rte_eth_dev_configure() which is called by the application (for example test_pmd). So it seems to make sense to free this memory in rte_eth_dev_uninit(). Regards, Bernard.
Hi Michael, > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Qiu, Michael > Sent: Monday, June 29, 2015 10:21 AM > To: Iremonger, Bernard; dev@dpdk.org > Cc: He, Shaopeng > Subject: Re: [dpdk-dev] [PATCH 1/2 v4] fm10k: Free queues when close port > > On 6/29/2015 4:57 PM, Iremonger, Bernard wrote: > >> -----Original Message----- > >> From: Qiu, Michael > >> Sent: Monday, June 29, 2015 9:17 AM > >> To: Iremonger, Bernard; dev@dpdk.org > >> Cc: Chen, Jing D; He, Shaopeng > >> Subject: Re: [PATCH 1/2 v4] fm10k: Free queues when close port > >> > >> On 6/26/2015 7:02 PM, Iremonger, Bernard wrote: > >>>> -----Original Message----- > >>>> From: Qiu, Michael > >>>> Sent: Friday, June 26, 2015 9:30 AM > >>>> To: dev@dpdk.org > >>>> Cc: Chen, Jing D; He, Shaopeng; Iremonger, Bernard; Qiu, Michael > >>>> Subject: [PATCH 1/2 v4] fm10k: Free queues when close port > >>>> > >>>> When close a port, lots of memory should be released, such as > >>>> software rings, queues, etc. > >>>> > >>>> Signed-off-by: Michael Qiu <michael.qiu@intel.com> > >>>> --- > >>> Hi Michael, > >>> > >>> There are 2 comments inline > >>> > >>>> drivers/net/fm10k/fm10k_ethdev.c | 37 > >>>> +++++++++++++++++++++++++++++++++---- > >>>> 1 file changed, 33 insertions(+), 4 deletions(-) > >>>> > >>>> diff --git a/drivers/net/fm10k/fm10k_ethdev.c > >>>> b/drivers/net/fm10k/fm10k_ethdev.c > >>>> index 406c350..eba7228 100644 > >>>> --- a/drivers/net/fm10k/fm10k_ethdev.c > >>>> +++ b/drivers/net/fm10k/fm10k_ethdev.c > >>>> @@ -65,6 +65,8 @@ static void > >>>> fm10k_MAC_filter_set(struct rte_eth_dev *dev, const u8 *mac, bool > >>>> add); static void fm10k_MACVLAN_remove_all(struct rte_eth_dev > >> *dev); > >>>> +static void fm10k_tx_queue_release(void *queue); static void > >>>> +fm10k_rx_queue_release(void *queue); > >>>> > >>>> static void > >>>> fm10k_mbx_initlock(struct fm10k_hw *hw) @@ -809,11 +811,37 @@ > >>>> fm10k_dev_stop(struct rte_eth_dev *dev) > >>>> > >>>> PMD_INIT_FUNC_TRACE(); > >>>> > >>>> - for (i = 0; i < dev->data->nb_tx_queues; i++) > >>>> - fm10k_dev_tx_queue_stop(dev, i); > >>>> + if (dev->data->tx_queues) > >>>> + for (i = 0; i < dev->data->nb_tx_queues; i++) > >>>> + fm10k_dev_tx_queue_stop(dev, i); > >>>> > >>>> - for (i = 0; i < dev->data->nb_rx_queues; i++) > >>>> - fm10k_dev_rx_queue_stop(dev, i); > >>>> + if (dev->data->rx_queues) > >>>> + for (i = 0; i < dev->data->nb_rx_queues; i++) > >>>> + fm10k_dev_rx_queue_stop(dev, i); > >>>> +} > >>>> + > >>>> +static void > >>>> +fm10k_dev_queue_release(struct rte_eth_dev *dev) { > >>>> + int i; > >>>> + > >>>> + PMD_INIT_FUNC_TRACE(); > >>>> + > >>>> + if (dev->data->tx_queues) { > >>>> + for (i = 0; i < dev->data->nb_tx_queues; i++) > >>>> + fm10k_tx_queue_release(dev->data- > >>>>> tx_queues[i]); > >>>> + rte_free(dev->data->tx_queues); > >>>> + dev->data->tx_queues = NULL; > >>> The memory for dev->data->tx_queues is not allocated in the fm10k > >>> PMD, so it should probably not be released here. > >>> I have submitted a patch today for rte_eth_dev.c to do this. > >>> /dev/patchwork/patch/5829/ > >>> > >>>> + dev->data->nb_tx_queues = 0; > >>>> + } > >>>> + > >>>> + if (dev->data->rx_queues) { > >>>> + for (i = 0; i < dev->data->nb_rx_queues; i++) > >>>> + fm10k_rx_queue_release(dev->data- > >>>>> rx_queues[i]); > >>>> + rte_free(dev->data->rx_queues); > >>>> + dev->data->rx_queues = NULL; > >>> The memory for dev->data->rx_queues is not allocated in the fm10k > >>> PMD, so it should probably not be released here. > >>> I have submitted a patch today for rte_eth_dev.c to do this. > >>> /dev/patchwork/patch/5829/ > >> Is it a good idea? What about to close the port for twice at a time? > >> I think it is better to do it in rte_eth_dev_close(), I will give the comments to > >> you. > >> > >> Thanks, > >> Michael > > Hi Michael, > > Could you take a look at the comments on http://dpdk.org/dev/patchwork/patch/5829/ > > Hi, Bernard > > I have give comments on it. > > > The consensus is that memory should be freed in the component that allocated it. > > In my pmd hotplug patches I have used a flag to ensure that dev_close is not called twice. > > In the e1000 patch I have added a stopped flag to struct e1000_adapter. > > http://dpdk.org/dev/patchwork/patch/5655/ > > > > I reviewed your patch about ixgbe and fvl before. But forget e1000. > > In my logic, when dev->data->rx_queues is NULL, that means this device > has been closed before. What else, we even do not care about whether it > has been closed or not, when close() function be called, all memory > should be freed if exist am I right? > > So, check dev->data->rx_queues whether it is NULL will be recommend in > close function, only this could avoid unsafe situations for pointer. It seems you are mixing 2 things there: Contents of dev->data->rx_queues[] is mamanged by each PMD, and yes should be alloced and freed by each PMD. dev->data->rx_queues[] itself is allocated/reallocated and should be freed by rte_ethdev layer. PMD, in theory, simly doesn't know how it was allocated. Konstantin > > Thanks, > Michael > > Regards, > > > > Bernard. > > > > <snip> > > > > > >
On 2015/6/29 17:54, Iremonger, Bernard wrote: >> -----Original Message----- >> From: Qiu, Michael >> Sent: Monday, June 29, 2015 10:21 AM >> To: Iremonger, Bernard; dev@dpdk.org >> Cc: Chen, Jing D; He, Shaopeng >> Subject: Re: [PATCH 1/2 v4] fm10k: Free queues when close port >> >> On 6/29/2015 4:57 PM, Iremonger, Bernard wrote: >>>> -----Original Message----- >>>> From: Qiu, Michael >>>> Sent: Monday, June 29, 2015 9:17 AM >>>> To: Iremonger, Bernard; dev@dpdk.org >>>> Cc: Chen, Jing D; He, Shaopeng >>>> Subject: Re: [PATCH 1/2 v4] fm10k: Free queues when close port >>>> >>>> On 6/26/2015 7:02 PM, Iremonger, Bernard wrote: >>>>>> -----Original Message----- >>>>>> From: Qiu, Michael >>>>>> Sent: Friday, June 26, 2015 9:30 AM >>>>>> To: dev@dpdk.org >>>>>> Cc: Chen, Jing D; He, Shaopeng; Iremonger, Bernard; Qiu, Michael >>>>>> Subject: [PATCH 1/2 v4] fm10k: Free queues when close port >>>>>> >>>>>> When close a port, lots of memory should be released, such as >>>>>> software rings, queues, etc. >>>>>> >>>>>> Signed-off-by: Michael Qiu <michael.qiu@intel.com> >>>>>> --- >>>>> Hi Michael, >>>>> >>>>> There are 2 comments inline >>>>> >>>>>> drivers/net/fm10k/fm10k_ethdev.c | 37 >>>>>> +++++++++++++++++++++++++++++++++---- >>>>>> 1 file changed, 33 insertions(+), 4 deletions(-) >>>>>> >>>>>> diff --git a/drivers/net/fm10k/fm10k_ethdev.c >>>>>> b/drivers/net/fm10k/fm10k_ethdev.c >>>>>> index 406c350..eba7228 100644 >>>>>> --- a/drivers/net/fm10k/fm10k_ethdev.c >>>>>> +++ b/drivers/net/fm10k/fm10k_ethdev.c >>>>>> @@ -65,6 +65,8 @@ static void >>>>>> fm10k_MAC_filter_set(struct rte_eth_dev *dev, const u8 *mac, bool >>>>>> add); static void fm10k_MACVLAN_remove_all(struct rte_eth_dev >>>> *dev); >>>>>> +static void fm10k_tx_queue_release(void *queue); static void >>>>>> +fm10k_rx_queue_release(void *queue); >>>>>> >>>>>> static void >>>>>> fm10k_mbx_initlock(struct fm10k_hw *hw) @@ -809,11 +811,37 @@ >>>>>> fm10k_dev_stop(struct rte_eth_dev *dev) >>>>>> >>>>>> PMD_INIT_FUNC_TRACE(); >>>>>> >>>>>> - for (i = 0; i < dev->data->nb_tx_queues; i++) >>>>>> - fm10k_dev_tx_queue_stop(dev, i); >>>>>> + if (dev->data->tx_queues) >>>>>> + for (i = 0; i < dev->data->nb_tx_queues; i++) >>>>>> + fm10k_dev_tx_queue_stop(dev, i); >>>>>> >>>>>> - for (i = 0; i < dev->data->nb_rx_queues; i++) >>>>>> - fm10k_dev_rx_queue_stop(dev, i); >>>>>> + if (dev->data->rx_queues) >>>>>> + for (i = 0; i < dev->data->nb_rx_queues; i++) >>>>>> + fm10k_dev_rx_queue_stop(dev, i); } >>>>>> + >>>>>> +static void >>>>>> +fm10k_dev_queue_release(struct rte_eth_dev *dev) { >>>>>> + int i; >>>>>> + >>>>>> + PMD_INIT_FUNC_TRACE(); >>>>>> + >>>>>> + if (dev->data->tx_queues) { >>>>>> + for (i = 0; i < dev->data->nb_tx_queues; i++) >>>>>> + fm10k_tx_queue_release(dev->data- >>>>>>> tx_queues[i]); >>>>>> + rte_free(dev->data->tx_queues); >>>>>> + dev->data->tx_queues = NULL; >>>>> The memory for dev->data->tx_queues is not allocated in the fm10k >>>>> PMD, so it should probably not be released here. >>>>> I have submitted a patch today for rte_eth_dev.c to do this. >>>>> /dev/patchwork/patch/5829/ >>>>> >>>>>> + dev->data->nb_tx_queues = 0; >>>>>> + } >>>>>> + >>>>>> + if (dev->data->rx_queues) { >>>>>> + for (i = 0; i < dev->data->nb_rx_queues; i++) >>>>>> + fm10k_rx_queue_release(dev->data- >>>>>>> rx_queues[i]); >>>>>> + rte_free(dev->data->rx_queues); >>>>>> + dev->data->rx_queues = NULL; >>>>> The memory for dev->data->rx_queues is not allocated in the fm10k >>>>> PMD, so it should probably not be released here. >>>>> I have submitted a patch today for rte_eth_dev.c to do this. >>>>> /dev/patchwork/patch/5829/ >>>> Is it a good idea? What about to close the port for twice at a time? >>>> I think it is better to do it in rte_eth_dev_close(), I will give the >>>> comments to you. >>>> >>>> Thanks, >>>> Michael >>> Hi Michael, >>> Could you take a look at the comments on >>> http://dpdk.org/dev/patchwork/patch/5829/ >> Hi, Bernard >> >> I have give comments on it. >> >>> The consensus is that memory should be freed in the component that >> allocated it. >>> In my pmd hotplug patches I have used a flag to ensure that dev_close is >> not called twice. >>> In the e1000 patch I have added a stopped flag to struct e1000_adapter. >>> http://dpdk.org/dev/patchwork/patch/5655/ >> >> >> I reviewed your patch about ixgbe and fvl before. But forget e1000. >> >> In my logic, when dev->data->rx_queues is NULL, that means this device has >> been closed before. What else, we even do not care about whether it has >> been closed or not, when close() function be called, all memory should be >> freed if exist am I right? >> >> So, check dev->data->rx_queues whether it is NULL will be recommend in >> close function, only this could avoid unsafe situations for pointer. >> >> Thanks, >> Michael > Hi Michael, > > In most of the pmd's memory is allocated in the dev_init()functions and released in the dev_uninit() functions. The dev_uninit() functions call dev_close(), so either way the memory is released. Yes, but consider that, without hotplug, users just stop a port and close it. then what happens? the memory does not released! So that's why I recommend to release the memory on close() function, it could be in EAL level like rte_eth_dev_close(). Maybe my understand is wrong, please point me out :) Thanks, Michael > The point I am trying to make is that the rx_queue and tx_queue memory is not allocated by the pmd and so it should not be freed by the pmd (please see comments on > http://dpdk.org/dev/patchwork/patch/5790/) > The memory is allocated in rte_eth_dev_rx_queue_config() and rte_eth_dev_tx_queue_config(), > which are both called from rte_eth_dev_configure() which is called by the application (for example test_pmd). So it seems to make sense to free this memory in rte_eth_dev_uninit(). > > Regards, > > Bernard. > > >
On 2015/6/29 19:08, Ananyev, Konstantin wrote: > Hi Michael, > >> -----Original Message----- >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Qiu, Michael >> Sent: Monday, June 29, 2015 10:21 AM >> To: Iremonger, Bernard; dev@dpdk.org >> Cc: He, Shaopeng >> Subject: Re: [dpdk-dev] [PATCH 1/2 v4] fm10k: Free queues when close port >> >> On 6/29/2015 4:57 PM, Iremonger, Bernard wrote: >>>> -----Original Message----- >>>> From: Qiu, Michael >>>> Sent: Monday, June 29, 2015 9:17 AM >>>> To: Iremonger, Bernard; dev@dpdk.org >>>> Cc: Chen, Jing D; He, Shaopeng >>>> Subject: Re: [PATCH 1/2 v4] fm10k: Free queues when close port >>>> >>>> On 6/26/2015 7:02 PM, Iremonger, Bernard wrote: >>>>>> -----Original Message----- >>>>>> From: Qiu, Michael >>>>>> Sent: Friday, June 26, 2015 9:30 AM >>>>>> To: dev@dpdk.org >>>>>> Cc: Chen, Jing D; He, Shaopeng; Iremonger, Bernard; Qiu, Michael >>>>>> Subject: [PATCH 1/2 v4] fm10k: Free queues when close port >>>>>> >>>>>> When close a port, lots of memory should be released, such as >>>>>> software rings, queues, etc. >>>>>> >>>>>> Signed-off-by: Michael Qiu <michael.qiu@intel.com> >>>>>> --- >>>>> Hi Michael, >>>>> >>>>> There are 2 comments inline >>>>> >>>>>> drivers/net/fm10k/fm10k_ethdev.c | 37 >>>>>> +++++++++++++++++++++++++++++++++---- >>>>>> 1 file changed, 33 insertions(+), 4 deletions(-) >>>>>> >>>>>> diff --git a/drivers/net/fm10k/fm10k_ethdev.c >>>>>> b/drivers/net/fm10k/fm10k_ethdev.c >>>>>> index 406c350..eba7228 100644 >>>>>> --- a/drivers/net/fm10k/fm10k_ethdev.c >>>>>> +++ b/drivers/net/fm10k/fm10k_ethdev.c >>>>>> @@ -65,6 +65,8 @@ static void >>>>>> fm10k_MAC_filter_set(struct rte_eth_dev *dev, const u8 *mac, bool >>>>>> add); static void fm10k_MACVLAN_remove_all(struct rte_eth_dev >>>> *dev); >>>>>> +static void fm10k_tx_queue_release(void *queue); static void >>>>>> +fm10k_rx_queue_release(void *queue); >>>>>> >>>>>> static void >>>>>> fm10k_mbx_initlock(struct fm10k_hw *hw) @@ -809,11 +811,37 @@ >>>>>> fm10k_dev_stop(struct rte_eth_dev *dev) >>>>>> >>>>>> PMD_INIT_FUNC_TRACE(); >>>>>> >>>>>> - for (i = 0; i < dev->data->nb_tx_queues; i++) >>>>>> - fm10k_dev_tx_queue_stop(dev, i); >>>>>> + if (dev->data->tx_queues) >>>>>> + for (i = 0; i < dev->data->nb_tx_queues; i++) >>>>>> + fm10k_dev_tx_queue_stop(dev, i); >>>>>> >>>>>> - for (i = 0; i < dev->data->nb_rx_queues; i++) >>>>>> - fm10k_dev_rx_queue_stop(dev, i); >>>>>> + if (dev->data->rx_queues) >>>>>> + for (i = 0; i < dev->data->nb_rx_queues; i++) >>>>>> + fm10k_dev_rx_queue_stop(dev, i); >>>>>> +} >>>>>> + >>>>>> +static void >>>>>> +fm10k_dev_queue_release(struct rte_eth_dev *dev) { >>>>>> + int i; >>>>>> + >>>>>> + PMD_INIT_FUNC_TRACE(); >>>>>> + >>>>>> + if (dev->data->tx_queues) { >>>>>> + for (i = 0; i < dev->data->nb_tx_queues; i++) >>>>>> + fm10k_tx_queue_release(dev->data- >>>>>>> tx_queues[i]); >>>>>> + rte_free(dev->data->tx_queues); >>>>>> + dev->data->tx_queues = NULL; >>>>> The memory for dev->data->tx_queues is not allocated in the fm10k >>>>> PMD, so it should probably not be released here. >>>>> I have submitted a patch today for rte_eth_dev.c to do this. >>>>> /dev/patchwork/patch/5829/ >>>>> >>>>>> + dev->data->nb_tx_queues = 0; >>>>>> + } >>>>>> + >>>>>> + if (dev->data->rx_queues) { >>>>>> + for (i = 0; i < dev->data->nb_rx_queues; i++) >>>>>> + fm10k_rx_queue_release(dev->data- >>>>>>> rx_queues[i]); >>>>>> + rte_free(dev->data->rx_queues); >>>>>> + dev->data->rx_queues = NULL; >>>>> The memory for dev->data->rx_queues is not allocated in the fm10k >>>>> PMD, so it should probably not be released here. >>>>> I have submitted a patch today for rte_eth_dev.c to do this. >>>>> /dev/patchwork/patch/5829/ >>>> Is it a good idea? What about to close the port for twice at a time? >>>> I think it is better to do it in rte_eth_dev_close(), I will give the comments to >>>> you. >>>> >>>> Thanks, >>>> Michael >>> Hi Michael, >>> Could you take a look at the comments on http://dpdk.org/dev/patchwork/patch/5829/ >> Hi, Bernard >> >> I have give comments on it. >> >>> The consensus is that memory should be freed in the component that allocated it. >>> In my pmd hotplug patches I have used a flag to ensure that dev_close is not called twice. >>> In the e1000 patch I have added a stopped flag to struct e1000_adapter. >>> http://dpdk.org/dev/patchwork/patch/5655/ >> >> >> I reviewed your patch about ixgbe and fvl before. But forget e1000. >> >> In my logic, when dev->data->rx_queues is NULL, that means this device >> has been closed before. What else, we even do not care about whether it >> has been closed or not, when close() function be called, all memory >> should be freed if exist am I right? >> >> So, check dev->data->rx_queues whether it is NULL will be recommend in >> close function, only this could avoid unsafe situations for pointer. > > It seems you are mixing 2 things there: > Contents of dev->data->rx_queues[] is mamanged by each PMD, and yes should be alloced and freed by each PMD. > dev->data->rx_queues[] itself is allocated/reallocated and should be freed by rte_ethdev layer. > PMD, in theory, simly doesn't know how it was allocated. I really do not mix these, what I mean is when you try to release the dev->data->rx_queues you must confirm two things: 1. dev->data->rx_queues is not NULL 2. dev->data->rx_queues[i] is already freed Also I agree dev->data->rx_queues is better freed in rte_ethdev layer. And I will remove free dev->data->rx_queues memory in PMD, and based on bernard's new patch. Also in PMD, to check dev->data->rx_queues to see if could release dev->data->rx_queues[i] when close() Thanks, Michael > Konstantin > > >> Thanks, >> Michael >>> Regards, >>> >>> Bernard. >>> >>> <snip> >>> >>> >>> >
On 2015/6/29 22:58, Qiu, Michael wrote: > On 2015/6/29 17:54, Iremonger, Bernard wrote: >>> -----Original Message----- >>> From: Qiu, Michael >>> Sent: Monday, June 29, 2015 10:21 AM >>> To: Iremonger, Bernard; dev@dpdk.org >>> Cc: Chen, Jing D; He, Shaopeng >>> Subject: Re: [PATCH 1/2 v4] fm10k: Free queues when close port >>> >>> On 6/29/2015 4:57 PM, Iremonger, Bernard wrote: >>>>> -----Original Message----- >>>>> From: Qiu, Michael >>>>> Sent: Monday, June 29, 2015 9:17 AM >>>>> To: Iremonger, Bernard; dev@dpdk.org >>>>> Cc: Chen, Jing D; He, Shaopeng >>>>> Subject: Re: [PATCH 1/2 v4] fm10k: Free queues when close port >>>>> >>>>> On 6/26/2015 7:02 PM, Iremonger, Bernard wrote: >>>>>>> -----Original Message----- >>>>>>> From: Qiu, Michael >>>>>>> Sent: Friday, June 26, 2015 9:30 AM >>>>>>> To: dev@dpdk.org >>>>>>> Cc: Chen, Jing D; He, Shaopeng; Iremonger, Bernard; Qiu, Michael >>>>>>> Subject: [PATCH 1/2 v4] fm10k: Free queues when close port >>>>>>> >>>>>>> When close a port, lots of memory should be released, such as >>>>>>> software rings, queues, etc. >>>>>>> >>>>>>> Signed-off-by: Michael Qiu <michael.qiu@intel.com> >>>>>>> --- >>>>>> Hi Michael, >>>>>> >>>>>> There are 2 comments inline >>>>>> >>>>>>> drivers/net/fm10k/fm10k_ethdev.c | 37 >>>>>>> +++++++++++++++++++++++++++++++++---- >>>>>>> 1 file changed, 33 insertions(+), 4 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/net/fm10k/fm10k_ethdev.c >>>>>>> b/drivers/net/fm10k/fm10k_ethdev.c >>>>>>> index 406c350..eba7228 100644 >>>>>>> --- a/drivers/net/fm10k/fm10k_ethdev.c >>>>>>> +++ b/drivers/net/fm10k/fm10k_ethdev.c >>>>>>> @@ -65,6 +65,8 @@ static void >>>>>>> fm10k_MAC_filter_set(struct rte_eth_dev *dev, const u8 *mac, bool >>>>>>> add); static void fm10k_MACVLAN_remove_all(struct rte_eth_dev >>>>> *dev); >>>>>>> +static void fm10k_tx_queue_release(void *queue); static void >>>>>>> +fm10k_rx_queue_release(void *queue); >>>>>>> >>>>>>> static void >>>>>>> fm10k_mbx_initlock(struct fm10k_hw *hw) @@ -809,11 +811,37 @@ >>>>>>> fm10k_dev_stop(struct rte_eth_dev *dev) >>>>>>> >>>>>>> PMD_INIT_FUNC_TRACE(); >>>>>>> >>>>>>> - for (i = 0; i < dev->data->nb_tx_queues; i++) >>>>>>> - fm10k_dev_tx_queue_stop(dev, i); >>>>>>> + if (dev->data->tx_queues) >>>>>>> + for (i = 0; i < dev->data->nb_tx_queues; i++) >>>>>>> + fm10k_dev_tx_queue_stop(dev, i); >>>>>>> >>>>>>> - for (i = 0; i < dev->data->nb_rx_queues; i++) >>>>>>> - fm10k_dev_rx_queue_stop(dev, i); >>>>>>> + if (dev->data->rx_queues) >>>>>>> + for (i = 0; i < dev->data->nb_rx_queues; i++) >>>>>>> + fm10k_dev_rx_queue_stop(dev, i); } >>>>>>> + >>>>>>> +static void >>>>>>> +fm10k_dev_queue_release(struct rte_eth_dev *dev) { >>>>>>> + int i; >>>>>>> + >>>>>>> + PMD_INIT_FUNC_TRACE(); >>>>>>> + >>>>>>> + if (dev->data->tx_queues) { >>>>>>> + for (i = 0; i < dev->data->nb_tx_queues; i++) >>>>>>> + fm10k_tx_queue_release(dev->data- >>>>>>>> tx_queues[i]); >>>>>>> + rte_free(dev->data->tx_queues); >>>>>>> + dev->data->tx_queues = NULL; >>>>>> The memory for dev->data->tx_queues is not allocated in the fm10k >>>>>> PMD, so it should probably not be released here. >>>>>> I have submitted a patch today for rte_eth_dev.c to do this. >>>>>> /dev/patchwork/patch/5829/ >>>>>> >>>>>>> + dev->data->nb_tx_queues = 0; >>>>>>> + } >>>>>>> + >>>>>>> + if (dev->data->rx_queues) { >>>>>>> + for (i = 0; i < dev->data->nb_rx_queues; i++) >>>>>>> + fm10k_rx_queue_release(dev->data- >>>>>>>> rx_queues[i]); >>>>>>> + rte_free(dev->data->rx_queues); >>>>>>> + dev->data->rx_queues = NULL; >>>>>> The memory for dev->data->rx_queues is not allocated in the fm10k >>>>>> PMD, so it should probably not be released here. >>>>>> I have submitted a patch today for rte_eth_dev.c to do this. >>>>>> /dev/patchwork/patch/5829/ >>>>> Is it a good idea? What about to close the port for twice at a time? >>>>> I think it is better to do it in rte_eth_dev_close(), I will give the >>>>> comments to you. >>>>> >>>>> Thanks, >>>>> Michael >>>> Hi Michael, >>>> Could you take a look at the comments on >>>> http://dpdk.org/dev/patchwork/patch/5829/ >>> Hi, Bernard >>> >>> I have give comments on it. >>> >>>> The consensus is that memory should be freed in the component that >>> allocated it. >>>> In my pmd hotplug patches I have used a flag to ensure that dev_close is >>> not called twice. >>>> In the e1000 patch I have added a stopped flag to struct e1000_adapter. >>>> http://dpdk.org/dev/patchwork/patch/5655/ >>> >>> I reviewed your patch about ixgbe and fvl before. But forget e1000. >>> >>> In my logic, when dev->data->rx_queues is NULL, that means this device has >>> been closed before. What else, we even do not care about whether it has >>> been closed or not, when close() function be called, all memory should be >>> freed if exist am I right? >>> >>> So, check dev->data->rx_queues whether it is NULL will be recommend in >>> close function, only this could avoid unsafe situations for pointer. >>> >>> Thanks, >>> Michael >> Hi Michael, >> >> In most of the pmd's memory is allocated in the dev_init()functions and released in the dev_uninit() functions. The dev_uninit() functions call dev_close(), so either way the memory is released. > Yes, but consider that, without hotplug, users just stop a port and > close it. then what happens? the memory does not released! > So that's why I recommend to release the memory on close() function, it > could be in EAL level like rte_eth_dev_close(). Sorry, here EAL should be rte_ether > > Maybe my understand is wrong, please point me out :) > > Thanks, > Michael > >> The point I am trying to make is that the rx_queue and tx_queue memory is not allocated by the pmd and so it should not be freed by the pmd (please see comments on >> http://dpdk.org/dev/patchwork/patch/5790/) >> The memory is allocated in rte_eth_dev_rx_queue_config() and rte_eth_dev_tx_queue_config(), >> which are both called from rte_eth_dev_configure() which is called by the application (for example test_pmd). So it seems to make sense to free this memory in rte_eth_dev_uninit(). >> >> Regards, >> >> Bernard. >> >> >> >
diff --git a/drivers/net/fm10k/fm10k_ethdev.c b/drivers/net/fm10k/fm10k_ethdev.c index 406c350..eba7228 100644 --- a/drivers/net/fm10k/fm10k_ethdev.c +++ b/drivers/net/fm10k/fm10k_ethdev.c @@ -65,6 +65,8 @@ static void fm10k_MAC_filter_set(struct rte_eth_dev *dev, const u8 *mac, bool add); static void fm10k_MACVLAN_remove_all(struct rte_eth_dev *dev); +static void fm10k_tx_queue_release(void *queue); +static void fm10k_rx_queue_release(void *queue); static void fm10k_mbx_initlock(struct fm10k_hw *hw) @@ -809,11 +811,37 @@ fm10k_dev_stop(struct rte_eth_dev *dev) PMD_INIT_FUNC_TRACE(); - for (i = 0; i < dev->data->nb_tx_queues; i++) - fm10k_dev_tx_queue_stop(dev, i); + if (dev->data->tx_queues) + for (i = 0; i < dev->data->nb_tx_queues; i++) + fm10k_dev_tx_queue_stop(dev, i); - for (i = 0; i < dev->data->nb_rx_queues; i++) - fm10k_dev_rx_queue_stop(dev, i); + if (dev->data->rx_queues) + for (i = 0; i < dev->data->nb_rx_queues; i++) + fm10k_dev_rx_queue_stop(dev, i); +} + +static void +fm10k_dev_queue_release(struct rte_eth_dev *dev) +{ + int i; + + PMD_INIT_FUNC_TRACE(); + + if (dev->data->tx_queues) { + for (i = 0; i < dev->data->nb_tx_queues; i++) + fm10k_tx_queue_release(dev->data->tx_queues[i]); + rte_free(dev->data->tx_queues); + dev->data->tx_queues = NULL; + dev->data->nb_tx_queues = 0; + } + + if (dev->data->rx_queues) { + for (i = 0; i < dev->data->nb_rx_queues; i++) + fm10k_rx_queue_release(dev->data->rx_queues[i]); + rte_free(dev->data->rx_queues); + dev->data->rx_queues = NULL; + dev->data->nb_rx_queues = 0; + } } static void @@ -828,6 +856,7 @@ fm10k_dev_close(struct rte_eth_dev *dev) /* Stop mailbox service first */ fm10k_close_mbx_service(hw); fm10k_dev_stop(dev); + fm10k_dev_queue_release(dev); fm10k_stop_hw(hw); }