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 A2B2A46084; Tue, 14 Jan 2025 13:13:19 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A1A754025F; Tue, 14 Jan 2025 13:13:18 +0100 (CET) Received: from szxga03-in.huawei.com (szxga03-in.huawei.com [45.249.212.189]) by mails.dpdk.org (Postfix) with ESMTP id AC836400EF for ; Tue, 14 Jan 2025 13:13:15 +0100 (CET) Received: from mail.maildlp.com (unknown [172.19.163.48]) by szxga03-in.huawei.com (SkyGuard) with ESMTP id 4YXSdR0n9KzRlG4; Tue, 14 Jan 2025 20:10:51 +0800 (CST) Received: from dggemv704-chm.china.huawei.com (unknown [10.3.19.47]) by mail.maildlp.com (Postfix) with ESMTPS id E5071180087; Tue, 14 Jan 2025 20:13:12 +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, 14 Jan 2025 20:13: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, 14 Jan 2025 20:13:11 +0800 Message-ID: <93d2853a-ef15-5be6-5ef4-51ebd6b9b3ca@huawei.com> Date: Tue, 14 Jan 2025 20:13:11 +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> <3524462.QJadu78ljV@thomas> <22acc285-aca6-b65e-7d8e-65145338b1dc@huawei.com> <2489922.jE0xQCEvom@thomas> From: "lihuisong (C)" In-Reply-To: <2489922.jE0xQCEvom@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/14 19:13, Thomas Monjalon 写道: > 14/01/2025 02:50, lihuisong (C): >> 在 2025/1/13 21:14, Thomas Monjalon 写道: >>> 13/01/2025 13:47, lihuisong (C): >>>> 在 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; >>> Yes. Would it work? >> I think it can work for this API. >> >> From the disscussion for this patch, we've come to an aggreement that >> application can think port is valid in new event. > We don't want an application to configure a port before probing is finished > (like still in the event processing). Ok > >> Now that the port id is valid, the new event callback of application may >> call other API, for example, rte_eth_dev_info_get(). >> (Apllication may call rte_eth_dev_info_get to get someting in new event >> callback) >> Note: patch 4/5 modified in the series[1] also used this API. >> --> >> eth_event_callback >> -->setup_attached_port >> -->reconfig >> -->init_config_port_offloads >> -->eth_dev_info_get_print_err >> --- > I don't agree with configuring a port which is not fully probed. Got it. > >> There is RTE_ETH_VALID_PORTID_OR_ERR_RET to check port_id is valid in >> rte_eth_dev_info_get. >> Application also happen to this issue like rte_eth_dev_socket_id, right? > Right, I think such application is abusing the new event. > > testpmd set a flag when receiving an event, it should not do more: > > case RTE_ETH_EVENT_NEW: > ports[port_id].need_setup = 1; > ports[port_id].port_status = RTE_PORT_HANDLING; > break; ok I know what you mean. >> This macro is also widely used in ethdev layer. We probability need to >> filter out all these interfaces which can be used in new event callback. >> And then handle the check for port_id in these interfaces like >> rte_eth_dev_socket_id. >> What do you think? Are there any other similar interfaces in ethdev layer? > As explained above, we should not do allow much API from RTE_ETH_EVENT_NEW. > rte_eth_dev_socket_id() is reasonnable. > Functions rte_eth_dev_owner_*() are fine. > Others functions should be called only after probing. All right, will fix it in new patch set. And I'll also add these comments like above you said for RTE_ETH_EVENT_NEW. > > > > .