From mboxrd@z Thu Jan 1 00:00:00 1970 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 68BE11B60C for ; Wed, 10 Oct 2018 20:02: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-us1.ppe-hosted.com (Proofpoint Essentials ESMTP Server) with ESMTPS id 0AF02100063; Wed, 10 Oct 2018 18:02:15 +0000 (UTC) Received: from [192.168.1.192] (188.242.181.57) by ukex01.SolarFlarecom.com (10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Wed, 10 Oct 2018 19:02:02 +0100 To: Thomas Monjalon CC: , , References: <20180907233929.21950-1-thomas@monjalon.net> <1961538.UcvSjTfz0W@xps> <2227895.hyEOWiVRGC@xps> From: Andrew Rybchenko Message-ID: <32cc4a78-44e7-f9e8-c2b8-3735e00f5322@solarflare.com> Date: Wed, 10 Oct 2018 21:01:41 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <2227895.hyEOWiVRGC@xps> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US X-Originating-IP: [188.242.181.57] 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-24146.003 X-TM-AS-Result: No-19.288800-8.000000-10 X-TMASE-MatchedRID: Jm7Yxmmj9OkOwH4pD14DsPHkpkyUphL9wuIWIvQEbW6vPoND+wakFlPK Q4g0ENBTz0kHebusWqG/OcTZGXKzdinPugGsN3p5bc297PAGtWbhKQh1LCmGBhqB+wKK9uZeyU7 XgTs6W4LuzcUA0q2gBeKOmN63egZIOVzKEd+ERcomEURBmKrZlG803dnzHHML+Cckfm+bb6AomI 3JawtquaRXh45fPtHvOduZmbQPXWnVFtf7bVfns7Sw7varainhKx5ICGp/WtFtw+n+iKWyyDsFp QORr8vEKx+bRc8nrpP0B9yN5j90xNeMRzCcJKBBMIiU395I8H1eu73mFK6GNPYoIVNmrCb9vDLF ASwRRL2ty1K+7wsP/aXRt1+2BH1IhOhmpQI70nmwHK2BMXhNNKcJxWZ5/lR8+yNYYwngrxYUQWS MNammyQyfh8LRV1QUdLY/EQoITct5HAnE5ooegZN3sInxtjDTmyqQJWNsukkY0A95tjAn+2GXI2 D57ZnKMlUQ3EAWuyCN6fDbnTu0n01+zyfzlN7ygxsfzkNRlfKx5amWK2anSPoLR4+zsDTtAqYBE 3k9Mpw= X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10--19.288800-8.000000 X-TMASE-Version: SMEX-12.5.0.1300-8.5.1010-24146.003 X-MDID: 1539194536-v-F5AMPy79TB Subject: Re: [dpdk-dev] [PATCH v2] 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: Wed, 10 Oct 2018 18:02:17 -0000 On 10.10.2018 19:43, Thomas Monjalon wrote: > 10/10/2018 17:01, Andrew Rybchenko: >> 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) > Yes, you missed my explanation above :) > Let me try again: > > rte_eth_dev_release_port() does 3 things: > - RTE_ETH_EVENT_DESTROY notification > - state = RTE_ETH_DEV_UNUSED > - memset data to 0 > > Because of state == RTE_ETH_DEV_UNUSED and data->name == NULL, > you should not try to use data->dev_private. > Before calling uninit function, the dev is retrieved by name: > > ethdev = rte_eth_dev_allocated(ethdev->data->name); > if (!ethdev) > return -ENODEV; > > In our case, it will be -ENODEV, which is a good return when trying > to release a closed port. Yes, it replies on the question why dev_uninit is not called upon device removal after close. But it does not reply on the question what does call dev_uninit before/during dev_close. > Now I am thinking that PMDs could ignore this -ENODEV error: > it is OK to free the rte_device with ethdev port already closed. I'll apply all 4 patch series tomorrow, add printout to dev_uninit, build, run testpmd, do close and check if printout appears. I hope it will reply on my question. I'll come back when I do and have results. >>> - 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.