From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 43066A0547; Wed, 21 Apr 2021 05:45:02 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1638A418DB; Wed, 21 Apr 2021 05:45:02 +0200 (CEST) Received: from szxga07-in.huawei.com (szxga07-in.huawei.com [45.249.212.35]) by mails.dpdk.org (Postfix) with ESMTP id 142A0418AA for ; Wed, 21 Apr 2021 05:45:00 +0200 (CEST) Received: from DGGEMS414-HUB.china.huawei.com (unknown [172.30.72.58]) by szxga07-in.huawei.com (SkyGuard) with ESMTP id 4FQ5wV6gcPz7wYN for ; Wed, 21 Apr 2021 11:42:34 +0800 (CST) Received: from [10.67.103.128] (10.67.103.128) by DGGEMS414-HUB.china.huawei.com (10.3.19.214) with Microsoft SMTP Server id 14.3.498.0; Wed, 21 Apr 2021 11:44:53 +0800 To: Ferruh Yigit , CC: References: <1618813303-32945-1-git-send-email-humin29@huawei.com> <1618813303-32945-2-git-send-email-humin29@huawei.com> <7272e681-af7c-c700-1107-66ad1ae7d051@intel.com> From: "Min Hu (Connor)" Message-ID: <88a432df-6da7-1e99-1adb-36343982cc8e@huawei.com> Date: Wed, 21 Apr 2021 11:44:53 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.3.1 MIME-Version: 1.0 In-Reply-To: <7272e681-af7c-c700-1107-66ad1ae7d051@intel.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.103.128] X-CFilter-Loop: Reflected Subject: Re: [dpdk-dev] [PATCH 1/2] app/testpmd: add link speed check before port start X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 在 2021/4/20 20:11, Ferruh Yigit 写道: > On 4/19/2021 7:21 AM, Min Hu (Connor) wrote: >> From: Huisong Li >> >> 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 >> Signed-off-by: Min Hu (Connor) >> --- >>   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); >>   } >> > > .