From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Rybchenko Subject: Re: [PATCH v3 1/2] ethdev: free all common data when releasing port Date: Tue, 16 Oct 2018 15:55:26 +0300 Message-ID: <287a1f00-5dfc-f6bf-ab26-a9e9fac24570@solarflare.com> References: <20180907233929.21950-1-thomas@monjalon.net> <1585953.FJ6NyjtPXU@xps> <118b3e0b-38ac-52bc-1e3c-f8430c4111c0@solarflare.com> <1789121.QrbBs7g2FO@xps> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Cc: , , , "Rahul Lakkireddy" To: Thomas Monjalon Return-path: Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [67.231.154.164]) by dpdk.org (Postfix) with ESMTP id 73F0C2BD3 for ; Tue, 16 Oct 2018 14:56:15 +0200 (CEST) In-Reply-To: <1789121.QrbBs7g2FO@xps> Content-Language: en-GB List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 10/16/18 3:52 PM, Thomas Monjalon wrote: > 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. One more thought that we should document it since it is used by all PMDs and we have really a lot already and it looks like every release cycle we have at least plus one.