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 4364EA0548;
Tue, 20 Apr 2021 14:11:54 +0200 (CEST)
Received: from [217.70.189.124] (localhost [127.0.0.1])
by mails.dpdk.org (Postfix) with ESMTP id C90F341749;
Tue, 20 Apr 2021 14:11:53 +0200 (CEST)
Received: from mga17.intel.com (mga17.intel.com [192.55.52.151])
by mails.dpdk.org (Postfix) with ESMTP id B273841735
for <dev@dpdk.org>; Tue, 20 Apr 2021 14:11:51 +0200 (CEST)
IronPort-SDR: 1HsrnIbr5QzMHUOLrUQ6Q8yJGPrhajRDX4yMCFdIO8K9TvuqPavjH7wubGx1NXJRsODHuxNaWm
LOGjO0JqglhA==
X-IronPort-AV: E=McAfee;i="6200,9189,9959"; a="175601013"
X-IronPort-AV: E=Sophos;i="5.82,236,1613462400"; d="scan'208";a="175601013"
Received: from fmsmga008.fm.intel.com ([10.253.24.58])
by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384;
20 Apr 2021 05:11:50 -0700
IronPort-SDR: lYbo37LVkCQ7YYqyw0VNok4nK+dAPBdgAfTzH5rgqYJBcb+aR3RhJrmUfTKQrGaRa6MMvNTyI7
zs8nZfaKiAqA==
X-IronPort-AV: E=Sophos;i="5.82,236,1613462400"; d="scan'208";a="420384584"
Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.207.27])
([10.213.207.27])
by fmsmga008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384;
20 Apr 2021 05:11:49 -0700
To: "Min Hu (Connor)" <humin29@huawei.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>
From: Ferruh Yigit <ferruh.yigit@intel.com>
X-User: ferruhy
Message-ID: <7272e681-af7c-c700-1107-66ad1ae7d051@intel.com>
Date: Tue, 20 Apr 2021 13:11:46 +0100
MIME-Version: 1.0
In-Reply-To: <1618813303-32945-2-git-send-email-humin29@huawei.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
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>
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.
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.
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?
> 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);
> }
>