From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
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 <dev@dpdk.org>; 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: <xms:wRGFZ3-vVxhERC9MZUqUoGGZf7QiDYK1I58xA-zAh_EnKAxkrQr7wQ>
<xme:wRGFZzvvC1hpx_Wa7gcYJcRwZrOUSNHJ3M_lhjSy85TgL0IrmTNVW2-IGgyU6LI-B
SjjbjehRRzs6Fyb8w>
X-ME-Received: <xmr:wRGFZ1A2yOEAhFj8Jqm3NWtHSq5SpkQ7Ix7CyvSvlvKmKseNUq4tt9Wik-8XwRYbrYI8nXW6M1I-ZmtoPf0GJaNlHQ>
X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeefuddrudehgedggeelucetufdoteggodetrfdotf
fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdggtfgfnhhsuhgsshgtrhhisggvpdfu
rfetoffkrfgpnffqhgenuceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnh
htshculddquddttddmnecujfgurhephffvvefufffkjghfggfgtgesthhqredttddtjeen
ucfhrhhomhepvfhhohhmrghsucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrg
hlohhnrdhnvghtqeenucggtffrrghtthgvrhhnpeegtddtleejjeegffekkeektdejvedt
heevtdekiedvueeuvdeiuddvleevjeeujeenucevlhhushhtvghrufhiiigvpedtnecurf
grrhgrmhepmhgrihhlfhhrohhmpehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtpdhn
sggprhgtphhtthhopedujedpmhhouggvpehsmhhtphhouhhtpdhrtghpthhtoheplhhihh
huihhsohhngheshhhurgifvghirdgtohhmpdhrtghpthhtohepuggvvhesughpughkrdho
rhhgpdhrtghpthhtohepshhtvghphhgvnhesnhgvthifohhrkhhplhhumhgsvghrrdhorh
hgpdhrtghpthhtohepfhgvrhhruhhhrdihihhgihhtsegrmhgurdgtohhmpdhrtghpthht
oheprghjihhtrdhkhhgrphgrrhguvgessghrohgruggtohhmrdgtohhmpdhrtghpthhtoh
epshhomhhnrghthhdrkhhothhurhessghrohgruggtohhmrdgtohhmpdhrtghpthhtohep
phhrrghvvggvnhdrshhhvghtthihsehinhhtvghlrdgtohhmpdhrtghpthhtoheprghnug
hrvgifrdgsohihvghrsegrmhgurdgtohhmpdhrtghpthhtohepughsohhsnhhofihskhhi
sehnvhhiughirgdrtghomh
X-ME-Proxy: <xmx:wRGFZzdsrrl1LGdrioKqJ2SkOJQZIQn3Io9dnXiM9Fv2DoGVAovHoA>
<xmx:wRGFZ8NBdXVuPSMRIRBcDPyRHBB7Y82OJBE6uzgBDtwW8WLIKKE8Jw>
<xmx:wRGFZ1nOrJa6v3pOWF32Vkk7JBs7dly2BOhvdmlEKGgoqyaLb52boQ>
<xmx:wRGFZ2tjoUO5dh_Fdrf0zYaIuRFBLrdqCHP8IaglzHNCLFDgQ93vMQ>
<xmx:whGFZ3Hldk5ijOHlexhEN8yaEue-6r2za83OWhMuzSArQ621tenVjRDM>
Feedback-ID: i47234305:Fastmail
Received: by mail.messagingengine.com (Postfix) with ESMTPA; Mon,
13 Jan 2025 08:14:38 -0500 (EST)
From: Thomas Monjalon <thomas@monjalon.net>
To: "lihuisong (C)" <lihuisong@huawei.com>
Cc: dev@dpdk.org, stephen@networkplumber.org, ferruh.yigit@amd.com,
Ajit Khaparde <ajit.khaparde@broadcom.com>,
Somnath Kotur <somnath.kotur@broadcom.com>,
Praveen Shetty <praveen.shetty@intel.com>,
Andrew Boyer <andrew.boyer@amd.com>,
Dariusz Sosnowski <dsosnowski@nvidia.com>,
Viacheslav Ovsiienko <viacheslavo@nvidia.com>, Bing Zhao <bingz@nvidia.com>,
Ori Kam <orika@nvidia.com>, Suanming Mou <suanmingm@nvidia.com>,
Matan Azrad <matan@nvidia.com>, Chaoyong He <chaoyong.he@corigine.com>,
Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>, 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: <bce48ca0-8af4-2241-d90d-988a12d9a607@huawei.com>
References: <20250113025521.32703-1-lihuisong@huawei.com>
<8515179.NyiUUSuA9g@thomas>
<bce48ca0-8af4-2241-d90d-988a12d9a607@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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=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?