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 0438B46063; Mon, 13 Jan 2025 03:32:39 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C6A1D4029F; Mon, 13 Jan 2025 03:32:37 +0100 (CET) Received: from szxga03-in.huawei.com (szxga03-in.huawei.com [45.249.212.189]) by mails.dpdk.org (Postfix) with ESMTP id 27DB940288 for ; Mon, 13 Jan 2025 03:32:35 +0100 (CET) Received: from mail.maildlp.com (unknown [172.19.163.174]) by szxga03-in.huawei.com (SkyGuard) with ESMTP id 4YWbnw0gkXzRlD8; Mon, 13 Jan 2025 10:30:12 +0800 (CST) Received: from dggemv711-chm.china.huawei.com (unknown [10.1.198.66]) by mail.maildlp.com (Postfix) with ESMTPS id 684C1140257; Mon, 13 Jan 2025 10:32:32 +0800 (CST) Received: from kwepemn100009.china.huawei.com (7.202.194.112) by dggemv711-chm.china.huawei.com (10.1.198.66) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.39; Mon, 13 Jan 2025 10:32:32 +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; Mon, 13 Jan 2025 10:32:31 +0800 Message-ID: <1bcc387a-fa57-dbe3-4e94-bd8c63391739@huawei.com> Date: Mon, 13 Jan 2025 10:32:30 +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> <7319082e-10ff-000e-abdf-e6d0d3a6549e@huawei.com> <9177e30c-bb30-5a1e-cdbd-18050c689376@huawei.com> <20250110095409.1e381023@hermes.local> From: "lihuisong (C)" In-Reply-To: <20250110095409.1e381023@hermes.local> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.121.59] X-ClientProxiedBy: dggems702-chm.china.huawei.com (10.3.19.179) 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 在 2025/1/11 1:54, Stephen Hemminger 写道: > On Fri, 10 Jan 2025 11:21:26 +0800 > "lihuisong (C)" wrote: > >> Hi Stephen, >> >> Can you take a look at my below reply and reconsider this patch? >> >> /Huisong >> >> 在 2024/12/10 9:50, lihuisong (C) 写道: >>> 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; >>> . > Please resubmit for 25.03 release. > But it looks like an API/ABI change since rte_eth_dev_state is visible > to applications. > > A more detailed bug report would also help > . ok，many thanks for your reply. I will resubmit this patch and send out separately. And this series that testpmd add attach and detach port for multiple process will be updated later.