在 2021/5/6 10:36, Li, Xiaoyun 写道:
-----Original Message-----
From: Huisong Li <lihuis...@huawei.com>
Sent: Tuesday, May 4, 2021 09:46
To: Li, Xiaoyun <xiaoyun...@intel.com>
Cc: Yigit, Ferruh <ferruh.yi...@intel.com>; dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v2 1/2] app/testpmd: add link speed check
before port start
在 2021/4/30 12:46, Li, Xiaoyun 写道:
-----Original Message-----
From: Huisong Li <lihuis...@huawei.com>
Sent: Friday, April 30, 2021 12:04
To: Li, Xiaoyun <xiaoyun...@intel.com>
Cc: Yigit, Ferruh <ferruh.yi...@intel.com>; dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v2 1/2] app/testpmd: add link speed
check before port start
在 2021/4/30 11:19, Li, Xiaoyun 写道:
-----Original Message-----
From: Min Hu (Connor) <humi...@huawei.com>
Sent: Wednesday, April 28, 2021 16:37
To: dev@dpdk.org
Cc: Yigit, Ferruh <ferruh.yi...@intel.com>; Li, Xiaoyun
<xiaoyun...@intel.com>
Subject: [PATCH v2 1/2] app/testpmd: add link speed check before
port start
From: Huisong Li <lihuis...@huawei.com>
Currently, to check whether the configured link_speeds is valid, we
have to run "port start". In addition, if the configuration fails,
"port-
dev_conf.link_speeds"
maintained in testpmd cannot be restored.
This patch adds the link_speeds check before port start by calling
dev_configure, and resolves these problems.
Not sure about this patch. I don't think you can fix the issue you mentioned.
Probably only hns3 does speed check in dev_configure. I don't see
this in other
drivers, not in i40e/ice/mlx.
I guess it's because if it's not supported speed, it will just be
UNKNOWN and
user can config again?
I think that the validity of the configuration delivered by
dev_configure is ensured by this interface and cannot be left to the backend.
Because it facilitates users to handle abnormal configurations in a
timely manner. It may be more appropriate for the driver to do this
check in dev_configure.
I still think it's not necessary.
ok😂
@Ferruh, what do you think?
In addition, even if other drivers do not add this check in
dev_configure, this patch does not seem to affect the current behavior of
these drivers.
BTW, even if this behavior is accepted by others, still some comments below.
Signed-off-by: Huisong Li <lihuis...@huawei.com>
Signed-off-by: Min Hu (Connor) <humi...@huawei.com>
---
app/test-pmd/cmdline.c | 42
++++++++++++++++++++++++++++++++++++++++-
-
1 file changed, 40 insertions(+), 2 deletions(-)
diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
5fdcc1c..ddbc629 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -1549,8 +1549,12 @@ cmd_config_speed_all_parsed(void
*parsed_result,
__rte_unused void *data)
{
struct cmd_config_speed_all *res = parsed_result;
+ uint32_t old_link_speeds[RTE_MAX_ETHPORTS];
+ struct rte_port *port;
uint32_t link_speed;
portid_t pid;
+ portid_t i;
+ int ret;
if (!all_ports_stopped()) {
printf("Please stop all ports first\n"); @@ -1562,7 +1566,26
@@ cmd_config_speed_all_parsed(void *parsed_result,
return;
RTE_ETH_FOREACH_DEV(pid) {
- ports[pid].dev_conf.link_speeds = link_speed;
+ port = &ports[pid];
+ old_link_speeds[pid] = port->dev_conf.link_speeds;
+ port->dev_conf.link_speeds = link_speed;
+ ret = rte_eth_dev_configure(pid, nb_rxq, nb_txq,
+ &port->dev_conf);
+ if (ret < 0) {
+ printf("Failed to check link speeds for port %d, ret
= %d.\n",
+ pid, ret);
+ goto roolback;
Why don't you just add restoring all ports speed here and then
"break"? No
matter one dev fails or not, all ports will do reconfig from your code logic.
And you type rollback wrongly.
It cannot exit directly after restoring all ports speed. If the cmd
fails to execute, it is necessary to reconfigure device with the correct
configuration.
Because "nb_rx/tx_queues" in dev->data are cleared to zero if
dev_configure fails to be executed in PMD driver.
?
cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1); is at the end of this cmd.
This will re-config all ports.
I don't understand why can't you add a restoring in this if and break this loop
and do this reconfig.
What do you mean? If a port fails among all ports, only the failed port is
restored and reconfigured. I suppose that's what you mean?
The cmd "port config all speed xxx duplex xxx" applies to all ports. If it
fails to be
delivered, the user considers that the cmd does not take effect.
So it is necessary to restore and reconfigure all ports to the previous state.
You still don't understand me. See below. I mean the following.
You don't need to got to rollback. Because "cmd_reconfig_device_queue(RTE_PORT_ALL,
1, 1);" will config all ports anyway already no matter you restore speeds or not.
Can you read the code carefully? "cmd_reconfig_device_queue(RTE_PORT_ALL, 1,
1);" is always there and it configs ALL ports.
static void
cmd_config_speed_all_parsed(void *parsed_result,
__rte_unused struct cmdline *cl,
__rte_unused void *data)
{
struct cmd_config_speed_all *res = parsed_result;
uint32_t link_speed;
portid_t pid;
if (!all_ports_stopped()) {
printf("Please stop all ports first\n");
return;
}
if (parse_and_check_speed_duplex(res->value1, res->value2,
&link_speed) < 0)
return;
RTE_ETH_FOREACH_DEV(pid) {
- ports[pid].dev_conf.link_speeds = link_speed;
+ port = &ports[pid];
+ old_link_speeds[pid] = port->dev_conf.link_speeds;
+ port->dev_conf.link_speeds = link_speed;
+ ret = rte_eth_dev_configure(pid, nb_rxq, nb_txq,
+ &port->dev_conf);
+ if (ret < 0) {
+ printf("Failed to check link speeds for port %d, ret =
%d.\n",
+ pid, ret);
+ for (i = 0; i <= pid; i++) {
+ port = &ports[i];
+ port->dev_conf.link_speeds = old_link_speeds[i];
+ }
+ break;
+ }
}
cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
}
It looks good! Now we're left with a question about whether we need to
add this link speed check.
@Ferruh, what do you think?
+ }
+ }
+
+ cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
+
+ return;
+
+roolback:
+ for (i = 0; i <= pid; i++) {
+ port = &ports[i];
+ port->dev_conf.link_speeds = old_link_speeds[i];
}
cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1); @@ -1621,7
+1644,10 @@ cmd_config_speed_specific_parsed(void *parsed_result,
__rte_unused void *data)
{
struct cmd_config_speed_specific *res = parsed_result;
+ uint32_t old_link_speeds;
+ struct rte_port *port;
uint32_t link_speed;
+ int ret;
if (!all_ports_stopped()) {
printf("Please stop all ports first\n"); @@ -1635,8 +1661,20
@@ cmd_config_speed_specific_parsed(void *parsed_result,
&link_speed) < 0)
return;
- ports[res->id].dev_conf.link_speeds = link_speed;
+ port = &ports[res->id];
+ old_link_speeds = port->dev_conf.link_speeds;
+ port->dev_conf.link_speeds = link_speed;
+ ret = rte_eth_dev_configure(res->id, nb_rxq, nb_txq,
+ &port->dev_conf);
+ if (ret < 0) {
+ printf("Failed to check link speeds for port %d, ret = %d.\n",
+ res->id, ret);
+ port->dev_conf.link_speeds = old_link_speeds;
+ }
+ /*
+ * If the cmd fails to execute, it is necessary to reconfigure device.
+ */
cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1); }
--
2.7.4
.
