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 E924F5F3B for ; Tue, 16 Oct 2018 14:52:35 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 90C0922295; Tue, 16 Oct 2018 08:52:35 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute1.internal (MEProxy); Tue, 16 Oct 2018 08:52:35 -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=37gklHp21c/kpt4lhuRJZtG4kNK8/juLnxQH9Q/JF/s=; b=f9uic3/Qzd+K TOwR9SratuTjvVkArSYanLBNZjU9RmPSTbwD+ldYwPeEviunyMlcRObb+JCv/q8S bzhe32fTaDpoSaqQ8KWN/de7TRQ/06RRQQYlwnYVVeKPlcSN6zK8Mc8BnAKWNTJk OkGL2YxTpbqVrt6A4DH8MpQ6FC4EGGU= 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=37gklHp21c/kpt4lhuRJZtG4kNK8/juLnxQH9Q/JF /s=; b=lHQ6HkTcBmzLT9F7pIwOoqE0dt1kXkNdB7hSDcRpls30tBBfbmCPA04sG WF1BJZyH7n/43ORTDZhJfTWMk+hUFE1luHu/3CIlcslbPpHyywKFCtgS55mH/oxz Nd0paxaAakYlGujs4dCwT+qmNwI0TxYWSDFV3B58JRAlj7I56e5O2Oqdq77shsTJ 1G9aUG3oCBfXHRXhGHszEM7DDAizoQpfH+u8pcAQo4SjmQGbl5YGxcoeL/izNEr1 3x8y85EA+5UQ5/8JsXU+7RyVwRegdUSPE3UfwlhnNRRO7y6Y/FlAC1bf/IdgoF7b 1nZPiq2fJh+8Irch93CEBXVxw/drA== 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 6D00EE4240; Tue, 16 Oct 2018 08:52:34 -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:52:37 +0200 Message-ID: <1789121.QrbBs7g2FO@xps> In-Reply-To: <118b3e0b-38ac-52bc-1e3c-f8430c4111c0@solarflare.com> References: <20180907233929.21950-1-thomas@monjalon.net> <1585953.FJ6NyjtPXU@xps> <118b3e0b-38ac-52bc-1e3c-f8430c4111c0@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:52:36 -0000 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.