From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
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 <dev@dpdk.org>; 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 <dev@dpdk.org>; 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 <ferruh.yigit@intel.com>, <dev@dpdk.org>
CC: <xiaoyun.li@intel.com>
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)" <humin29@huawei.com>
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>
在 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);
>> }
>>
>
> .