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 750401B068 for ; Wed, 11 Jul 2018 18:05:33 +0200 (CEST) X-Virus-Scanned: Proofpoint Essentials engine Received: from webmail.solarflare.com (uk.solarflare.com [193.34.186.16]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1-us3.ppe-hosted.com (Proofpoint Essentials ESMTP Server) with ESMTPS id C1178B40075; Wed, 11 Jul 2018 16:05:28 +0000 (UTC) Received: from [192.168.1.16] (85.187.13.33) by ukex01.SolarFlarecom.com (10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1044.25; Wed, 11 Jul 2018 17:05:20 +0100 To: "Zhang, Qi Z" , "thomas@monjalon.net" , "Burakov, Anatoly" CC: "Ananyev, Konstantin" , "dev@dpdk.org" , "Richardson, Bruce" , "Yigit, Ferruh" , "Shelton, Benjamin H" , "Vangati, Narender" References: <20180607123849.14439-1-qi.z.zhang@intel.com> <20180711030917.181098-1-qi.z.zhang@intel.com> <20180711030917.181098-2-qi.z.zhang@intel.com> <0a0c7b4a-3f7d-8b5e-3e76-8b631edd1997@solarflare.com> <039ED4275CED7440929022BC67E7061153258BAE@SHSMSX103.ccr.corp.intel.com> From: Andrew Rybchenko Message-ID: <3a434539-2d48-2539-b970-6823add74d89@solarflare.com> Date: Wed, 11 Jul 2018 19:05:16 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: <039ED4275CED7440929022BC67E7061153258BAE@SHSMSX103.ccr.corp.intel.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US X-Originating-IP: [85.187.13.33] X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To ukex01.SolarFlarecom.com (10.17.10.4) X-TM-AS-Product-Ver: SMEX-11.0.0.1191-8.100.1062-23960.003 X-TM-AS-Result: No--16.447700-0.000000-31 X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-MDID: 1531325129-hJy97WIxWTxX Subject: Re: [dpdk-dev] [PATCH v11 01/19] ethdev: add function to release port in local process 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, 11 Jul 2018 16:05:33 -0000 On 11.07.2018 15:30, Zhang, Qi Z wrote: > >> -----Original Message----- >> From: Andrew Rybchenko [mailto:arybchenko@solarflare.com] >> Sent: Wednesday, July 11, 2018 5:27 PM >> To: Zhang, Qi Z ; thomas@monjalon.net; Burakov, >> Anatoly >> Cc: Ananyev, Konstantin ; dev@dpdk.org; >> Richardson, Bruce ; Yigit, Ferruh >> ; Shelton, Benjamin H >> ; Vangati, Narender >> >> Subject: Re: [dpdk-dev] [PATCH v11 01/19] ethdev: add function to release port >> in local process >> >> On 11.07.2018 06:08, Qi Zhang wrote: >>> Add driver API rte_eth_release_port_private to support the case when >>> an ethdev need to be detached on a secondary process. >>> Local state is set to unused and shared data will not be reset so the >>> primary process can still use it. >>> >>> Signed-off-by: Qi Zhang >>> Reviewed-by: Andrew Rybchenko >>> Acked-by: Remy Horton >>> --- > <...> >>> + /** >>> + * PCI device can only be globally detached directly by a >>> + * primary process. In secondary process, we only need to >>> + * release port. >>> + */ >>> + if (rte_eal_process_type() != RTE_PROC_PRIMARY) >>> + return rte_eth_dev_release_port_private(eth_dev); >> I've realized that some uninit functions which will not be called anymore in >> secondary processes have check for process type and handling of secondary >> process case. It makes code inconsistent and should be fixed. > Good point, I did a scan and check all the places that rte_eth_dev_pci_generic_remove be involved. > I found only sfc driver (sfc_eth_dev_unit) will call some cleanup on secondary process as below. The patch makes impossible dev_uninit to be executed for secondary process for all cases if rte_eth_dev_pci_generic_remove() is used. However, many drivers still check for process type. Yes, sfc does cleanup, but some drivers return -EPERM, some return 0. In fact it does not matter. It leaves dead code which is really confusing. > > if (rte_eal_process_type() != RTE_PROC_PRIMARY) { > sfc_eth_dev_secondary_clear_ops(dev); > return 0; > } > > But in sfc_eth_dev_secondary_clear_ops > > static void > sfc_eth_dev_secondary_clear_ops(struct rte_eth_dev *dev) > { > dev->dev_ops = NULL; > dev->tx_pkt_burst = NULL; > dev->rx_pkt_burst = NULL; > } > > So my understand is current change is not a problem for all exist drivers. > > Please let me know if I missed something > > Thanks > Qi > >>> + >>> if (dev_uninit) { >>> ret = dev_uninit(eth_dev); >>> if (ret)