DPDK patches and discussions
 help / color / mirror / Atom feed
From: Thomas Monjalon <thomas@monjalon.net>
To: Andrew Rybchenko <arybchenko@solarflare.com>
Cc: ferruh.yigit@intel.com, dev@dpdk.org, ophirmu@mellanox.com,
	Rahul Lakkireddy <rahul.lakkireddy@chelsio.com>
Subject: Re: [dpdk-dev] [PATCH v3 1/2] ethdev: free all common data when releasing port
Date: Tue, 16 Oct 2018 14:22:43 +0200	[thread overview]
Message-ID: <1585953.FJ6NyjtPXU@xps> (raw)
In-Reply-To: <95172f2a-41d4-aadf-65b0-0e4c57fca826@solarflare.com>

16/10/2018 13:16, Andrew Rybchenko:
> On 10/15/18 2:20 AM, Thomas Monjalon wrote:
> > This is a clean-up of common ethdev data freeing.
> > All data freeing are moved to rte_eth_dev_release_port()
> > and done only in case of primary process.
> >
> > It is probably fixing some memory leaks for PMDs which were
> > not freeing all data.
[...]
> > --- a/drivers/net/softnic/rte_eth_softnic.c
> > +++ b/drivers/net/softnic/rte_eth_softnic.c
> > @@ -556,7 +556,6 @@ static int
> >   pmd_remove(struct rte_vdev_device *vdev)
> >   {
> >   	struct rte_eth_dev *dev = NULL;
> > -	struct pmd_internals *p;
> >   
> >   	if (!vdev)
> >   		return -EINVAL;
> > @@ -567,12 +566,11 @@ pmd_remove(struct rte_vdev_device *vdev)
> >   	dev = rte_eth_dev_allocated(rte_vdev_device_name(vdev));
> >   	if (dev == NULL)
> >   		return -ENODEV;
> > -	p = dev->data->dev_private;
> >   
> >   	/* Free device data structures*/
> > -	rte_free(dev->data);
> > +	pmd_free(dev->data->dev_private);
> > +	dev->data->dev_private = NULL;
> >   	rte_eth_dev_release_port(dev);
> > -	pmd_free(p);
> >   
> >   	return 0;
> >   }
> 
> The above basically violates approach used everywhere else. It is OK to 
> go, but should be reconsidered.

Yes, it is because pmd_free does more than freeing just dev_private.

[...]
> Not directly related to the patch, but I don't understand why does
> rte_eth_dev_pci_release () exist? It does nothing PCI specific.
> May be just for symmetry to rte_eth_dev_pci_allocate().
> Why is intr_handle set to NULL above? Is it PCI specific?
> It does not look so, since failsafe uses it.

I agree that all these functions must be reconsidered and reworked.
We should consider a big cleanup in ethdev for 19.02.

> In general it looks good, really big work, but ideally it should be
> acked by cxgbe maintainer as well.

Actually, after more thoughts, I think this patch is not correct.
It is freeing MAC adresses in ethdev, even if there was no specific
allocation done for it in the PMD. Only the PMD can know what are the
memory blocks to free.
And it is the same for data->dev_private: are we sure it has been malloc'ed?
Are we sure it has not been allocated as part of a bigger block?
Historically, ethdev was freeing data->dev_private in some cases.
So maybe we can keep this assumption.
Or we can move all data freeing to the PMD?

  reply	other threads:[~2018-10-16 12:22 UTC|newest]

Thread overview: 78+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-07 23:39 [dpdk-dev] [RFC] ethdev: complete closing to free all resources Thomas Monjalon
2018-09-10  8:03 ` Andrew Rybchenko
2018-09-10  8:42   ` Thomas Monjalon
2018-09-10  8:54     ` Andrew Rybchenko
2018-09-12 14:57       ` Thomas Monjalon
2018-09-12 15:44         ` Andrew Rybchenko
2018-09-28 12:46 ` Ferruh Yigit
2018-10-09 22:00   ` Thomas Monjalon
2018-10-09 22:17 ` [dpdk-dev] [PATCH v2] ethdev: complete closing of port Thomas Monjalon
2018-10-10  6:15   ` Andrew Rybchenko
2018-10-10  7:44     ` Thomas Monjalon
2018-10-10  7:50       ` Andrew Rybchenko
2018-10-10  8:39         ` Thomas Monjalon
2018-10-10 15:01           ` Andrew Rybchenko
2018-10-10 16:43             ` Thomas Monjalon
2018-10-10 18:01               ` Andrew Rybchenko
2018-10-10 19:03                 ` Thomas Monjalon
2018-10-14 23:20 ` [dpdk-dev] [PATCH v3 0/2] ethdev port freeing Thomas Monjalon
2018-10-14 23:20   ` [dpdk-dev] [PATCH v3 1/2] ethdev: free all common data when releasing port Thomas Monjalon
2018-10-16 11:16     ` Andrew Rybchenko
2018-10-16 12:22       ` Thomas Monjalon [this message]
2018-10-16 12:47         ` Andrew Rybchenko
2018-10-16 12:52           ` Thomas Monjalon
2018-10-16 12:55             ` Andrew Rybchenko
2018-10-14 23:20   ` [dpdk-dev] [PATCH v3 2/2] ethdev: complete closing of port Thomas Monjalon
2018-10-16 11:24     ` Andrew Rybchenko
2018-10-16 12:25       ` Thomas Monjalon
2018-10-17  1:54 ` [dpdk-dev] [PATCH v3 0/4] ethdev port freeing Thomas Monjalon
2018-10-17  1:54   ` [dpdk-dev] [PATCH v3 1/4] app/testpmd: allow detaching a port not closed Thomas Monjalon
2018-10-17  2:06     ` Thomas Monjalon
2018-10-17  6:26     ` Andrew Rybchenko
2018-10-17  8:20       ` Thomas Monjalon
2018-10-17 10:30         ` Iremonger, Bernard
2018-10-17 11:33           ` Thomas Monjalon
2018-10-17  1:54   ` [dpdk-dev] [PATCH v3 2/4] ethdev: free all common data when releasing port Thomas Monjalon
2018-10-17  7:13     ` Andrew Rybchenko
2018-10-17  8:22       ` Thomas Monjalon
2018-10-17  1:54   ` [dpdk-dev] [PATCH v3 3/4] ethdev: remove release function for secondary process Thomas Monjalon
2018-10-17  7:25     ` Andrew Rybchenko
2018-10-17  9:27       ` Thomas Monjalon
2018-10-17  1:54   ` [dpdk-dev] [PATCH v3 4/4] ethdev: complete closing of port Thomas Monjalon
2018-10-17  2:12     ` Thomas Monjalon
2018-10-17 10:24   ` [dpdk-dev] [PATCH v3 0/4] ethdev port freeing Shreyansh Jain
2018-10-17 11:31     ` Thomas Monjalon
2018-10-18  1:23 ` [dpdk-dev] [PATCH v5 0/6] " Thomas Monjalon
2018-10-18  1:23   ` [dpdk-dev] [PATCH v5 1/6] app/testpmd: fix ports list after removing several at once Thomas Monjalon
2018-10-18 10:40     ` Iremonger, Bernard
2018-10-18 11:29       ` Thomas Monjalon
2018-10-18 11:41         ` Iremonger, Bernard
2018-10-18 14:21           ` Thomas Monjalon
2018-10-18 16:42             ` Iremonger, Bernard
2018-10-18 17:06               ` Thomas Monjalon
2018-10-18 11:49         ` Wisam Monther
2018-10-18 13:22           ` Iremonger, Bernard
2018-10-18  1:23   ` [dpdk-dev] [PATCH v5 2/6] app/testpmd: allow detaching a port not closed Thomas Monjalon
2018-10-18  7:45     ` Andrew Rybchenko
2018-10-18 10:51       ` Iremonger, Bernard
2018-10-18 11:24         ` Thomas Monjalon
2018-10-18  1:23   ` [dpdk-dev] [PATCH v5 3/6] ethdev: fix doxygen comments of shared data fields Thomas Monjalon
2018-10-18  7:11     ` Andrew Rybchenko
2018-10-18  1:24   ` [dpdk-dev] [PATCH v5 4/6] ethdev: free all common data when releasing port Thomas Monjalon
2018-10-18  7:13     ` Andrew Rybchenko
2018-10-18  1:24   ` [dpdk-dev] [PATCH v5 5/6] ethdev: remove release function for secondary process Thomas Monjalon
2018-10-18  7:15     ` Andrew Rybchenko
2018-10-18  1:24   ` [dpdk-dev] [PATCH v5 6/6] ethdev: complete closing of port Thomas Monjalon
2018-10-18  8:33     ` Andrew Rybchenko
2018-10-18  9:32       ` Thomas Monjalon
2018-10-19  2:07 ` [dpdk-dev] [PATCH v6 0/6] ethdev port freeing Thomas Monjalon
2018-10-19  2:07   ` [dpdk-dev] [PATCH v6 1/6] app/testpmd: update port list for multiple removals Thomas Monjalon
2018-10-19 14:32     ` Iremonger, Bernard
2018-10-19  2:07   ` [dpdk-dev] [PATCH v6 2/6] app/testpmd: allow detaching a port not closed Thomas Monjalon
2018-10-19 14:32     ` Iremonger, Bernard
2018-10-19  2:07   ` [dpdk-dev] [PATCH v6 3/6] ethdev: fix doxygen comments of shared data fields Thomas Monjalon
2018-10-19  2:07   ` [dpdk-dev] [PATCH v6 4/6] ethdev: free all common data when releasing port Thomas Monjalon
2018-10-19  2:07   ` [dpdk-dev] [PATCH v6 5/6] ethdev: remove release function for secondary process Thomas Monjalon
2018-10-19  2:07   ` [dpdk-dev] [PATCH v6 6/6] ethdev: complete closing of port Thomas Monjalon
2018-10-19 10:13     ` Andrew Rybchenko
2018-10-22 15:43   ` [dpdk-dev] [PATCH v6 0/6] ethdev port freeing Ferruh Yigit

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=1585953.FJ6NyjtPXU@xps \
    --to=thomas@monjalon.net \
    --cc=arybchenko@solarflare.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=ophirmu@mellanox.com \
    --cc=rahul.lakkireddy@chelsio.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).