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 E0F2D4606B; Mon, 13 Jan 2025 13:47:12 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 5CF6E40A80; Mon, 13 Jan 2025 13:47:12 +0100 (CET) Received: from szxga08-in.huawei.com (szxga08-in.huawei.com [45.249.212.255]) by mails.dpdk.org (Postfix) with ESMTP id C3B14402A7 for ; Mon, 13 Jan 2025 13:47:10 +0100 (CET) Received: from mail.maildlp.com (unknown [172.19.163.252]) by szxga08-in.huawei.com (SkyGuard) with ESMTP id 4YWsPM4Z2Pz1W45y; Mon, 13 Jan 2025 20:43:19 +0800 (CST) Received: from dggemv704-chm.china.huawei.com (unknown [10.3.19.47]) by mail.maildlp.com (Postfix) with ESMTPS id 042C31800D9; Mon, 13 Jan 2025 20:47:08 +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; Mon, 13 Jan 2025 20:47:07 +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 20:47:06 +0800 Message-ID: Date: Mon, 13 Jan 2025 20:47: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 v1 2/2] ethdev: fix skip valid port in probing callback To: Thomas Monjalon CC: , , , "Ajit Khaparde" , Somnath Kotur , Praveen Shetty , Andrew Boyer , Dariusz Sosnowski , Viacheslav Ovsiienko , "Bing Zhao" , Ori Kam , Suanming Mou , Matan Azrad , Chaoyong He , Andrew Rybchenko , References: <20250113025521.32703-1-lihuisong@huawei.com> <870781f8-951e-7575-0ee0-7b1ceb7833c0@huawei.com> <8515179.NyiUUSuA9g@thomas> From: "lihuisong (C)" In-Reply-To: <8515179.NyiUUSuA9g@thomas> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.121.59] X-ClientProxiedBy: dggems703-chm.china.huawei.com (10.3.19.180) 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/13 20:30, Thomas Monjalon 写道: > 13/01/2025 13:05, lihuisong (C): >> 在 2025/1/13 19:23, lihuisong (C) 写道: >>> 在 2025/1/13 18:57, Thomas Monjalon 写道: >>>> 13/01/2025 10:35, lihuisong (C): >>>>> 在 2025/1/13 16:16, Thomas Monjalon 写道: >>>>>> 13/01/2025 03:55, 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'. >>>>>> If you do that, changing the definition of eth_dev_find_free_port() >>>>>> you allow the application using a port before probing is finished. >>>>> Yes, it's not reasonable. >>>>> >>>>> Thinking your comment twice, I feel that the root cause of this >>>>> issue is >>>>> application want to check if the port id is valid. >>>>> However, application just receive the new event from the device and the >>>>> port id of this device must be valid when report new event. >>>>> So application can think the received new event is valid and don't need >>>>> to check, right? >>>> Yes >>>> Do you think it should be highlighted in the API doc? >>> Security detection is common and always good for application. >>> So I think it's better to highlight that in doc. >>> >> Now I remember why I have to put this patch into the patchset [1] that >> testpmd support multiple process attach and detach port. >> Becase patch 4/5 in this series depands on this patch. >> The setup_attached_port() have to move to eth_event_callback() in >> testpmd to update something. >> And the setup_attached_port() would indirectyly check if this port is >> valid by rte_eth_dev_is_valid_port(). >> Their caller stack is as follows: >> eth_event_callback >> -->setup_attached_port >> -->rte_eth_dev_socket_id >> -->rte_eth_dev_is_valid_port >> >> From the testpmd's modification, that is to say, it is possible for >> appllication to call some APIs like rte_eth_dev_socket_id() and >> indirectyly check if this port id is valid in event new callback. >> So should we add this patch? I think there are many like these API in >> ethdev layer. I'm confused a bit now. > Yes rte_eth_dev_is_valid_port() is used in many API functions, > so that's a valid concern. > I would say we should not call much of these functions in the "new port" > event callback. > But the case of rte_eth_dev_socket_id() is concerning. > > I suggest to update rte_eth_dev_socket_id() to make it work with > a newly allocated port. > I suppose we can use the function eth_dev_is_allocated(). What you mean is doing it like the following code? --> --- a/lib/ethdev/rte_ethdev.c +++ b/lib/ethdev/rte_ethdev.c @@ -635,8 +635,10 @@ int  rte_eth_dev_socket_id(uint16_t port_id)  {         int socket_id = SOCKET_ID_ANY; +       struct rte_eth_dev *ethdev; -       if (!rte_eth_dev_is_valid_port(port_id)) { +       ethdev = &rte_eth_devices[port_id]; +       if (!eth_dev_is_allocated(ethdev)) {                 rte_errno = EINVAL;         } else {                 socket_id = rte_eth_devices[port_id].data->numa_node; > > > .