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 992CF4606F; Mon, 13 Jan 2025 14:14:45 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 5736640A80; Mon, 13 Jan 2025 14:14:45 +0100 (CET) Received: from fhigh-b2-smtp.messagingengine.com (fhigh-b2-smtp.messagingengine.com [202.12.124.153]) by mails.dpdk.org (Postfix) with ESMTP id B02CD402A7 for ; Mon, 13 Jan 2025 14:14:43 +0100 (CET) Received: from phl-compute-04.internal (phl-compute-04.phl.internal [10.202.2.44]) by mailfhigh.stl.internal (Postfix) with ESMTP id 8BFD8254010C; Mon, 13 Jan 2025 08:14:42 -0500 (EST) Received: from phl-mailfrontend-02 ([10.202.2.163]) by phl-compute-04.internal (MEProxy); Mon, 13 Jan 2025 08:14:43 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= cc:cc:content-transfer-encoding:content-type:content-type:date :date:from:from:in-reply-to:in-reply-to:message-id:mime-version :references:reply-to:subject:subject:to:to; s=fm2; t=1736774082; x=1736860482; bh=xNfIjb5i8+r+nCP0hdLiidTvgYrbVu7Mea0qspbDpng=; b= YhcBmzsBCdRp5B91StKOJawi0zvQJvUDLILKlQ+XHQ6S1apoKVe4X3wOseQruc0a MBmREa9i/hCl/uzGwIxm+SKxc23g8c3VG2scckgmn0J+c5vLmFfwWoNE+yQErh3p NeNwoeH7vM0fMESAtz5FqIx87xkXpwfokOh0sodOKlLklfZ0uB/GvVa6fZcq4RDS CEsKk5Hq2iouUpB7FbluMmg7ISFdhFF/Hn7vwQ8Rqi8oRMRAwGPMW7GYulHDc+EI hFXV1bbHBzSwkKGg1PIR13b74IVW+bQFRIsDBx35aFneOzP0C0u8dVQc1J0LnKZR DdiGFWMz00cHRG1Xs0EnSg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding :content-type:content-type:date:date:feedback-id:feedback-id :from:from:in-reply-to:in-reply-to:message-id:mime-version :references:reply-to:subject:subject:to:to:x-me-proxy :x-me-sender:x-me-sender:x-sasl-enc; s=fm2; t=1736774082; x= 1736860482; bh=xNfIjb5i8+r+nCP0hdLiidTvgYrbVu7Mea0qspbDpng=; b=Z 42wRGYCXXk47pcjXyJrovLmMfyJg1/Bj17DktvMsRuCoIbXosW6TlsG5eCWWjPQt V0hh/A1XILV8j7/5OkPh+Wl09JH4tEeMMcd/rewvO9yZpBZcalfpwNH1H8Dedw7n 3PJ2kn5KSWi0lHoJ8TAcDk1q1d/U1XmAzChaXwX4uENx6xdScFDkDyZqQOuBWZM5 bb24wuowANa0kgmkyAsRznHw5pVozy0ytQ8+eK0iux8TIBed886NmTNnZO7Pi4bQ NmEeJqAJ2dtX8dnXyeI/gL4D+AeX2csMwo6haDF0WgITmYZOjn23wV8Q/ZnC6vTl HWPeBZNdvqUgjSrX4OzHg== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeefuddrudehgedggeelucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdggtfgfnhhsuhgsshgtrhhisggvpdfu rfetoffkrfgpnffqhgenuceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnh htshculddquddttddmnecujfgurhephffvvefufffkjghfggfgtgesthhqredttddtjeen ucfhrhhomhepvfhhohhmrghsucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrg hlohhnrdhnvghtqeenucggtffrrghtthgvrhhnpeegtddtleejjeegffekkeektdejvedt heevtdekiedvueeuvdeiuddvleevjeeujeenucevlhhushhtvghrufhiiigvpedtnecurf grrhgrmhepmhgrihhlfhhrohhmpehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtpdhn sggprhgtphhtthhopedujedpmhhouggvpehsmhhtphhouhhtpdhrtghpthhtoheplhhihh huihhsohhngheshhhurgifvghirdgtohhmpdhrtghpthhtohepuggvvhesughpughkrdho rhhgpdhrtghpthhtohepshhtvghphhgvnhesnhgvthifohhrkhhplhhumhgsvghrrdhorh hgpdhrtghpthhtohepfhgvrhhruhhhrdihihhgihhtsegrmhgurdgtohhmpdhrtghpthht oheprghjihhtrdhkhhgrphgrrhguvgessghrohgruggtohhmrdgtohhmpdhrtghpthhtoh epshhomhhnrghthhdrkhhothhurhessghrohgruggtohhmrdgtohhmpdhrtghpthhtohep phhrrghvvggvnhdrshhhvghtthihsehinhhtvghlrdgtohhmpdhrtghpthhtoheprghnug hrvgifrdgsohihvghrsegrmhgurdgtohhmpdhrtghpthhtohepughsohhsnhhofihskhhi sehnvhhiughirgdrtghomh X-ME-Proxy: Feedback-ID: i47234305:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Mon, 13 Jan 2025 08:14:38 -0500 (EST) From: Thomas Monjalon To: "lihuisong (C)" Cc: dev@dpdk.org, stephen@networkplumber.org, ferruh.yigit@amd.com, Ajit Khaparde , Somnath Kotur , Praveen Shetty , Andrew Boyer , Dariusz Sosnowski , Viacheslav Ovsiienko , Bing Zhao , Ori Kam , Suanming Mou , Matan Azrad , Chaoyong He , Andrew Rybchenko , fengchengwen@huawei.com Subject: Re: [PATCH v1 2/2] ethdev: fix skip valid port in probing callback Date: Mon, 13 Jan 2025 14:14:37 +0100 Message-ID: <3524462.QJadu78ljV@thomas> In-Reply-To: References: <20250113025521.32703-1-lihuisong@huawei.com> <8515179.NyiUUSuA9g@thomas> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="utf-8" 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 13/01/2025 13:47, lihuisong (C): > =E5=9C=A8 2025/1/13 20:30, Thomas Monjalon =E5=86=99=E9=81=93: > > 13/01/2025 13:05, lihuisong (C): > >> =E5=9C=A8 2025/1/13 19:23, lihuisong (C) =E5=86=99=E9=81=93: > >>> =E5=9C=A8 2025/1/13 18:57, Thomas Monjalon =E5=86=99=E9=81=93: > >>>> 13/01/2025 10:35, lihuisong (C): > >>>>> =E5=9C=A8 2025/1/13 16:16, Thomas Monjalon =E5=86=99=E9=81=93: > >>>>>> 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 p= ort. > >>>>>>> > >>>>>>> However, this state is set to RTE_ETH_DEV_ATTACHED after pushing > >>>>>>> probing > >>>>>>> event. It means that probing callback will skip this port. But th= is > >>>>>>> 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? > --> >=20 > --- 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 =3D SOCKET_ID_ANY; > + struct rte_eth_dev *ethdev; >=20 > - if (!rte_eth_dev_is_valid_port(port_id)) { > + ethdev =3D &rte_eth_devices[port_id]; > + if (!eth_dev_is_allocated(ethdev)) { > rte_errno =3D EINVAL; > } else { > socket_id =3D rte_eth_devices[port_id].data->numa_node; Yes. Would it work?