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 9D0DFA0524; Thu, 6 May 2021 08:46:45 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 17CF2410DB; Thu, 6 May 2021 08:46:45 +0200 (CEST) Received: from szxga07-in.huawei.com (szxga07-in.huawei.com [45.249.212.35]) by mails.dpdk.org (Postfix) with ESMTP id DDCB840040 for ; Thu, 6 May 2021 08:46:42 +0200 (CEST) Received: from DGGEMS404-HUB.china.huawei.com (unknown [172.30.72.58]) by szxga07-in.huawei.com (SkyGuard) with ESMTP id 4FbPDy1303zB3yH for ; Thu, 6 May 2021 14:44:02 +0800 (CST) Received: from [10.66.74.184] (10.66.74.184) by DGGEMS404-HUB.china.huawei.com (10.3.19.204) with Microsoft SMTP Server id 14.3.498.0; Thu, 6 May 2021 14:46:36 +0800 To: "Li, Xiaoyun" , "Yigit, Ferruh" CC: "dev@dpdk.org" 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> <95042d3b-408a-a290-de18-a887de558555@huawei.com> <2574477f-8eb7-91ab-6cc2-2f11434fee89@huawei.com> From: Huisong Li Message-ID: Date: Thu, 6 May 2021 14:46:36 +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/5/6 10:36, Li, Xiaoyun 写道: > >> -----Original Message----- >> From: Huisong Li >> Sent: Tuesday, May 4, 2021 09:46 >> To: Li, Xiaoyun >> Cc: Yigit, Ferruh ; dev@dpdk.org >> Subject: Re: [dpdk-dev] [PATCH v2 1/2] app/testpmd: add link speed check >> before port start >> >> >> 在 2021/4/30 12:46, Li, Xiaoyun 写道: >>>> -----Original Message----- >>>> From: Huisong Li >>>> Sent: Friday, April 30, 2021 12:04 >>>> To: Li, Xiaoyun >>>> Cc: Yigit, Ferruh ; dev@dpdk.org >>>> Subject: Re: [dpdk-dev] [PATCH v2 1/2] app/testpmd: add link speed >>>> check before port start >>>> >>>> >>>> 在 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. >>> I still think it's not necessary. >> ok😂 >> >> @Ferruh, what do you think? >> >>>> 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); is at the end of this cmd. >> This will re-config all ports. >>> I don't understand why can't you add a restoring in this if and break this loop >> and do this reconfig. >> What do you mean? If a port fails among all ports, only the failed port is >> restored and reconfigured. I suppose that's what you mean? >> The cmd "port config all speed xxx duplex xxx" applies to all ports. If it fails to be >> delivered, the user considers that the cmd does not take effect. >> So it is necessary to restore and reconfigure all ports to the previous state. > You still don't understand me. See below. I mean the following. > You don't need to got to rollback. Because "cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);" will config all ports anyway already no matter you restore speeds or not. > Can you read the code carefully? "cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);" is always there and it configs ALL ports. > > static void > cmd_config_speed_all_parsed(void *parsed_result, > __rte_unused struct cmdline *cl, > __rte_unused void *data) > { > struct cmd_config_speed_all *res = parsed_result; > uint32_t link_speed; > portid_t pid; > > if (!all_ports_stopped()) { > printf("Please stop all ports first\n"); > return; > } > > if (parse_and_check_speed_duplex(res->value1, res->value2, > &link_speed) < 0) > 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); > + for (i = 0; i <= pid; i++) { > + port = &ports[i]; > + port->dev_conf.link_speeds = old_link_speeds[i]; > + } > + break; > + } > } > > cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1); > } It looks good!  Now we're left with a question about whether we need to add this link speed check. @Ferruh, what do you think? >>>>>> + } >>>>>> + } >>>>>> + >>>>>> + 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 >>>>> .