From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Monjalon Subject: Re: [PATCH v3 1/2] ethdev: free all common data when releasing port Date: Tue, 16 Oct 2018 14:52:37 +0200 Message-ID: <1789121.QrbBs7g2FO@xps> References: <20180907233929.21950-1-thomas@monjalon.net> <1585953.FJ6NyjtPXU@xps> <118b3e0b-38ac-52bc-1e3c-f8430c4111c0@solarflare.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7Bit Cc: ferruh.yigit@intel.com, dev@dpdk.org, ophirmu@mellanox.com, Rahul Lakkireddy To: Andrew Rybchenko Return-path: Received: from out3-smtp.messagingengine.com (out3-smtp.messagingengine.com [66.111.4.27]) by dpdk.org (Postfix) with ESMTP id E924F5F3B for ; Tue, 16 Oct 2018 14:52:35 +0200 (CEST) In-Reply-To: <118b3e0b-38ac-52bc-1e3c-f8430c4111c0@solarflare.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 16/10/2018 14:47, Andrew Rybchenko: > On 10/16/18 3:22 PM, Thomas Monjalon wrote: > > 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. > > [...] > >> 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? > > It is allocated in rte_eth_dev_create(), rte_eth_vdev_allocate(), > rte_eth_dev_pci_allocate() and some PMDs. I can't say that I'm happy > with it, but it could be really required. May be it should be default > behaviour and if PMD wants override it, just free before and > set dev_private to NULL. Yes, you're right! So freeing is done by default in rte_eth_dev_release_port(), and PMD can avoid it by setting fields to NULL before calling it. In this case, I just need to double check the MAC freeing, and set NULL in some PMDs which do not allocate MAC separately.