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 0438B46063;
Mon, 13 Jan 2025 03:32:39 +0100 (CET)
Received: from mails.dpdk.org (localhost [127.0.0.1])
by mails.dpdk.org (Postfix) with ESMTP id C6A1D4029F;
Mon, 13 Jan 2025 03:32:37 +0100 (CET)
Received: from szxga03-in.huawei.com (szxga03-in.huawei.com [45.249.212.189])
by mails.dpdk.org (Postfix) with ESMTP id 27DB940288
for <dev@dpdk.org>; Mon, 13 Jan 2025 03:32:35 +0100 (CET)
Received: from mail.maildlp.com (unknown [172.19.163.174])
by szxga03-in.huawei.com (SkyGuard) with ESMTP id 4YWbnw0gkXzRlD8;
Mon, 13 Jan 2025 10:30:12 +0800 (CST)
Received: from dggemv711-chm.china.huawei.com (unknown [10.1.198.66])
by mail.maildlp.com (Postfix) with ESMTPS id 684C1140257;
Mon, 13 Jan 2025 10:32:32 +0800 (CST)
Received: from kwepemn100009.china.huawei.com (7.202.194.112) by
dggemv711-chm.china.huawei.com (10.1.198.66) 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 10:32:32 +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 10:32:31 +0800
Message-ID: <1bcc387a-fa57-dbe3-4e94-bd8c63391739@huawei.com>
Date: Mon, 13 Jan 2025 10:32:30 +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 RESEND v7 2/5] ethdev: fix skip valid port in probing
callback
To: Stephen Hemminger <stephen@networkplumber.org>
CC: <dev@dpdk.org>, <fengchengwen@huawei.com>, <liuyonglong@huawei.com>,
<andrew.rybchenko@oktetlabs.ru>, Somnath Kotur <somnath.kotur@broadcom.com>,
Ajit Khaparde <ajit.khaparde@broadcom.com>, Dariusz Sosnowski
<dsosnowski@nvidia.com>, Suanming Mou <suanmingm@nvidia.com>, Matan Azrad
<matan@nvidia.com>, Ori Kam <orika@nvidia.com>, Viacheslav Ovsiienko
<viacheslavo@nvidia.com>, <ferruh.yigit@amd.com>, <thomas@monjalon.net>
References: <20220825024425.10534-1-lihuisong@huawei.com>
<20240929055241.29268-1-lihuisong@huawei.com>
<20240929055241.29268-3-lihuisong@huawei.com>
<7319082e-10ff-000e-abdf-e6d0d3a6549e@huawei.com>
<9177e30c-bb30-5a1e-cdbd-18050c689376@huawei.com>
<20250110095409.1e381023@hermes.local>
From: "lihuisong (C)" <lihuisong@huawei.com>
In-Reply-To: <20250110095409.1e381023@hermes.local>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
X-Originating-IP: [10.67.121.59]
X-ClientProxiedBy: dggems702-chm.china.huawei.com (10.3.19.179) 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 <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
在 2025/1/11 1:54, Stephen Hemminger 写道:
> On Fri, 10 Jan 2025 11:21:26 +0800
> "lihuisong (C)" <lihuisong@huawei.com> wrote:
>
>> Hi Stephen,
>>
>> Can you take a look at my below reply and reconsider this patch?
>>
>> /Huisong
>>
>> 在 2024/12/10 9:50, lihuisong (C) 写道:
>>> Hi Ferruh, Stephen and Thomas,
>>>
>>> Can you take a look at this patch? After all, it is an issue in ethdev
>>> layer.
>>> This also is the fruit we disscussed with Thomas and Ferruh before.
>>> Please go back to this thread. If we don't need this patch, please let
>>> me know. I will drop it from my upstreaming list.
>>>
>>> /Huisong
>>>
>>>
>>> 在 2024/9/29 13:52, 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'.
>>>>
>>>> In addition, the new state has to be placed behind 'REMOVED' to avoid
>>>> ABI
>>>> break. Fortunately, this ethdev state is internal and applications
>>>> can not
>>>> access it directly. So this patch encapsulates an API,
>>>> rte_eth_dev_is_used,
>>>> for ethdev or PMD to call and eliminate concerns about using this state
>>>> enum value comparison.
>>>>
>>>> Fixes: be8cd210379a ("ethdev: fix port probing notification")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>> Acked-by: Chengwen Feng <fengchengwen@huawei.com>
>>>> ---
>>>> drivers/net/bnxt/bnxt_ethdev.c | 3 ++-
>>>> drivers/net/mlx5/mlx5.c | 2 +-
>>>> lib/ethdev/ethdev_driver.c | 13 ++++++++++---
>>>> lib/ethdev/ethdev_driver.h | 12 ++++++++++++
>>>> lib/ethdev/ethdev_pci.h | 2 +-
>>>> lib/ethdev/rte_class_eth.c | 2 +-
>>>> lib/ethdev/rte_ethdev.c | 4 ++--
>>>> lib/ethdev/rte_ethdev.h | 4 +++-
>>>> lib/ethdev/version.map | 1 +
>>>> 9 files changed, 33 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/net/bnxt/bnxt_ethdev.c
>>>> b/drivers/net/bnxt/bnxt_ethdev.c
>>>> index c6ad764813..7401dcd8b5 100644
>>>> --- a/drivers/net/bnxt/bnxt_ethdev.c
>>>> +++ b/drivers/net/bnxt/bnxt_ethdev.c
>>>> @@ -6612,7 +6612,8 @@ bnxt_dev_uninit(struct rte_eth_dev *eth_dev)
>>>> PMD_DRV_LOG(DEBUG, "Calling Device uninit\n");
>>>> - if (eth_dev->state != RTE_ETH_DEV_UNUSED)
>>>> +
>>>> + if (rte_eth_dev_is_used(eth_dev->state))
>>>> bnxt_dev_close_op(eth_dev);
>>>> return 0;
>>>> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
>>>> index 8d266b0e64..0df49e1f69 100644
>>>> --- a/drivers/net/mlx5/mlx5.c
>>>> +++ b/drivers/net/mlx5/mlx5.c
>>>> @@ -3371,7 +3371,7 @@ mlx5_eth_find_next(uint16_t port_id, struct
>>>> rte_device *odev)
>>>> while (port_id < RTE_MAX_ETHPORTS) {
>>>> struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>>>> - if (dev->state != RTE_ETH_DEV_UNUSED &&
>>>> + if (rte_eth_dev_is_used(dev->state) &&
>>>> dev->device &&
>>>> (dev->device == odev ||
>>>> (dev->device->driver &&
>>>> diff --git a/lib/ethdev/ethdev_driver.c b/lib/ethdev/ethdev_driver.c
>>>> index c335a25a82..a87dbb00ff 100644
>>>> --- a/lib/ethdev/ethdev_driver.c
>>>> +++ b/lib/ethdev/ethdev_driver.c
>>>> @@ -55,8 +55,8 @@ eth_dev_find_free_port(void)
>>>> for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
>>>> /* Using shared name field to find a free port. */
>>>> if (eth_dev_shared_data->data[i].name[0] == '\0') {
>>>> - RTE_ASSERT(rte_eth_devices[i].state ==
>>>> - RTE_ETH_DEV_UNUSED);
>>>> + RTE_ASSERT(!rte_eth_dev_is_used(
>>>> + rte_eth_devices[i].state));
>>>> return i;
>>>> }
>>>> }
>>>> @@ -221,11 +221,18 @@ rte_eth_dev_probing_finish(struct rte_eth_dev
>>>> *dev)
>>>> if (rte_eal_process_type() == RTE_PROC_SECONDARY)
>>>> eth_dev_fp_ops_setup(rte_eth_fp_ops + dev->data->port_id,
>>>> dev);
>>>> + dev->state = RTE_ETH_DEV_ALLOCATED;
>>>> rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_NEW, NULL);
>>>> dev->state = RTE_ETH_DEV_ATTACHED;
>>>> }
>>>> +bool rte_eth_dev_is_used(uint16_t dev_state)
>>>> +{
>>>> + return dev_state == RTE_ETH_DEV_ALLOCATED ||
>>>> + dev_state == RTE_ETH_DEV_ATTACHED;
>>>> +}
>>>> +
>>>> int
>>>> rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
>>>> {
>>>> @@ -243,7 +250,7 @@ rte_eth_dev_release_port(struct rte_eth_dev
>>>> *eth_dev)
>>>> if (ret != 0)
>>>> return ret;
>>>> - if (eth_dev->state != RTE_ETH_DEV_UNUSED)
>>>> + if (rte_eth_dev_is_used(eth_dev->state))
>>>> rte_eth_dev_callback_process(eth_dev,
>>>> RTE_ETH_EVENT_DESTROY, NULL);
>>>> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
>>>> index abed4784aa..aa35b65848 100644
>>>> --- a/lib/ethdev/ethdev_driver.h
>>>> +++ b/lib/ethdev/ethdev_driver.h
>>>> @@ -1704,6 +1704,18 @@ int rte_eth_dev_callback_process(struct
>>>> rte_eth_dev *dev,
>>>> __rte_internal
>>>> void rte_eth_dev_probing_finish(struct rte_eth_dev *dev);
>>>> +/**
>>>> + * Check if a Ethernet device state is used or not
>>>> + *
>>>> + * @param dev_state
>>>> + * The state of the Ethernet device
>>>> + * @return
>>>> + * - true if the state of the Ethernet device is allocated or
>>>> attached
>>>> + * - false if this state is neither allocated nor attached
>>>> + */
>>>> +__rte_internal
>>>> +bool rte_eth_dev_is_used(uint16_t dev_state);
>>>> +
>>>> /**
>>>> * Create memzone for HW rings.
>>>> * malloc can't be used as the physical address is needed.
>>>> diff --git a/lib/ethdev/ethdev_pci.h b/lib/ethdev/ethdev_pci.h
>>>> index ec4f731270..05dec6716b 100644
>>>> --- a/lib/ethdev/ethdev_pci.h
>>>> +++ b/lib/ethdev/ethdev_pci.h
>>>> @@ -179,7 +179,7 @@ rte_eth_dev_pci_generic_remove(struct
>>>> rte_pci_device *pci_dev,
>>>> * eth device has been released.
>>>> */
>>>> if (rte_eal_process_type() == RTE_PROC_SECONDARY &&
>>>> - eth_dev->state == RTE_ETH_DEV_UNUSED)
>>>> + !rte_eth_dev_is_used(eth_dev->state))
>>>> return 0;
>>>> if (dev_uninit) {
>>>> diff --git a/lib/ethdev/rte_class_eth.c b/lib/ethdev/rte_class_eth.c
>>>> index b52f1dd9f2..81e70670d9 100644
>>>> --- a/lib/ethdev/rte_class_eth.c
>>>> +++ b/lib/ethdev/rte_class_eth.c
>>>> @@ -118,7 +118,7 @@ eth_dev_match(const struct rte_eth_dev *edev,
>>>> const struct rte_kvargs *kvlist = arg->kvlist;
>>>> unsigned int pair;
>>>> - if (edev->state == RTE_ETH_DEV_UNUSED)
>>>> + if (!rte_eth_dev_is_used(edev->state))
>>>> return -1;
>>>> if (arg->device != NULL && arg->device != edev->device)
>>>> return -1;
>>>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>>>> index a1f7efa913..4dc66abb7b 100644
>>>> --- a/lib/ethdev/rte_ethdev.c
>>>> +++ b/lib/ethdev/rte_ethdev.c
>>>> @@ -349,7 +349,7 @@ uint16_t
>>>> rte_eth_find_next(uint16_t port_id)
>>>> {
>>>> while (port_id < RTE_MAX_ETHPORTS &&
>>>> - rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED)
>>>> + !rte_eth_dev_is_used(rte_eth_devices[port_id].state))
>>>> port_id++;
>>>> if (port_id >= RTE_MAX_ETHPORTS)
>>>> @@ -408,7 +408,7 @@ rte_eth_dev_is_valid_port(uint16_t port_id)
>>>> int is_valid;
>>>> if (port_id >= RTE_MAX_ETHPORTS ||
>>>> - (rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED))
>>>> + !rte_eth_dev_is_used(rte_eth_devices[port_id].state))
>>>> is_valid = 0;
>>>> else
>>>> is_valid = 1;
>>>> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
>>>> index a9f92006da..9cc37e8cde 100644
>>>> --- a/lib/ethdev/rte_ethdev.h
>>>> +++ b/lib/ethdev/rte_ethdev.h
>>>> @@ -2083,10 +2083,12 @@ typedef uint16_t
>>>> (*rte_tx_callback_fn)(uint16_t port_id, uint16_t queue,
>>>> enum rte_eth_dev_state {
>>>> /** Device is unused before being probed. */
>>>> RTE_ETH_DEV_UNUSED = 0,
>>>> - /** Device is attached when allocated in probing. */
>>>> + /** Device is attached when definitely probed. */
>>>> RTE_ETH_DEV_ATTACHED,
>>>> /** Device is in removed state when plug-out is detected. */
>>>> RTE_ETH_DEV_REMOVED,
>>>> + /** Device is allocated and is set before reporting new event. */
>>>> + RTE_ETH_DEV_ALLOCATED,
>>>> };
>>>> struct rte_eth_dev_sriov {
>>>> diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
>>>> index f63dc32aa2..6ecf1ab89d 100644
>>>> --- a/lib/ethdev/version.map
>>>> +++ b/lib/ethdev/version.map
>>>> @@ -349,6 +349,7 @@ INTERNAL {
>>>> rte_eth_dev_get_by_name;
>>>> rte_eth_dev_is_rx_hairpin_queue;
>>>> rte_eth_dev_is_tx_hairpin_queue;
>>>> + rte_eth_dev_is_used;
>>>> rte_eth_dev_probing_finish;
>>>> rte_eth_dev_release_port;
>>>> rte_eth_dev_internal_reset;
>>> .
> Please resubmit for 25.03 release.
> But it looks like an API/ABI change since rte_eth_dev_state is visible
> to applications.
>
> A more detailed bug report would also help
> .
ok，many thanks for your reply.
I will resubmit this patch and send out separately.
And this series that testpmd add attach and detach port for multiple
process will be updated later.