From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from out3-smtp.messagingengine.com (out3-smtp.messagingengine.com [66.111.4.27]) by dpdk.org (Postfix) with ESMTP id 11BBD5F11 for ; Tue, 16 Oct 2018 14:22:42 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id A487822347; Tue, 16 Oct 2018 08:22:41 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute1.internal (MEProxy); Tue, 16 Oct 2018 08:22:41 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding:content-type; s=mesmtp; bh=fXLRae6ivBTzwQEc7tAda6/x4Agn8jvn7jjU3LQtxK4=; b=IvdBak8dv/Kj 0uie5e8yOBce4820nMhu5FaTMlic6bkwjfvOdBE9f1nHiq3ZExcKMjUL3aMVwRhc FxK4zyM3ZdKnMJU7l5IMnOnsCRP66dA+j1Swix+UpeSXbna/DV6f4z8DM3B7cy3B zzi42deL4KctPTk/wIbMzfHsHFHgoVs= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm1; bh=fXLRae6ivBTzwQEc7tAda6/x4Agn8jvn7jjU3LQtx K4=; b=C+N8Egrbpp/LQHrjFdFH1NoSPSVEUBuYEJ5BPuPh8mgxcZiEhV8hA9GcD Fql7iplwLl6SujkSdPmRGjXjp77IvIqIAGHuIvZfTHRZe9Ire6qLFcKm40ShsVLQ Baxh12vYVLye8tn2aOgJtnEOgTYbORlCxDHFM8xrEY87yGEObqFkA8Hw7RQDhGIU ZGwS4rhD3DhcjjKIFGdjAqAiAR/z1fPvTocEnXsaazHZpbgbKhnaxCLu4cAWSThh Z/fPU1gtwlEGXyLKQNBygu3AMvEGbhQPk0oykJ/15tMO1mpyO03DW8MGmzX2t4SP L9JfykAhCaH+sZ9G52r39mHSpOdLA== X-ME-Sender: X-ME-Proxy: Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id 875CFE43AC; Tue, 16 Oct 2018 08:22:40 -0400 (EDT) From: Thomas Monjalon To: Andrew Rybchenko Cc: ferruh.yigit@intel.com, dev@dpdk.org, ophirmu@mellanox.com, Rahul Lakkireddy Date: Tue, 16 Oct 2018 14:22:43 +0200 Message-ID: <1585953.FJ6NyjtPXU@xps> In-Reply-To: <95172f2a-41d4-aadf-65b0-0e4c57fca826@solarflare.com> References: <20180907233929.21950-1-thomas@monjalon.net> <20181014232020.12114-2-thomas@monjalon.net> <95172f2a-41d4-aadf-65b0-0e4c57fca826@solarflare.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH v3 1/2] ethdev: free all common data when releasing port X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 16 Oct 2018 12:22:42 -0000 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?