From mboxrd@z Thu Jan 1 00:00:00 1970 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 ACC1E2BCD for ; Mon, 20 Aug 2018 10:53:14 +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-us3.ppe-hosted.com (Proofpoint Essentials ESMTP Server) with ESMTPS id 4EF9BB8005A; Mon, 20 Aug 2018 08:53:13 +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.1395.4; Mon, 20 Aug 2018 09:53:04 +0100 To: Qi Zhang , , , CC: , , , , , References: <20180607123849.14439-1-qi.z.zhang@intel.com> <20180816030455.41354-1-qi.z.zhang@intel.com> <20180816030455.41354-2-qi.z.zhang@intel.com> From: Andrew Rybchenko Message-ID: <2e70ce93-b258-a717-a675-0b162c2dac19@solarflare.com> Date: Mon, 20 Aug 2018 11:52:59 +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: <20180816030455.41354-2-qi.z.zhang@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-12.5.0.1300-8.5.1010-24042.003 X-TM-AS-Result: No-19.180200-8.000000-10 X-TMASE-MatchedRID: byfwvk+IcRkOwH4pD14DsPHkpkyUphL9MZm0+sEE9mv7efdnqtsaE+bG U8GB3wTOyDiB4i7QjTmvXn8ZlTtgDErCZPVEfRO5syNb+yeIRApAq6/y5AEOOlAoBBK61Bhcg3a kyfQVy8bU0OGKE76N6DwG5HNbAYX6W/Dpe0fsrIrhuXUWQoMQt35ErfRqpilVh5Q1ArtCPlxweO CJM432Mip6C6A3Owl9e4lfyUtHKEXxlOJuQNHlfc36paW7ZnFo9QPh82z6mRogUEQTkIWiYpG/q oYvgCpn2OsYla+9EV1HM9hrtlR+8wjZsijeXMGWFyqkfsPWu1CQoBr+SFneJBS11FlOYRohj3Qb FpHxze0X88vOXPeLZj7lXQ+mcqjWbMV9cTNo23wJT7rPzEqgu30tCKdnhB58vqq8s2MNhPCy5/t FZu9S3Ku6xVHLhqfxIAcCikR3vq982uHJ/Ae+r4xzZH9Gp4bdUJLK2Ynr9Q4ZB/fh1vZcTD+tDZ VRZouQ X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10--19.180200-8.000000 X-TMASE-Version: SMEX-12.5.0.1300-8.5.1010-24042.003 X-MDID: 1534755194-MRSJFr2cE64O Subject: Re: [dpdk-dev] [PATCH v15 1/7] ethdev: add function to release port in secondary 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: Mon, 20 Aug 2018 08:53:14 -0000 On 16.08.2018 06:04, Qi Zhang wrote: > Add driver API rte_eth_release_port_secondary 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. There are few questions below, but in general I'm really puzzled after looking at variety of release, destroy etc functions and how call chains look like. IMHO, introduction of the function is really wrong direction. It duplicates part of rte_eth_dev_release_port() functionality, it will complicate maintenance since it will be required to remember to find and update both. Also it looks like it already has bugs (missing init of shared data, missing lock). I would prefer to update rte_eth_dev_release_port() to make it secondary process aware. > Signed-off-by: Qi Zhang > --- > lib/librte_ethdev/rte_ethdev.c | 17 +++++++++++++++-- > lib/librte_ethdev/rte_ethdev_driver.h | 16 +++++++++++++++- > lib/librte_ethdev/rte_ethdev_pci.h | 10 ++++++++-- > lib/librte_ethdev/rte_ethdev_version.map | 7 +++++++ > 4 files changed, 45 insertions(+), 5 deletions(-) > > diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c > index 4c3202505..1a1cc1125 100644 > --- a/lib/librte_ethdev/rte_ethdev.c > +++ b/lib/librte_ethdev/rte_ethdev.c > @@ -359,6 +359,18 @@ rte_eth_dev_attach_secondary(const char *name) > } > > int > +rte_eth_dev_release_port_secondary(struct rte_eth_dev *eth_dev) > +{ > + if (eth_dev == NULL) > + return -EINVAL; > + > + _rte_eth_dev_callback_process(eth_dev, RTE_ETH_EVENT_DESTROY, NULL); > + eth_dev->state = RTE_ETH_DEV_UNUSED; rte_eth_dev_release_port() does it under ownership lock. Why is lock not required here? > + > + return 0; > +} > + > +int > rte_eth_dev_release_port(struct rte_eth_dev *eth_dev) > { > if (eth_dev == NULL) > @@ -3532,9 +3544,10 @@ rte_eth_dev_destroy(struct rte_eth_dev *ethdev, > return ret; > } > > - if (rte_eal_process_type() == RTE_PROC_PRIMARY) > - rte_free(ethdev->data->dev_private); > + if (rte_eal_process_type() != RTE_PROC_PRIMARY) > + return rte_eth_dev_release_port_secondary(ethdev); > > + rte_free(ethdev->data->dev_private); > ethdev->data->dev_private = NULL; > > return rte_eth_dev_release_port(ethdev); > diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h > index c6d9bc1a3..8fe82d2ab 100644 > --- a/lib/librte_ethdev/rte_ethdev_driver.h > +++ b/lib/librte_ethdev/rte_ethdev_driver.h > @@ -61,7 +61,7 @@ struct rte_eth_dev *rte_eth_dev_attach_secondary(const char *name); > * Release the specified ethdev port. > * > * @param eth_dev > - * The *eth_dev* pointer is the address of the *rte_eth_dev* structure. > + * Device to be detached. > * @return > * - 0 on success, negative on error > */ > @@ -69,6 +69,20 @@ int rte_eth_dev_release_port(struct rte_eth_dev *eth_dev); > > /** > * @internal > + * Release the specified ethdev port in the local process. > + * Only set ethdev state to unused, but not reset shared data since > + * it assume other processes is still using it. typically it is > + * called by a secondary process. > + * > + * @param eth_dev > + * Device to be detached. > + * @return > + * - 0 on success, negative on error > + */ > +int rte_eth_dev_release_port_secondary(struct rte_eth_dev *eth_dev); > + > +/** > + * @internal > * Release device queues and clear its configuration to force the user > * application to reconfigure it. It is for internal use only. > * > diff --git a/lib/librte_ethdev/rte_ethdev_pci.h b/lib/librte_ethdev/rte_ethdev_pci.h > index f652596f4..70d2d2503 100644 > --- a/lib/librte_ethdev/rte_ethdev_pci.h > +++ b/lib/librte_ethdev/rte_ethdev_pci.h > @@ -135,9 +135,15 @@ rte_eth_dev_pci_allocate(struct rte_pci_device *dev, size_t private_data_size) > static inline void > rte_eth_dev_pci_release(struct rte_eth_dev *eth_dev) > { > - if (rte_eal_process_type() == RTE_PROC_PRIMARY) > - rte_free(eth_dev->data->dev_private); > + if (rte_eal_process_type() != RTE_PROC_PRIMARY) { > + eth_dev->device = NULL; > + eth_dev->intr_handle = NULL; Why are above two assignments done here in the PCI device release, but not included in rte_eth_dev_release_port_secondary()? > + rte_eth_dev_release_port_secondary(eth_dev); > + return; > + } > > + /* primary process */ > + rte_free(eth_dev->data->dev_private); > eth_dev->data->dev_private = NULL; > > /* > diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map > index 38f117f01..acc407f86 100644 > --- a/lib/librte_ethdev/rte_ethdev_version.map > +++ b/lib/librte_ethdev/rte_ethdev_version.map > @@ -220,6 +220,13 @@ DPDK_18.08 { > > } DPDK_18.05; > > +DPDK_18.11 { > + global: > + > + rte_eth_dev_release_port_secondary; > + > +} DPDK_18.08; Shouldn't it be experimental? > + > EXPERIMENTAL { > global: >