From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 0AF6B45E61; Tue, 10 Dec 2024 02:50:18 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C71564029B; Tue, 10 Dec 2024 02:50:17 +0100 (CET) Received: from szxga05-in.huawei.com (szxga05-in.huawei.com [45.249.212.191]) by mails.dpdk.org (Postfix) with ESMTP id 5D17B40264 for ; Tue, 10 Dec 2024 02:50:15 +0100 (CET) Received: from mail.maildlp.com (unknown [172.19.163.17]) by szxga05-in.huawei.com (SkyGuard) with ESMTP id 4Y6hWC3Zwxz1JF04; Tue, 10 Dec 2024 09:49:59 +0800 (CST) Received: from dggemv704-chm.china.huawei.com (unknown [10.3.19.47]) by mail.maildlp.com (Postfix) with ESMTPS id 2A1081A0188; Tue, 10 Dec 2024 09:50:13 +0800 (CST) Received: from kwepemn100009.china.huawei.com (7.202.194.112) by dggemv704-chm.china.huawei.com (10.3.19.47) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.39; Tue, 10 Dec 2024 09:50:12 +0800 Received: from [10.67.121.59] (10.67.121.59) by kwepemn100009.china.huawei.com (7.202.194.112) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1544.11; Tue, 10 Dec 2024 09:50:12 +0800 Message-ID: <7319082e-10ff-000e-abdf-e6d0d3a6549e@huawei.com> Date: Tue, 10 Dec 2024 09:50:06 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 Subject: Re: [PATCH RESEND v7 2/5] ethdev: fix skip valid port in probing callback To: , , Stephen Hemminger CC: , , , , Somnath Kotur , Ajit Khaparde , Dariusz Sosnowski , Suanming Mou , Matan Azrad , Ori Kam , Viacheslav Ovsiienko References: <20220825024425.10534-1-lihuisong@huawei.com> <20240929055241.29268-1-lihuisong@huawei.com> <20240929055241.29268-3-lihuisong@huawei.com> From: "lihuisong (C)" In-Reply-To: <20240929055241.29268-3-lihuisong@huawei.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.121.59] X-ClientProxiedBy: dggems701-chm.china.huawei.com (10.3.19.178) To kwepemn100009.china.huawei.com (7.202.194.112) X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Hi Ferruh, Stephen and Thomas, Can you take a look at this patch? After all, it is an issue in ethdev layer. This also is the fruit we disscussed with Thomas and Ferruh before. Please go back to this thread. If we don't need this patch, please let me know. I will drop it from my upstreaming list. /Huisong 在 2024/9/29 13:52, Huisong Li 写道: > The event callback in application may use the macro RTE_ETH_FOREACH_DEV to > iterate over all enabled ports to do something(like, verifying the port id > validity) when receive a probing event. If the ethdev state of a port is > not RTE_ETH_DEV_UNUSED, this port will be considered as a valid port. > > However, this state is set to RTE_ETH_DEV_ATTACHED after pushing probing > event. It means that probing callback will skip this port. But this > assignment can not move to front of probing notification. See > commit be8cd210379a ("ethdev: fix port probing notification") > > So this patch has to add a new state, RTE_ETH_DEV_ALLOCATED. Set the ethdev > state to RTE_ETH_DEV_ALLOCATED before pushing probing event and set it to > RTE_ETH_DEV_ATTACHED after definitely probed. And this port is valid if its > device state is 'ALLOCATED' or 'ATTACHED'. > > In addition, the new state has to be placed behind 'REMOVED' to avoid ABI > break. Fortunately, this ethdev state is internal and applications can not > access it directly. So this patch encapsulates an API, rte_eth_dev_is_used, > for ethdev or PMD to call and eliminate concerns about using this state > enum value comparison. > > Fixes: be8cd210379a ("ethdev: fix port probing notification") > Cc: stable@dpdk.org > > Signed-off-by: Huisong Li > Acked-by: Chengwen Feng > --- > drivers/net/bnxt/bnxt_ethdev.c | 3 ++- > drivers/net/mlx5/mlx5.c | 2 +- > lib/ethdev/ethdev_driver.c | 13 ++++++++++--- > lib/ethdev/ethdev_driver.h | 12 ++++++++++++ > lib/ethdev/ethdev_pci.h | 2 +- > lib/ethdev/rte_class_eth.c | 2 +- > lib/ethdev/rte_ethdev.c | 4 ++-- > lib/ethdev/rte_ethdev.h | 4 +++- > lib/ethdev/version.map | 1 + > 9 files changed, 33 insertions(+), 10 deletions(-) > > diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c > index c6ad764813..7401dcd8b5 100644 > --- a/drivers/net/bnxt/bnxt_ethdev.c > +++ b/drivers/net/bnxt/bnxt_ethdev.c > @@ -6612,7 +6612,8 @@ bnxt_dev_uninit(struct rte_eth_dev *eth_dev) > > PMD_DRV_LOG(DEBUG, "Calling Device uninit\n"); > > - if (eth_dev->state != RTE_ETH_DEV_UNUSED) > + > + if (rte_eth_dev_is_used(eth_dev->state)) > bnxt_dev_close_op(eth_dev); > > return 0; > diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c > index 8d266b0e64..0df49e1f69 100644 > --- a/drivers/net/mlx5/mlx5.c > +++ b/drivers/net/mlx5/mlx5.c > @@ -3371,7 +3371,7 @@ mlx5_eth_find_next(uint16_t port_id, struct rte_device *odev) > while (port_id < RTE_MAX_ETHPORTS) { > struct rte_eth_dev *dev = &rte_eth_devices[port_id]; > > - if (dev->state != RTE_ETH_DEV_UNUSED && > + if (rte_eth_dev_is_used(dev->state) && > dev->device && > (dev->device == odev || > (dev->device->driver && > diff --git a/lib/ethdev/ethdev_driver.c b/lib/ethdev/ethdev_driver.c > index c335a25a82..a87dbb00ff 100644 > --- a/lib/ethdev/ethdev_driver.c > +++ b/lib/ethdev/ethdev_driver.c > @@ -55,8 +55,8 @@ eth_dev_find_free_port(void) > for (i = 0; i < RTE_MAX_ETHPORTS; i++) { > /* Using shared name field to find a free port. */ > if (eth_dev_shared_data->data[i].name[0] == '\0') { > - RTE_ASSERT(rte_eth_devices[i].state == > - RTE_ETH_DEV_UNUSED); > + RTE_ASSERT(!rte_eth_dev_is_used( > + rte_eth_devices[i].state)); > return i; > } > } > @@ -221,11 +221,18 @@ rte_eth_dev_probing_finish(struct rte_eth_dev *dev) > if (rte_eal_process_type() == RTE_PROC_SECONDARY) > eth_dev_fp_ops_setup(rte_eth_fp_ops + dev->data->port_id, dev); > > + dev->state = RTE_ETH_DEV_ALLOCATED; > rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_NEW, NULL); > > dev->state = RTE_ETH_DEV_ATTACHED; > } > > +bool rte_eth_dev_is_used(uint16_t dev_state) > +{ > + return dev_state == RTE_ETH_DEV_ALLOCATED || > + dev_state == RTE_ETH_DEV_ATTACHED; > +} > + > int > rte_eth_dev_release_port(struct rte_eth_dev *eth_dev) > { > @@ -243,7 +250,7 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev) > if (ret != 0) > return ret; > > - if (eth_dev->state != RTE_ETH_DEV_UNUSED) > + if (rte_eth_dev_is_used(eth_dev->state)) > rte_eth_dev_callback_process(eth_dev, > RTE_ETH_EVENT_DESTROY, NULL); > > diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h > index abed4784aa..aa35b65848 100644 > --- a/lib/ethdev/ethdev_driver.h > +++ b/lib/ethdev/ethdev_driver.h > @@ -1704,6 +1704,18 @@ int rte_eth_dev_callback_process(struct rte_eth_dev *dev, > __rte_internal > void rte_eth_dev_probing_finish(struct rte_eth_dev *dev); > > +/** > + * Check if a Ethernet device state is used or not > + * > + * @param dev_state > + * The state of the Ethernet device > + * @return > + * - true if the state of the Ethernet device is allocated or attached > + * - false if this state is neither allocated nor attached > + */ > +__rte_internal > +bool rte_eth_dev_is_used(uint16_t dev_state); > + > /** > * Create memzone for HW rings. > * malloc can't be used as the physical address is needed. > diff --git a/lib/ethdev/ethdev_pci.h b/lib/ethdev/ethdev_pci.h > index ec4f731270..05dec6716b 100644 > --- a/lib/ethdev/ethdev_pci.h > +++ b/lib/ethdev/ethdev_pci.h > @@ -179,7 +179,7 @@ rte_eth_dev_pci_generic_remove(struct rte_pci_device *pci_dev, > * eth device has been released. > */ > if (rte_eal_process_type() == RTE_PROC_SECONDARY && > - eth_dev->state == RTE_ETH_DEV_UNUSED) > + !rte_eth_dev_is_used(eth_dev->state)) > return 0; > > if (dev_uninit) { > diff --git a/lib/ethdev/rte_class_eth.c b/lib/ethdev/rte_class_eth.c > index b52f1dd9f2..81e70670d9 100644 > --- a/lib/ethdev/rte_class_eth.c > +++ b/lib/ethdev/rte_class_eth.c > @@ -118,7 +118,7 @@ eth_dev_match(const struct rte_eth_dev *edev, > const struct rte_kvargs *kvlist = arg->kvlist; > unsigned int pair; > > - if (edev->state == RTE_ETH_DEV_UNUSED) > + if (!rte_eth_dev_is_used(edev->state)) > return -1; > if (arg->device != NULL && arg->device != edev->device) > return -1; > diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c > index a1f7efa913..4dc66abb7b 100644 > --- a/lib/ethdev/rte_ethdev.c > +++ b/lib/ethdev/rte_ethdev.c > @@ -349,7 +349,7 @@ uint16_t > rte_eth_find_next(uint16_t port_id) > { > while (port_id < RTE_MAX_ETHPORTS && > - rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED) > + !rte_eth_dev_is_used(rte_eth_devices[port_id].state)) > port_id++; > > if (port_id >= RTE_MAX_ETHPORTS) > @@ -408,7 +408,7 @@ rte_eth_dev_is_valid_port(uint16_t port_id) > int is_valid; > > if (port_id >= RTE_MAX_ETHPORTS || > - (rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED)) > + !rte_eth_dev_is_used(rte_eth_devices[port_id].state)) > is_valid = 0; > else > is_valid = 1; > diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h > index a9f92006da..9cc37e8cde 100644 > --- a/lib/ethdev/rte_ethdev.h > +++ b/lib/ethdev/rte_ethdev.h > @@ -2083,10 +2083,12 @@ typedef uint16_t (*rte_tx_callback_fn)(uint16_t port_id, uint16_t queue, > enum rte_eth_dev_state { > /** Device is unused before being probed. */ > RTE_ETH_DEV_UNUSED = 0, > - /** Device is attached when allocated in probing. */ > + /** Device is attached when definitely probed. */ > RTE_ETH_DEV_ATTACHED, > /** Device is in removed state when plug-out is detected. */ > RTE_ETH_DEV_REMOVED, > + /** Device is allocated and is set before reporting new event. */ > + RTE_ETH_DEV_ALLOCATED, > }; > > struct rte_eth_dev_sriov { > diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map > index f63dc32aa2..6ecf1ab89d 100644 > --- a/lib/ethdev/version.map > +++ b/lib/ethdev/version.map > @@ -349,6 +349,7 @@ INTERNAL { > rte_eth_dev_get_by_name; > rte_eth_dev_is_rx_hairpin_queue; > rte_eth_dev_is_tx_hairpin_queue; > + rte_eth_dev_is_used; > rte_eth_dev_probing_finish; > rte_eth_dev_release_port; > rte_eth_dev_internal_reset;