From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <ferruh.yigit@intel.com>
Received: from mga06.intel.com (mga06.intel.com [134.134.136.31])
by dpdk.org (Postfix) with ESMTP id 86B251B429
for <dev@dpdk.org>; Fri, 28 Sep 2018 14:47:02 +0200 (CEST)
X-Amp-Result: SKIPPED(no attachment in message)
X-Amp-File-Uploaded: False
Received: from orsmga005.jf.intel.com ([10.7.209.41])
by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;
28 Sep 2018 05:47:01 -0700
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.54,315,1534834800"; d="scan'208";a="261208014"
Received: from fyigit-mobl.ger.corp.intel.com (HELO [10.252.3.51])
([10.252.3.51])
by orsmga005.jf.intel.com with ESMTP; 28 Sep 2018 05:46:10 -0700
To: Thomas Monjalon <thomas@monjalon.net>, arybchenko@solarflare.com
Cc: dev@dpdk.org
References: <20180907233929.21950-1-thomas@monjalon.net>
From: Ferruh Yigit <ferruh.yigit@intel.com>
Openpgp: preference=signencrypt
Message-ID: <e6cc3b3a-bae7-3946-534a-78603bd488cb@intel.com>
Date: Fri, 28 Sep 2018 13:46:09 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
Thunderbird/52.9.1
MIME-Version: 1.0
In-Reply-To: <20180907233929.21950-1-thomas@monjalon.net>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Subject: Re: [dpdk-dev] [RFC] ethdev: complete closing to free all resources
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: Fri, 28 Sep 2018 12:47:03 -0000
On 9/8/2018 12:39 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 removing the associated rte_device, the driver should check
> if no more port (ethdev, cryptodev, etc) is still open for the device.
> Then the device resources can be freed by the driver inside the
> dev_close() driver callback operation.
>
> The last ethdev freeing (dev_private and final release), which were done
> by rte_eth_dev_detach(), are now done at the end of rte_eth_dev_close().
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
> This patch contains only the change in the close function as RFC.
>
> This idea was presented at Dublin during the "hotplug talk".
> ---
> lib/librte_ethdev/rte_ethdev.c | 5 +++++
> lib/librte_ethdev/rte_ethdev.h | 5 +++--
> 2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 4c3202505..071fcbd23 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -1358,6 +1358,7 @@ void
> rte_eth_dev_close(uint16_t port_id)
> {
> struct rte_eth_dev *dev;
> + struct rte_bus *bus;
>
> RTE_ETH_VALID_PORTID_OR_RET(port_id);
> dev = &rte_eth_devices[port_id];
> @@ -1372,6 +1373,10 @@ rte_eth_dev_close(uint16_t port_id)
> dev->data->nb_tx_queues = 0;
> rte_free(dev->data->tx_queues);
> dev->data->tx_queues = NULL;
> +
> + rte_free(dev->data->dev_private);
> +
> + rte_eth_dev_release_port(dev);
These already done in:
rte_eth_dev_pci_generic_remove()
rte_eth_dev_pci_release()
Perhaps all content of rte_eth_dev_pci_release(), including above updates,
should move to rte_eth_dev_close() and rte_eth_dev_pci_generic_remove() call
rte_eth_dev_close() directly.
Just thinking aloud,
driver->probe() called when a new device added.
Application startup can be thought as all devices added one by one. [Perhaps
this can be change in the future to add devices only when explicitly stated]
driver->remove() called when device removed.
When application terminated this path not called at all, perhaps we need a way
to remove all devices one by one on exit.
eth_dev_close(), when eth_dev removed in ethdev layer but device is not removed
in eal level,
I think it make sense for eth_dev_close():
- It does more cleanup, free resources and port_id
- but it may need to do more clean up, like call uninit() and do more driver
internal clean up too, and clean up the hw.
- so call stack can be:
driver->remove()
rte_eth_dev_pci_generic_remove()
eth_dev_close()
dev_uninit() [driver unint function ]
Another question, after eth_dev_close() ethdev is not usable and not able to
restart it back. So why we need eth_dev_close() in addition to dev remove, why
not directly call rte_eth_dev_detach()?
> }
>
> int
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index 7070e9ab4..37a757a7a 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1797,8 +1797,9 @@ int rte_eth_dev_set_link_down(uint16_t port_id);
>
> /**
> * Close a stopped Ethernet device. The device cannot be restarted!
> - * The function frees all resources except for needed by the
> - * closed state. To free these resources, call rte_eth_dev_detach().
> + * The function frees all port resources.
> + * If there is no more port associated with the underlying device,
> + * the driver should free the device resources.
> *
> * @param port_id
> * The port identifier of the Ethernet device.
>