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 B784D423B0; Thu, 12 Jan 2023 05:12:33 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 85DE640A7D; Thu, 12 Jan 2023 05:12:33 +0100 (CET) Received: from szxga03-in.huawei.com (szxga03-in.huawei.com [45.249.212.189]) by mails.dpdk.org (Postfix) with ESMTP id 0EC20400EF for ; Thu, 12 Jan 2023 05:12:31 +0100 (CET) Received: from kwepemm600004.china.huawei.com (unknown [172.30.72.55]) by szxga03-in.huawei.com (SkyGuard) with ESMTP id 4Nsrd024mgzJqGd; Thu, 12 Jan 2023 12:08:20 +0800 (CST) Received: from [10.67.103.231] (10.67.103.231) by kwepemm600004.china.huawei.com (7.193.23.242) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.34; Thu, 12 Jan 2023 12:12:28 +0800 Message-ID: <0e699ace-c2a8-3f81-0cd2-88f9433651b4@huawei.com> Date: Thu, 12 Jan 2023 12:12:27 +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 V4 2/5] ethdev: fix skip valid port in probing callback To: Ferruh Yigit , , CC: , , , References: <20220825024425.10534-1-lihuisong@huawei.com> <20221206092649.8287-1-lihuisong@huawei.com> <20221206092649.8287-3-lihuisong@huawei.com> From: "lihuisong (C)" In-Reply-To: Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.103.231] X-ClientProxiedBy: dggems702-chm.china.huawei.com (10.3.19.179) To kwepemm600004.china.huawei.com (7.193.23.242) X-CFilter-Loop: Reflected 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 在 2023/1/11 20:51, Ferruh Yigit 写道: > On 12/6/2022 9:26 AM, Huisong Li wrote: >> 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 >> RTE_ETH_DEV_UNUSED, this port will be skipped. >> >> Currently, 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") >> > OK to abstract ethdev state check to a function, 'rte_eth_dev_in_used'. > BTW, I guess intention is to say 'is' instead of 'in', like > 'rte_eth_dev_is_used'. Ack > > But not sure if new state is necessary. > I can see from next patches that if testpmd event callback wants to > validate port_id, it is using 'port_id_is_invalid()' function which is > using 'RTE_ETH_FOREACH_DEV' macro. > And using 'RTE_ETH_FOREACH_DEV' macro in callback function has the > problem you described above. > > 1) Do we really need to validate port_id in the event callback, callback > provided 'port_id' from 'rte_eth_dev_callback_process()' API, from > 'dev->data->port_id'. I am not sure if there is case that > 'dev->data->port_id' can be invalid. > > 2) is there any other specific usecase to use 'RTE_ETH_FOREACH_DEV' in > the 'RTE_ETH_EVENT_NEW' event callback function? Reply 1) and 2): this check should be useful for failsafe. > > > >> 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 to >> RTE_ETH_DEV_ATTACHED when definitely probed. And this port is valid if its >> device state is ALLOCATED or ATTACHED. Naturally, a lot of places had to be >> modified for this change. >> >> Fixes: be8cd210379a ("ethdev: fix port probing notification") >> Cc: stable@dpdk.org >> >> Signed-off-by: Huisong Li >> --- >> 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 b3de490d36..7b08353ed5 100644 >> --- a/drivers/net/bnxt/bnxt_ethdev.c >> +++ b/drivers/net/bnxt/bnxt_ethdev.c >> @@ -6003,7 +6003,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_in_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 e55be8720e..949d8c1367 100644 >> --- a/drivers/net/mlx5/mlx5.c >> +++ b/drivers/net/mlx5/mlx5.c >> @@ -3014,7 +3014,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_in_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 0be1e8ca04..7868958dff 100644 >> --- a/lib/ethdev/ethdev_driver.c >> +++ b/lib/ethdev/ethdev_driver.c >> @@ -50,8 +50,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_in_used( >> + rte_eth_devices[i].state)); >> return i; >> } >> } >> @@ -208,11 +208,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_in_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) >> { >> @@ -221,7 +228,7 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev) >> >> eth_dev_shared_data_prepare(); >> >> - if (eth_dev->state != RTE_ETH_DEV_UNUSED) >> + if (rte_eth_dev_in_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 6a550cfc83..e04f88fe46 100644 >> --- a/lib/ethdev/ethdev_driver.h >> +++ b/lib/ethdev/ethdev_driver.h >> @@ -1542,6 +1542,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 in 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_in_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 94b8fba5d7..362e5c2dec 100644 >> --- a/lib/ethdev/ethdev_pci.h >> +++ b/lib/ethdev/ethdev_pci.h >> @@ -164,7 +164,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_in_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 838b3a8f9f..a78d503e59 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_in_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 5d5e18db1e..a225141937 100644 >> --- a/lib/ethdev/rte_ethdev.c >> +++ b/lib/ethdev/rte_ethdev.c >> @@ -325,7 +325,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_in_used(rte_eth_devices[port_id].state)) >> port_id++; >> >> if (port_id >= RTE_MAX_ETHPORTS) >> @@ -372,7 +372,7 @@ int >> rte_eth_dev_is_valid_port(uint16_t port_id) >> { >> if (port_id >= RTE_MAX_ETHPORTS || >> - (rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED)) >> + !rte_eth_dev_in_used(rte_eth_devices[port_id].state)) >> return 0; >> else >> return 1; >> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h >> index c129ca1eaf..459ad44021 100644 >> --- a/lib/ethdev/rte_ethdev.h >> +++ b/lib/ethdev/rte_ethdev.h >> @@ -2000,7 +2000,9 @@ 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 allocated and is set before reporting new event. */ >> + RTE_ETH_DEV_ALLOCATED, >> + /** Device is attached when definitely probed. */ >> RTE_ETH_DEV_ATTACHED, >> /** Device is in removed state when plug-out is detected. */ >> RTE_ETH_DEV_REMOVED, >> diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map >> index 17201fbe0f..66d70f1aac 100644 >> --- a/lib/ethdev/version.map >> +++ b/lib/ethdev/version.map >> @@ -327,4 +327,5 @@ INTERNAL { >> rte_eth_representor_id_get; >> rte_eth_switch_domain_alloc; >> rte_eth_switch_domain_free; >> + rte_eth_dev_in_used; >> }; > .