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 B7749A0546; Fri, 30 Apr 2021 06:04:16 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 3519B40693; Fri, 30 Apr 2021 06:04:15 +0200 (CEST) Received: from szxga05-in.huawei.com (szxga05-in.huawei.com [45.249.212.191]) by mails.dpdk.org (Postfix) with ESMTP id 94E1940395 for ; Fri, 30 Apr 2021 06:04:13 +0200 (CEST) Received: from DGGEMS403-HUB.china.huawei.com (unknown [172.30.72.60]) by szxga05-in.huawei.com (SkyGuard) with ESMTP id 4FWdvd25zZzPvsY for ; Fri, 30 Apr 2021 12:01:01 +0800 (CST) Received: from [10.66.74.184] (10.66.74.184) by DGGEMS403-HUB.china.huawei.com (10.3.19.203) with Microsoft SMTP Server id 14.3.498.0; Fri, 30 Apr 2021 12:04:07 +0800 To: "Li, Xiaoyun" References: <1618813303-32945-1-git-send-email-humin29@huawei.com> <1619599019-46246-1-git-send-email-humin29@huawei.com> <1619599019-46246-2-git-send-email-humin29@huawei.com> From: Huisong Li CC: "Yigit, Ferruh" , "dev@dpdk.org" Message-ID: <95042d3b-408a-a290-de18-a887de558555@huawei.com> Date: Fri, 30 Apr 2021 12:04:07 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.66.74.184] X-CFilter-Loop: Reflected Subject: Re: [dpdk-dev] [PATCH v2 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/30 11:19, Li, Xiaoyun 写道: >> -----Original Message----- >> From: Min Hu (Connor) >> Sent: Wednesday, April 28, 2021 16:37 >> To: dev@dpdk.org >> Cc: Yigit, Ferruh ; Li, Xiaoyun >> Subject: [PATCH v2 1/2] app/testpmd: add link speed check before port start >> >> 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. > 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. 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 >> Signed-off-by: Min Hu (Connor) >> --- >> 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); >> + >> + 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 > .