[1/2] app/testpmd: add link speed check before port start
Checks
Commit Message
From: Huisong Li <lihuisong@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.
Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
app/test-pmd/cmdline.c | 40 ++++++++++++++++++++++++++++++++++++++--
1 file changed, 38 insertions(+), 2 deletions(-)
Comments
On 4/19/2021 7:21 AM, Min Hu (Connor) wrote:
> From: Huisong Li <lihuisong@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.
>
Hi Connor,
I am not sure about this patch, overall design in many commands seems store the
config changes and mark port or queue as needs reconfiguration
('cmd_reconfig_device_queue()'), and do the reconfigure during port_start.
Your change is against this overall design.
What is the downside of learning that speed is invalid in port start?
To not able to restore the previous speed is not good, but user can set the
desired speed again in case of failure.
And do you think adding a check that if the requested speed is in the device
reported speed capability help on early fail and restore?
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
> app/test-pmd/cmdline.c | 40 ++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 38 insertions(+), 2 deletions(-)
>
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 5bf1497..09e30cf 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -1537,8 +1537,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");
> @@ -1550,7 +1554,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;
> + }
> + }
> +
> + 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);
> @@ -1609,7 +1632,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");
> @@ -1623,7 +1649,17 @@ 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;
> + return;
> + }
>
> cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
> }
>
在 2021/4/20 20:11, Ferruh Yigit 写道:
> On 4/19/2021 7:21 AM, Min Hu (Connor) wrote:
>> From: Huisong Li <lihuisong@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.
>>
>
> Hi Connor,
>
> I am not sure about this patch, overall design in many commands seems
> store the config changes and mark port or queue as needs reconfiguration
> ('cmd_reconfig_device_queue()'), and do the reconfigure during port_start.
> Your change is against this overall design.
>
Not exactly. DCB configuration in testpmd is also the same operation. I
understand that the DCB configuration needs to be verified in driver
dev_configure before doing the reconfigure during port_start.
> What is the downside of learning that speed is invalid in port start?
> To not able to restore the previous speed is not good, but user can set
> the desired speed again in case of failure.
>
I think there are two things:
1/ User may not know the previous configuration.
2/ Generally, if a command fails to be configured, the current
configuration does not聽 take effect and remains in the previous state.
This is achieved by adding a check and recovery here.
> And do you think adding a check that if the requested speed is in the
> device reported speed capability help on early fail and restore?
>
It may not be feasible. After all, the definition of 'speed_capa' and
'link_speeds' is not very relevant in the framework.
In addition, the scheme for reporting 'speed_capa' varies from the PMD
driver. Many drivers report all speed capabilities, but the actual state
may not support all of them.
However, if the driver supports the 'link_speeds' configuration, the
driver should verify the link_speeds configuration because the ethdev
layer does not verify the 'link_speeds' configuration.
Above is my personal opinion, look forward to reply.
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> ---
>> app/test-pmd/cmdline.c | 40 ++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 38 insertions(+), 2 deletions(-)
>>
>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>> index 5bf1497..09e30cf 100644
>> --- a/app/test-pmd/cmdline.c
>> +++ b/app/test-pmd/cmdline.c
>> @@ -1537,8 +1537,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");
>> @@ -1550,7 +1554,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;
>> + }
>> + }
>> +
>> + 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);
>> @@ -1609,7 +1632,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");
>> @@ -1623,7 +1649,17 @@ 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;
>> + return;
>> + }
>> cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
>> }
>>
>
> .
@@ -1537,8 +1537,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");
@@ -1550,7 +1554,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;
+ }
+ }
+
+ 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);
@@ -1609,7 +1632,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");
@@ -1623,7 +1649,17 @@ 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;
+ return;
+ }
cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
}