[dpdk-dev] librte_ether: release memory in uninit function.
diff mbox

Message ID 1435242628-8619-1-git-send-email-bernard.iremonger@intel.com
State Superseded, archived
Headers show

Commit Message

Iremonger, Bernard June 25, 2015, 2:30 p.m. UTC
Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
---
 lib/librte_ether/rte_ethdev.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

Comments

Stephen Hemminger June 25, 2015, 2:41 p.m. UTC | #1
On Thu, 25 Jun 2015 15:30:28 +0100
Bernard Iremonger <bernard.iremonger@intel.com> wrote:

> Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
> ---
>  lib/librte_ether/rte_ethdev.c |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
> 
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index e13fde5..2404556 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -369,8 +369,14 @@ rte_eth_dev_uninit(struct rte_pci_device *pci_dev)
>  	/* free ether device */
>  	rte_eth_dev_release_port(eth_dev);
>  
> -	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
> +	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> +		rte_free(eth_dev->data->rx_queues);
> +		rte_free(eth_dev->data->tx_queues);
>  		rte_free(eth_dev->data->dev_private);
> +		rte_free(eth_dev->data->mac_addrs);
> +		rte_free(eth_dev->data->hash_mac_addrs);
> +		memset(eth_dev->data, 0, sizeof(struct rte_eth_dev_data));


Glad to see this problem addressed.

I would prefer that the component that created the object be responsible
for doing its own cleanup.
Ananyev, Konstantin June 25, 2015, 6:32 p.m. UTC | #2
Hi Bernard,

> -----Original Message-----
> From: Iremonger, Bernard
> Sent: Thursday, June 25, 2015 3:30 PM
> To: dev@dpdk.org
> Cc: Zhang, Helin; Ananyev, Konstantin; Qiu, Michael; mukawa@igel.co.jp; Iremonger, Bernard
> Subject: [PATCH] librte_ether: release memory in uninit function.
> 
> 
> Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
> ---
>  lib/librte_ether/rte_ethdev.c |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
> 
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index e13fde5..2404556 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -369,8 +369,14 @@ rte_eth_dev_uninit(struct rte_pci_device *pci_dev)
>  	/* free ether device */
>  	rte_eth_dev_release_port(eth_dev);
> 
> -	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
> +	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> +		rte_free(eth_dev->data->rx_queues);
> +		rte_free(eth_dev->data->tx_queues);
>  		rte_free(eth_dev->data->dev_private);
> +		rte_free(eth_dev->data->mac_addrs);
> +		rte_free(eth_dev->data->hash_mac_addrs);

Sorry, but I don't understand why you put last 2 rte_free()s here.
You already do relese mac_addrs and hash_mac_addrs memory at each PMD _uninit routine.
Plus, as Stephen said - it would be better if same component (PMD in that case) would do both alloc and free.
Apart from that, patch looks good to me.

Konstantin 


> +		memset(eth_dev->data, 0, sizeof(struct rte_eth_dev_data));
> +	}
> 
>  	eth_dev->pci_dev = NULL;
>  	eth_dev->driver = NULL;
> --
> 1.7.4.1
Iremonger, Bernard June 26, 2015, 8:17 a.m. UTC | #3
> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Thursday, June 25, 2015 7:33 PM
> To: Iremonger, Bernard; dev@dpdk.org
> Cc: Zhang, Helin; Qiu, Michael; mukawa@igel.co.jp
> Subject: RE: [PATCH] librte_ether: release memory in uninit function.
> 
> Hi Bernard,
> 
> > -----Original Message-----
> > From: Iremonger, Bernard
> > Sent: Thursday, June 25, 2015 3:30 PM
> > To: dev@dpdk.org
> > Cc: Zhang, Helin; Ananyev, Konstantin; Qiu, Michael;
> > mukawa@igel.co.jp; Iremonger, Bernard
> > Subject: [PATCH] librte_ether: release memory in uninit function.
> >
> >
> > Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
> > ---
> >  lib/librte_ether/rte_ethdev.c |    8 +++++++-
> >  1 files changed, 7 insertions(+), 1 deletions(-)
> >
> > diff --git a/lib/librte_ether/rte_ethdev.c
> > b/lib/librte_ether/rte_ethdev.c index e13fde5..2404556 100644
> > --- a/lib/librte_ether/rte_ethdev.c
> > +++ b/lib/librte_ether/rte_ethdev.c
> > @@ -369,8 +369,14 @@ rte_eth_dev_uninit(struct rte_pci_device
> *pci_dev)
> >  	/* free ether device */
> >  	rte_eth_dev_release_port(eth_dev);
> >
> > -	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
> > +	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> > +		rte_free(eth_dev->data->rx_queues);
> > +		rte_free(eth_dev->data->tx_queues);
> >  		rte_free(eth_dev->data->dev_private);
> > +		rte_free(eth_dev->data->mac_addrs);
> > +		rte_free(eth_dev->data->hash_mac_addrs);
> 
> Sorry, but I don't understand why you put last 2 rte_free()s here.
> You already do relese mac_addrs and hash_mac_addrs memory at each PMD
> _uninit routine.
> Plus, as Stephen said - it would be better if same component (PMD in that
> case) would do both alloc and free.
> Apart from that, patch looks good to me.
> 
> Konstantin

Hi Konstantin,

I thought it might be safer to free the memory here rather than relying on the PMD to do it.
It is probably better if the PMD who does the alloc also does the free.

I will send a v2 of the patch.

Regards,

Bernard.

> 
> 
> > +		memset(eth_dev->data, 0, sizeof(struct
> rte_eth_dev_data));
> > +	}
> >
> >  	eth_dev->pci_dev = NULL;
> >  	eth_dev->driver = NULL;
> > --
> > 1.7.4.1

Patch
diff mbox

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index e13fde5..2404556 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -369,8 +369,14 @@  rte_eth_dev_uninit(struct rte_pci_device *pci_dev)
 	/* free ether device */
 	rte_eth_dev_release_port(eth_dev);
 
-	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+		rte_free(eth_dev->data->rx_queues);
+		rte_free(eth_dev->data->tx_queues);
 		rte_free(eth_dev->data->dev_private);
+		rte_free(eth_dev->data->mac_addrs);
+		rte_free(eth_dev->data->hash_mac_addrs);
+		memset(eth_dev->data, 0, sizeof(struct rte_eth_dev_data));
+	}
 
 	eth_dev->pci_dev = NULL;
 	eth_dev->driver = NULL;