[dpdk-dev] [PATCH] librte_ether: release memory in uninit function.
> -Original Message-
> From: Ananyev, Konstantin
> Sent: Thursday, June 25, 2015 7:33 PM
> To: Iremonger, Bernard; dev at dpdk.org
> Cc: Zhang, Helin; Qiu, Michael; mukawa at 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 at dpdk.org
> > Cc: Zhang, Helin; Ananyev, Konstantin; Qiu, Michael;
> > mukawa at igel.co.jp; Iremonger, Bernard
> > Subject: [PATCH] librte_ether: release memory in uninit function.
> >
> >
> > Signed-off-by: Bernard Iremonger
> > ---
> > 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
[dpdk-dev] [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 at dpdk.org
> Cc: Zhang, Helin; Ananyev, Konstantin; Qiu, Michael; mukawa at igel.co.jp;
> Iremonger, Bernard
> Subject: [PATCH] librte_ether: release memory in uninit function.
>
>
> Signed-off-by: Bernard Iremonger
> ---
> 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
[dpdk-dev] [PATCH] librte_ether: release memory in uninit function.
Signed-off-by: Bernard Iremonger
---
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));
+ }
eth_dev->pci_dev = NULL;
eth_dev->driver = NULL;
--
1.7.4.1
[dpdk-dev] [PATCH] librte_ether: release memory in uninit function.
On Thu, 25 Jun 2015 15:30:28 +0100
Bernard Iremonger wrote:
> Signed-off-by: Bernard Iremonger
> ---
> 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.
