From: "lihuisong (C)" <lihuisong@huawei.com>
To: Ferruh Yigit <ferruh.yigit@amd.com>, <dev@dpdk.org>,
<andrew.rybchenko@oktetlabs.ru>
Cc: <thomas@monjalon.net>, <liudongdong3@huawei.com>,
<huangdaode@huawei.com>, <fengchengwen@huawei.com>
Subject: Re: [PATCH V4 2/5] ethdev: fix skip valid port in probing callback
Date: Thu, 12 Jan 2023 12:12:27 +0800 [thread overview]
Message-ID: <0e699ace-c2a8-3f81-0cd2-88f9433651b4@huawei.com> (raw)
In-Reply-To: <a5d3ed79-bcac-d5b9-05d4-271311e6d2a4@amd.com>
在 2023/1/11 20:51, Ferruh Yigit 写道:
> On 12/6/2022 9:26 AM, Huisong Li wrote:
>> 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
>> RTE_ETH_DEV_UNUSED, this port will be skipped.
>>
>> Currently, 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")
>>
> OK to abstract ethdev state check to a function, 'rte_eth_dev_in_used'.
> BTW, I guess intention is to say 'is' instead of 'in', like
> 'rte_eth_dev_is_used'.
Ack
>
> But not sure if new state is necessary.
> I can see from next patches that if testpmd event callback wants to
> validate port_id, it is using 'port_id_is_invalid()' function which is
> using 'RTE_ETH_FOREACH_DEV' macro.
> And using 'RTE_ETH_FOREACH_DEV' macro in callback function has the
> problem you described above.
>
> 1) Do we really need to validate port_id in the event callback, callback
> provided 'port_id' from 'rte_eth_dev_callback_process()' API, from
> 'dev->data->port_id'. I am not sure if there is case that
> 'dev->data->port_id' can be invalid.
>
> 2) is there any other specific usecase to use 'RTE_ETH_FOREACH_DEV' in
> the 'RTE_ETH_EVENT_NEW' event callback function?
Reply 1) and 2): this check should be useful for failsafe.
>
>
>
>> 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 to
>> RTE_ETH_DEV_ATTACHED when definitely probed. And this port is valid if its
>> device state is ALLOCATED or ATTACHED. Naturally, a lot of places had to be
>> modified for this change.
>>
>> Fixes: be8cd210379a ("ethdev: fix port probing notification")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Huisong Li <lihuisong@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 b3de490d36..7b08353ed5 100644
>> --- a/drivers/net/bnxt/bnxt_ethdev.c
>> +++ b/drivers/net/bnxt/bnxt_ethdev.c
>> @@ -6003,7 +6003,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_in_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 e55be8720e..949d8c1367 100644
>> --- a/drivers/net/mlx5/mlx5.c
>> +++ b/drivers/net/mlx5/mlx5.c
>> @@ -3014,7 +3014,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_in_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 0be1e8ca04..7868958dff 100644
>> --- a/lib/ethdev/ethdev_driver.c
>> +++ b/lib/ethdev/ethdev_driver.c
>> @@ -50,8 +50,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_in_used(
>> + rte_eth_devices[i].state));
>> return i;
>> }
>> }
>> @@ -208,11 +208,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_in_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)
>> {
>> @@ -221,7 +228,7 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
>>
>> eth_dev_shared_data_prepare();
>>
>> - if (eth_dev->state != RTE_ETH_DEV_UNUSED)
>> + if (rte_eth_dev_in_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 6a550cfc83..e04f88fe46 100644
>> --- a/lib/ethdev/ethdev_driver.h
>> +++ b/lib/ethdev/ethdev_driver.h
>> @@ -1542,6 +1542,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 in 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_in_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 94b8fba5d7..362e5c2dec 100644
>> --- a/lib/ethdev/ethdev_pci.h
>> +++ b/lib/ethdev/ethdev_pci.h
>> @@ -164,7 +164,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_in_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 838b3a8f9f..a78d503e59 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_in_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 5d5e18db1e..a225141937 100644
>> --- a/lib/ethdev/rte_ethdev.c
>> +++ b/lib/ethdev/rte_ethdev.c
>> @@ -325,7 +325,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_in_used(rte_eth_devices[port_id].state))
>> port_id++;
>>
>> if (port_id >= RTE_MAX_ETHPORTS)
>> @@ -372,7 +372,7 @@ int
>> rte_eth_dev_is_valid_port(uint16_t port_id)
>> {
>> if (port_id >= RTE_MAX_ETHPORTS ||
>> - (rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED))
>> + !rte_eth_dev_in_used(rte_eth_devices[port_id].state))
>> return 0;
>> else
>> return 1;
>> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
>> index c129ca1eaf..459ad44021 100644
>> --- a/lib/ethdev/rte_ethdev.h
>> +++ b/lib/ethdev/rte_ethdev.h
>> @@ -2000,7 +2000,9 @@ 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 allocated and is set before reporting new event. */
>> + RTE_ETH_DEV_ALLOCATED,
>> + /** Device is attached when definitely probed. */
>> RTE_ETH_DEV_ATTACHED,
>> /** Device is in removed state when plug-out is detected. */
>> RTE_ETH_DEV_REMOVED,
>> diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
>> index 17201fbe0f..66d70f1aac 100644
>> --- a/lib/ethdev/version.map
>> +++ b/lib/ethdev/version.map
>> @@ -327,4 +327,5 @@ INTERNAL {
>> rte_eth_representor_id_get;
>> rte_eth_switch_domain_alloc;
>> rte_eth_switch_domain_free;
>> + rte_eth_dev_in_used;
>> };
> .
next prev parent reply other threads:[~2023-01-12 4:12 UTC|newest]
Thread overview: 84+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20220825024425.10534-1-lihuisong@huawei.com>
2022-09-15 12:45 ` [PATCH V2 0/6] [PATCH 0/6] app/testpmd: support attach and detach port for MP Huisong Li
2022-09-15 12:45 ` [PATCH V2 1/6] bus/pci: fix a segfault when call callback Huisong Li
2022-10-10 19:49 ` Thomas Monjalon
2022-10-25 3:25 ` lihuisong (C)
2022-09-15 12:45 ` [PATCH V2 2/6] bus/vdev: " Huisong Li
2022-09-15 12:45 ` [PATCH V2 3/6] ethdev: fix push new event Huisong Li
2022-09-27 10:49 ` Thomas Monjalon
2022-10-08 4:09 ` lihuisong (C)
2022-10-25 3:26 ` lihuisong (C)
2022-09-15 12:45 ` [PATCH V2 4/6] app/testpmd: check the validity of the port Huisong Li
2022-09-22 5:07 ` Singh, Aman Deep
2022-09-15 12:45 ` [PATCH V2 5/6] app/testpmd: support attach and detach port for MP Huisong Li
2022-09-15 12:45 ` [PATCH V2 6/6] app/testpmd: stop packet forwarding in new and destroy event Huisong Li
2022-12-06 6:45 ` [PATCH V3 0/5] app/testpmd: support mulitple process attach and detach port Huisong Li
2022-12-06 6:45 ` [PATCH V3 1/5] drivers/bus: restore driver assignment at front of probing Huisong Li
2022-12-06 6:45 ` [PATCH V3 2/5] ethdev: fix skip valid port in probing callback Huisong Li
2022-12-06 6:45 ` [PATCH V3 3/5] app/testpmd: check the validity of the port Huisong Li
2022-12-06 6:45 ` [PATCH V3 4/5] app/testpmd: add attach and detach port for multiple process Huisong Li
2022-12-06 6:45 ` [PATCH V3 5/5] app/testpmd: stop forwarding in new or destroy event Huisong Li
2022-12-06 9:26 ` [PATCH V4 0/5] app/testpmd: support mulitple process attach and detach port Huisong Li
2022-12-06 9:26 ` [PATCH V4 1/5] drivers/bus: restore driver assignment at front of probing Huisong Li
2023-01-11 12:51 ` Ferruh Yigit
2023-01-12 2:44 ` lihuisong (C)
2023-02-15 16:09 ` Ferruh Yigit
2023-02-28 2:21 ` lihuisong (C)
2023-06-06 16:12 ` Ferruh Yigit
2023-06-07 10:11 ` lihuisong (C)
2023-06-15 2:21 ` lihuisong (C)
2022-12-06 9:26 ` [PATCH V4 2/5] ethdev: fix skip valid port in probing callback Huisong Li
2023-01-11 12:51 ` Ferruh Yigit
2023-01-12 4:12 ` lihuisong (C) [this message]
2022-12-06 9:26 ` [PATCH V4 3/5] app/testpmd: check the validity of the port Huisong Li
2022-12-06 9:26 ` [PATCH V4 4/5] app/testpmd: add attach and detach port for multiple process Huisong Li
2023-01-11 12:51 ` Ferruh Yigit
2023-01-12 4:14 ` lihuisong (C)
2022-12-06 9:26 ` [PATCH V4 5/5] app/testpmd: stop forwarding in new or destroy event Huisong Li
2023-01-11 12:52 ` Ferruh Yigit
2023-01-12 4:16 ` lihuisong (C)
2023-01-09 12:38 ` [PATCH V4 0/5] app/testpmd: support mulitple process attach and detach port lihuisong (C)
2023-01-10 16:51 ` Ferruh Yigit
2023-01-11 0:53 ` lihuisong (C)
2023-01-11 10:27 ` Ferruh Yigit
2023-01-11 10:46 ` Ferruh Yigit
2023-01-12 2:26 ` lihuisong (C)
2023-01-18 14:12 ` Thomas Monjalon
2023-01-19 10:31 ` lihuisong (C)
2023-01-19 14:35 ` Thomas Monjalon
2023-01-28 1:39 ` lihuisong (C)
2023-01-31 3:33 ` [PATCH V5 0/5] app/testpmd: support multiple " Huisong Li
2023-01-31 3:33 ` [PATCH V5 1/5] drivers/bus: restore driver assignment at front of probing Huisong Li
2023-01-31 3:33 ` [PATCH V5 2/5] ethdev: fix skip valid port in probing callback Huisong Li
2023-05-22 11:04 ` fengchengwen
2023-05-27 1:58 ` lihuisong (C)
2023-01-31 3:33 ` [PATCH V5 3/5] app/testpmd: check the validity of the port Huisong Li
2023-01-31 3:33 ` [PATCH V5 4/5] app/testpmd: add attach and detach port for multiple process Huisong Li
2023-01-31 3:33 ` [PATCH V5 5/5] app/testpmd: stop forwarding in new or destroy event Huisong Li
2023-05-16 11:27 ` [PATCH V5 0/5] app/testpmd: support multiple process attach and detach port lihuisong (C)
2023-05-23 0:46 ` fengchengwen
2023-05-27 2:11 ` [PATCH V6 " Huisong Li
2023-05-27 2:11 ` [PATCH V6 1/5] drivers/bus: restore driver assignment at front of probing Huisong Li
2023-05-27 2:11 ` [PATCH V6 2/5] ethdev: fix skip valid port in probing callback Huisong Li
2023-05-27 2:11 ` [PATCH V6 3/5] app/testpmd: check the validity of the port Huisong Li
2023-05-27 2:11 ` [PATCH V6 4/5] app/testpmd: add attach and detach port for multiple process Huisong Li
2023-05-27 2:11 ` [PATCH V6 5/5] app/testpmd: stop forwarding in new or destroy event Huisong Li
2023-06-06 16:26 ` [PATCH V6 0/5] app/testpmd: support multiple process attach and detach port Ferruh Yigit
2023-06-07 10:14 ` lihuisong (C)
2023-07-14 7:21 ` lihuisong (C)
2023-08-02 3:15 ` [PATCH RESEND v6 " Huisong Li
2023-08-02 3:15 ` [PATCH RESEND v6 1/5] drivers/bus: restore driver assignment at front of probing Huisong Li
2023-08-02 3:15 ` [PATCH RESEND v6 2/5] ethdev: fix skip valid port in probing callback Huisong Li
2023-08-02 3:15 ` [PATCH RESEND v6 3/5] app/testpmd: check the validity of the port Huisong Li
2023-08-02 3:15 ` [PATCH RESEND v6 4/5] app/testpmd: add attach and detach port for multiple process Huisong Li
2023-08-02 3:16 ` [PATCH RESEND v6 5/5] app/testpmd: stop forwarding in new or destroy event Huisong Li
2023-10-09 10:34 ` [PATCH RESEND v6 0/5] app/testpmd: support multiple process attach and detach port lihuisong (C)
2023-10-30 12:17 ` lihuisong (C)
2023-12-08 2:25 ` lihuisong (C)
2024-01-30 6:36 ` [PATCH v7 " Huisong Li
2024-01-30 6:36 ` [PATCH v7 1/5] drivers/bus: restore driver assignment at front of probing Huisong Li
2024-01-30 6:36 ` [PATCH v7 2/5] ethdev: fix skip valid port in probing callback Huisong Li
2024-01-30 6:36 ` [PATCH v7 3/5] app/testpmd: check the validity of the port Huisong Li
2024-01-30 6:36 ` [PATCH v7 4/5] app/testpmd: add attach and detach port for multiple process Huisong Li
2024-01-30 6:36 ` [PATCH v7 5/5] app/testpmd: stop forwarding in new or destroy event Huisong Li
2024-03-08 10:38 ` [PATCH v7 0/5] app/testpmd: support multiple process attach and detach port lihuisong (C)
2024-04-23 11:17 ` lihuisong (C)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0e699ace-c2a8-3f81-0cd2-88f9433651b4@huawei.com \
--to=lihuisong@huawei.com \
--cc=andrew.rybchenko@oktetlabs.ru \
--cc=dev@dpdk.org \
--cc=fengchengwen@huawei.com \
--cc=ferruh.yigit@amd.com \
--cc=huangdaode@huawei.com \
--cc=liudongdong3@huawei.com \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).