Re: [dpdk-dev] [PATCH 1/2] app/testpmd: add link speed check before port start

["Min Hu (Connor)" <humin29@huawei.com>]

在 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);
>>   }
>>
> 
> .