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 EAE9A5B32 for ; Tue, 16 Oct 2018 14:25:30 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 894F2223DF; Tue, 16 Oct 2018 08:25:30 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute1.internal (MEProxy); Tue, 16 Oct 2018 08:25:30 -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=hLEgZQMJwY8GacXhbYkkoruKvzjIaGlvviJQl1GsPyE=; b=G0TjvTYH3ouJ Vp6JYuycWe2qxXDDvga7sOwNV2USlzVdzCMhOrBjLdmEwyqAkTS5h1Ov7YUp3foT m5ztuQH2P1STXp3mOF026xscKzZbBW2GKQMyK9NUsBnBpB3417/W+NXQ9CW9v9Wz dUbXQSRLeAFKu6pGsAFcnu/jEZkL9rQ= 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=hLEgZQMJwY8GacXhbYkkoruKvzjIaGlvviJQl1GsP yE=; b=Bie501+5dwIdasez5FY+XCvemDGc7ofQmvfcm8FnexXjGy3PH/pyIUfFa jPeyBLBfkv1hGNPV6teFvZo8ykfBZvVUbtSqNya6ALdAlHOAqGQPQE+z0ZgioePk nAcmXgPIl/wXvUhAg/zwaybM4dLE5ST6kUtk3XpKSTQZGoabZNRsRX4G7LkENWOw N/MxXkt5xOtsLxs6ncgMbxBuLRwj5Dri9hr3IjG0XzKBRXQ5Qe2G0vltZ2yrvkRx uS81E7vwOSqCWXLQlhi2CB89+g0f579YOFo3NQjioNbOJBya+umTnpTvyy2AGhdL UK2eTLSbPAxOA2x6OyzBbuy6WXqSg== 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 361D5102DD; Tue, 16 Oct 2018 08:25:29 -0400 (EDT) From: Thomas Monjalon To: Andrew Rybchenko Cc: ferruh.yigit@intel.com, dev@dpdk.org, ophirmu@mellanox.com Date: Tue, 16 Oct 2018 14:25:31 +0200 Message-ID: <3636040.QE9XrHhWxf@xps> In-Reply-To: <96ba9604-6cc1-6716-c783-facb44161117@solarflare.com> References: <20180907233929.21950-1-thomas@monjalon.net> <20181014232020.12114-3-thomas@monjalon.net> <96ba9604-6cc1-6716-c783-facb44161117@solarflare.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH v3 2/2] ethdev: complete closing of 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:25:31 -0000 16/10/2018 13:24, Andrew Rybchenko: > On 10/15/18 2:20 AM, Thomas Monjalon wrote: > > After closing a port, it cannot be restarted. > > So there is no reason to not free all associated resources. > > > > The last step was done with rte_eth_dev_detach() which is deprecated. > > Instead of blindly removing the associated rte_device, the driver should > > check if no more port (ethdev, cryptodev, etc) is open for the device. > > > > The last ethdev freeing which were done by rte_eth_dev_detach(), > > are now done at the end of rte_eth_dev_close(). > > > > If the driver is trying to free the port again, the function > > rte_eth_dev_release_port() will abort with -ENODEV error. > > > > Signed-off-by: Thomas Monjalon > > --- > > drivers/net/bnxt/bnxt_ethdev.c | 4 ---- > > drivers/net/vhost/rte_eth_vhost.c | 4 ---- > > lib/librte_ethdev/rte_ethdev.c | 9 +++------ > > lib/librte_ethdev/rte_ethdev.h | 3 +-- > > 4 files changed, 4 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c > > index ff41cb0fa..183b40821 100644 > > --- a/drivers/net/bnxt/bnxt_ethdev.c > > +++ b/drivers/net/bnxt/bnxt_ethdev.c > > @@ -712,10 +712,6 @@ static void bnxt_dev_close_op(struct rte_eth_dev *eth_dev) > > if (bp->dev_stopped == 0) > > bnxt_dev_stop_op(eth_dev); > > > > - if (eth_dev->data->mac_addrs != NULL) { > > - rte_free(eth_dev->data->mac_addrs); > > - eth_dev->data->mac_addrs = NULL; > > - } > > if (bp->grp_info != NULL) { > > rte_free(bp->grp_info); > > bp->grp_info = NULL; > > diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c > > index 986bf9633..b9cefbbe5 100644 > > --- a/drivers/net/vhost/rte_eth_vhost.c > > +++ b/drivers/net/vhost/rte_eth_vhost.c > > @@ -998,12 +998,8 @@ eth_dev_close(struct rte_eth_dev *dev) > > for (i = 0; i < dev->data->nb_tx_queues; i++) > > rte_free(dev->data->tx_queues[i]); > > > > - rte_free(dev->data->mac_addrs); > > free(internal->dev_name); > > free(internal->iface_name); > > - rte_free(internal); > > - > > - dev->data->dev_private = NULL; > > } > > > > static int > > It looks like above snippets should be a part of previous changeset. No, this is related to close function. > > --- a/lib/librte_ethdev/rte_ethdev.c > > +++ b/lib/librte_ethdev/rte_ethdev.c > > @@ -367,6 +367,8 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev) > > { > > if (eth_dev == NULL) > > return -EINVAL; > > + if (eth_dev->state == RTE_ETH_DEV_UNUSED) > > + return -ENODEV; > > > > rte_eth_dev_shared_data_prepare(); > > > > @@ -1384,12 +1386,7 @@ rte_eth_dev_close(uint16_t port_id) > > dev->data->dev_started = 0; > > (*dev->dev_ops->dev_close)(dev); > > > > - dev->data->nb_rx_queues = 0; > > - rte_free(dev->data->rx_queues); > > - dev->data->rx_queues = NULL; > > - dev->data->nb_tx_queues = 0; > > - rte_free(dev->data->tx_queues); > > - dev->data->tx_queues = NULL; > > + rte_eth_dev_release_port(dev); > > } > > > > int > > Right now it introduces resource leak for resources which are > freed by PCI drivers in uninit. No, these resources are freed by rte_eth_dev_release_port.