[dpdk-dev] [PATCH 1/2] app/testpmd: add link speed check before port start
Min Hu (Connor)
humin29 at huawei.com
Wed Apr 21 05:44:53 CEST 2021
在 2021/4/20 20:11, Ferruh Yigit 写道:
> On 4/19/2021 7:21 AM, Min Hu (Connor) wrote:
>> From: Huisong Li <lihuisong at 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 at huawei.com>
>> Signed-off-by: Min Hu (Connor) <humin29 at 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);
>> }
>>
>
> .
More information about the dev
mailing list