From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Rybchenko Subject: Re: [PATCH v2] ethdev: complete closing of port Date: Wed, 10 Oct 2018 18:01:37 +0300 Message-ID: References: <20180907233929.21950-1-thomas@monjalon.net> <7172106.4TAESsr7Yr@xps> <5f013fa6-762c-0d7e-a057-a8225bed7634@solarflare.com> <1961538.UcvSjTfz0W@xps> 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 [67.231.154.164]) by dpdk.org (Postfix) with ESMTP id 57EE91B6E3 for ; Wed, 10 Oct 2018 17:02:27 +0200 (CEST) In-Reply-To: <1961538.UcvSjTfz0W@xps> 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/10/18 11:39 AM, Thomas Monjalon wrote: > 10/10/2018 09:50, Andrew Rybchenko: >> On 10/10/18 10:44 AM, Thomas Monjalon wrote: >>> 10/10/2018 08:15, Andrew Rybchenko: >>>> On 10/10/18 1:17 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 (dev_private and final release), 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 >>>>> --- >>>>> lib/librte_ethdev/rte_ethdev.c | 6 ++++++ >>>>> lib/librte_ethdev/rte_ethdev.h | 3 +-- >>>>> 2 files changed, 7 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c >>>>> index ed83e5954..3062dc711 100644 >>>>> --- a/lib/librte_ethdev/rte_ethdev.c >>>>> +++ b/lib/librte_ethdev/rte_ethdev.c >>>>> @@ -506,6 +506,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(); >>>>> >>>>> @@ -1441,6 +1443,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); >>>> It is used by, for example, PCI device uninit functions. >>>> What does guarantee that uninit is done and we can free the private data. >>> The state of the port is set to UNUSED and the name is NULL. >>> So nobody should try to use it anymore. >>> There are already some checks before calling uninit functions. >>> For instance, in rte_eth_dev_pci_generic_remove(), >>> rte_eth_dev_allocated() will return NULL and won't call uninit function. >> The questions are: >> Is application allowed to call the function? When? >> Who calls uninit in this case? (What does guarantee that uninit is done >> before close) > So far, everything is allowed: > - The application can close a port and remove the rte_device later. If the patch is applied, close frees dev_private which is used by uninit. So, uninit must be done first. Who does it? (it looks like I'm missing something obvious, but still can't find it) > - The application can remove the rte_device and expect the PMD is closing > associated ports. > > In other words, when rte_device is removed, the ports should be closed > by the PMD, except if the application has already closed the ports. > It means ethdev port close is optional, but EAL removal is always required. > The behaviour is not changed. > > If we want to go further, we could change the behaviour of the close op, > by asking the PMD to remove the rte_device automatically if all associated > ports are closed. It would allow the application to manage only ports > at ethdev layer without bothering with low-level EAL management. > We can think about it as a planned change for next releases. > >