From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Rybchenko Subject: Re: [PATCH v3 2/2] ethdev: complete closing of port Date: Tue, 16 Oct 2018 14:24:30 +0300 Message-ID: <96ba9604-6cc1-6716-c783-facb44161117@solarflare.com> References: <20180907233929.21950-1-thomas@monjalon.net> <20181014232020.12114-1-thomas@monjalon.net> <20181014232020.12114-3-thomas@monjalon.net> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Cc: , To: Thomas Monjalon , Return-path: 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 ; Tue, 16 Oct 2018 13:25:17 +0200 (CEST) In-Reply-To: <20181014232020.12114-3-thomas@monjalon.net> 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/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. > 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.