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 46BCE46087; Tue, 14 Jan 2025 12:14:02 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1F2F64025F; Tue, 14 Jan 2025 12:14:02 +0100 (CET) Received: from fout-b8-smtp.messagingengine.com (fout-b8-smtp.messagingengine.com [202.12.124.151]) by mails.dpdk.org (Postfix) with ESMTP id 3B206400EF for ; Tue, 14 Jan 2025 12:14:01 +0100 (CET) Received: from phl-compute-03.internal (phl-compute-03.phl.internal [10.202.2.43]) by mailfout.stl.internal (Postfix) with ESMTP id 3838711400E4; Tue, 14 Jan 2025 06:14:00 -0500 (EST) Received: from phl-mailfrontend-01 ([10.202.2.162]) by phl-compute-03.internal (MEProxy); Tue, 14 Jan 2025 06:14:00 -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=1736853240; x=1736939640; bh=r5NbdAq0HNoC9NEj/1aIAZx4D0jXa1aWlacB55zEb8I=; b= nKMD7FZJcfMgKqcwdJI4q6qObJXVpS6r3nbOUMMhlJ9Hp+QnP0CzJ8rykYKOuG+9 h7JV6BFMXhdq2ZSzmaKKKsg4K1UZ0ggTWj/nvEQ0qNraWpba9E5wlJl8kT80ZBLX uUSR3K7ZMEKI4NmUVDezu4V4ERjWDvJ71/KFvuGMztNyJ8g8zddHYGbEhrC5+XCU xoSzaSFE2J90gNe0O4iP5mb/yjCRKypxwpV2bI28rpnWo0iFC6fncsAklIG1853c kt8X3popQ7+jsfzd5kYYfXfAAMk1TuYEEBvqJrOTXny/WwXON/a6GDUacRXGxevU /qaP4CaSYNBwMwOEglJNBQ== 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=1736853240; x= 1736939640; bh=r5NbdAq0HNoC9NEj/1aIAZx4D0jXa1aWlacB55zEb8I=; b=d WdxZnDTIBTRG7yq1htJ2MnFEcl8oMZl6UV61KZJ2W5/JmWZ1DbRntrs6nAgrCC2t DBBOxLywdc9/ttShXQC8gj3NxNAUmYUD62VC+m6JJCO06TxpYYA7990cIK+Me5t8 K4rQpVALUFSnD3A73tTcZ0pQvn9ZibY5Xy7YdhzwP8+o3lalI8ASM7+JWn89q6dY DJkX32AAgftFVd9OyEWXn/pzUtF+vVDIukTkF2HHrydA6dEJ0r0W1PYa4sO38NDy DwshO5jR3RsRuE1E+5kfUeyX8CD1Yo/HiE9TKmazpUG3WupmUs4z7JiJj4ZY8R1c bfAeq3POhTttLvmazFfjQ== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeefuddrudehiedgvdehucetufdoteggodetrfdotf 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; Tue, 14 Jan 2025 06:13:56 -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: Tue, 14 Jan 2025 12:13:54 +0100 Message-ID: <2489922.jE0xQCEvom@thomas> In-Reply-To: <22acc285-aca6-b65e-7d8e-65145338b1dc@huawei.com> References: <20250113025521.32703-1-lihuisong@huawei.com> <3524462.QJadu78ljV@thomas> <22acc285-aca6-b65e-7d8e-65145338b1dc@huawei.com> 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 14/01/2025 02:50, lihuisong (C): > =E5=9C=A8 2025/1/13 21:14, Thomas Monjalon =E5=86=99=E9=81=93: > > 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= 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_por= t() > >>>>>>>> you allow the application using a port before probing is finishe= d. > >>>>>>> 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 a= nd 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] th= at > >>>> 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 f= or > >>>> 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 po= rt" > >>> 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 =3D SOCKET_ID_ANY; > >> + struct rte_eth_dev *ethdev; > >> > >> - 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_no= de; > > > > Yes. Would it work? > I think it can work for this API. >=20 > From the disscussion for this patch, we've come to an aggreement that=20 > 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). > Now that the port id is valid, the new event callback of application may= =20 > call other API, for example, rte_eth_dev_info_get(). > (Apllication may call rte_eth_dev_info_get to get someting in new event=20 > 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. > There is RTE_ETH_VALID_PORTID_OR_ERR_RET to check port_id is valid in=20 > 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: = =20 ports[port_id].need_setup =3D 1; ports[port_id].port_status =3D RTE_PORT_HANDLING; break; =20 > This macro is also widely used in ethdev layer. We probability need to=20 > 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=20 > rte_eth_dev_socket_id. > What do you think? Are there any other similar interfaces in ethdev laye= r? As explained above, we should not do allow much API from RTE_ETH_EVENT_NEW. rte_eth_dev_socket_id() is reasonnable. =46unctions rte_eth_dev_owner_*() are fine. Others functions should be called only after probing.