From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <arybchenko@solarflare.com>
Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com
[148.163.129.52]) by dpdk.org (Postfix) with ESMTP id 32C2D5911
for <dev@dpdk.org>; Tue, 16 Oct 2018 13:25:17 +0200 (CEST)
X-Virus-Scanned: Proofpoint Essentials engine
Received: from webmail.solarflare.com (uk.solarflare.com [193.34.186.16])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits))
(No client certificate requested)
by mx1-us3.ppe-hosted.com (Proofpoint Essentials ESMTP Server) with ESMTPS id
90A7A600057; Tue, 16 Oct 2018 11:25:15 +0000 (UTC)
Received: from [192.168.38.17] (91.220.146.112) by ukex01.SolarFlarecom.com
(10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Tue, 16 Oct
2018 12:25:10 +0100
To: Thomas Monjalon <thomas@monjalon.net>, <ferruh.yigit@intel.com>
CC: <dev@dpdk.org>, <ophirmu@mellanox.com>
References: <20180907233929.21950-1-thomas@monjalon.net>
<20181014232020.12114-1-thomas@monjalon.net>
<20181014232020.12114-3-thomas@monjalon.net>
From: Andrew Rybchenko <arybchenko@solarflare.com>
Message-ID: <96ba9604-6cc1-6716-c783-facb44161117@solarflare.com>
Date: Tue, 16 Oct 2018 14:24:30 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
Thunderbird/60.0
MIME-Version: 1.0
In-Reply-To: <20181014232020.12114-3-thomas@monjalon.net>
Content-Language: en-GB
X-Originating-IP: [91.220.146.112]
X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To
ukex01.SolarFlarecom.com (10.17.10.4)
X-TM-AS-Product-Ver: SMEX-12.5.0.1300-8.5.1010-24158.003
X-TM-AS-Result: No-10.672400-8.000000-10
X-TMASE-MatchedRID: eVEkOcJu0F4OwH4pD14DsPHkpkyUphL9wuIWIvQEbW6vPoND+wakFlPK
Q4g0ENBTz0kHebusWqG/OcTZGXKzdinPugGsN3p5bc297PAGtWbhKQh1LCmGBhqB+wKK9uZeyU7
XgTs6W4LuzcUA0q2gBeKOmN63egZIkKjL2IOi2LAc9jA4mLo8ueWNJG9IamrcvhQEQJhzwqaTrr
+C1WNmxaov67Z3Hm/vc8vcp0oWEzxy8WyFAksD+Gg4D2QV/2zL6r3HCixfuKcML9Wb3Qh/hVTNq
f3EQ8oxYnP+cq2mFg5XmxqYx7OZlsZXUijhl2l8GQlUVP2iPcwtlZY1WAfRjdmGo8tT/VtV3SlE
gVuyN9tvuYnoduGWEZGTpe1iiCJq71zr0FZRMbBGONWF/6P/CnTKzu8nNCP8vJqH1oJhQIeZw2M
2094eASGCrUN/b07Ynyd90O9+9FMznLy3yHoy2XFwQTliYmuA8cS7Zo364Bif641z1zZHC0HzTy
ciIli1CCtyZVduLNE=
X-TM-AS-User-Approved-Sender: Yes
X-TM-AS-User-Blocked-Sender: No
X-TMASE-Result: 10--10.672400-8.000000
X-TMASE-Version: SMEX-12.5.0.1300-8.5.1010-24158.003
X-MDID: 1539689116-y6dHKBClsaVa
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 7bit
X-Content-Filtered-By: Mailman/MimeDel 2.1.15
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Tue, 16 Oct 2018 11:25:17 -0000
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 <thomas@monjalon.net>
> ---
> 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.
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 178800a5b..987ba5ab1 100644
> --- 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.