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 72148A034F; Thu, 25 Feb 2021 19:25:40 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 3BD86160885; Thu, 25 Feb 2021 19:25:17 +0100 (CET) Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by mails.dpdk.org (Postfix) with ESMTP id C288316084A for ; Thu, 25 Feb 2021 19:25:14 +0100 (CET) IronPort-SDR: TgeJkurgGD5mk1SOdLIxyrOjAqo5fXs5YCz5WvwvE701Hx9koftzK+/tU2zZ9SVBI7yF/Zl3mz DSQ+CEfm1EMw== X-IronPort-AV: E=McAfee;i="6000,8403,9906"; a="183169254" X-IronPort-AV: E=Sophos;i="5.81,206,1610438400"; d="scan'208";a="183169254" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 25 Feb 2021 10:25:13 -0800 IronPort-SDR: akSlDZ/QWbSOGqoOVc2Ro1uejs4q8bFJgtf83QkRMZzsS2b52laEAVi8rGBg8IbuJhpNwKOfB1 5eltQ8oGc5Cg== X-IronPort-AV: E=Sophos;i="5.81,206,1610438400"; d="scan'208";a="404495069" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.252.17.100]) ([10.252.17.100]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 25 Feb 2021 10:25:12 -0800 To: Ajit Khaparde , dev@dpdk.org References: <20210222191824.40230-1-ajit.khaparde@broadcom.com> From: Ferruh Yigit X-User: ferruhy Message-ID: <3a90da98-da41-70fb-2b6b-99e68c260b62@intel.com> Date: Thu, 25 Feb 2021 18:25:08 +0000 MIME-Version: 1.0 In-Reply-To: <20210222191824.40230-1-ajit.khaparde@broadcom.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH] app/testpmd: add support for forced ethernet speed 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" On 2/22/2021 7:18 PM, Ajit Khaparde wrote: > Add support for forced ethernet speed setting. > Currently testpmd tries to configure the Ethernet port in autoneg mode. > It is not possible to set the Ethernet port to a specific speed while > starting testpmd. In some cases capability to configure a forced speed > for the Ethernet port during initialization may be necessary. This patch > tries to add this support. > > The patch assumes full duplex setting and does not attempt to change that. > So speeds like 10M, 100M are not configurable using this method. > > The command line to configure a forced speed of 10G: > dpdk-testpmd -c 0xff -- -i --eth-link-speed 10000 > > The command line to configure a forced speed of 50G: > dpdk-testpmd -c 0xff -- -i --eth-link-speed 50000 > > Signed-off-by: Ajit Khaparde > --- > app/test-pmd/parameters.c | 42 +++++++++++++++++++++++++++ > app/test-pmd/testpmd.c | 4 +++ > app/test-pmd/testpmd.h | 1 + > doc/guides/testpmd_app_ug/run_app.rst | 11 +++++++ > 4 files changed, 58 insertions(+) Can you also update the release notes to document the new parameter? > > diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c > index c8acd5d1b7..e10f7d38fb 100644 > --- a/app/test-pmd/parameters.c > +++ b/app/test-pmd/parameters.c > @@ -224,6 +224,7 @@ usage(char* progname) > printf(" --hairpin-mode=0xXX: bitmask set the hairpin port mode.\n " > " 0x10 - explicit Tx rule, 0x02 - hairpin ports paired\n" > " 0x01 - hairpin ports loop, 0x00 - hairpin port self\n"); > + printf(" --eth-link-speed: forced link speed.\n"); > } > > #ifdef RTE_LIB_CMDLINE > @@ -485,6 +486,41 @@ parse_event_printing_config(const char *optarg, int enable) > return 0; > } > > +static int > +parse_link_speed(int n) > +{ > + uint32_t speed; > + > + switch (n) { OK to not support "10M, 100M", not sure if anybody really uses them, but what do you think checking them and return an error? > + case 1000: > + speed = ETH_LINK_SPEED_1G; > + break; > + case 10000: > + speed = ETH_LINK_SPEED_10G; > + break; > + case 25000: > + speed = ETH_LINK_SPEED_25G; > + break; > + case 40000: > + speed = ETH_LINK_SPEED_40G; > + break; > + case 50000: > + speed = ETH_LINK_SPEED_50G; > + break; > + case 100000: > + speed = ETH_LINK_SPEED_100G; > + break; > + case 200000: > + speed = ETH_LINK_SPEED_200G; > + break; > + default: > + speed = ETH_LINK_SPEED_AUTONEG; > + break; Isn't this function to set a fixed link speed, why falling back to autoneg? Also shouldn't this function set 'ETH_LINK_SPEED_FIXED' too? > + } > + > + return speed; > +} > + > void > launch_args_parse(int argc, char** argv) > { > @@ -605,6 +641,7 @@ launch_args_parse(int argc, char** argv) > { "rx-mq-mode", 1, 0, 0 }, > { "record-core-cycles", 0, 0, 0 }, > { "record-burst-stats", 0, 0, 0 }, > + { "eth-link-speed", 1, 0, 0 }, > { 0, 0, 0, 0 }, > }; > > @@ -1366,6 +1403,11 @@ launch_args_parse(int argc, char** argv) > record_core_cycles = 1; > if (!strcmp(lgopts[opt_idx].name, "record-burst-stats")) > record_burst_stats = 1; > + if (!strcmp(lgopts[opt_idx].name, "eth-link-speed")) { > + n = atoi(optarg); > + if (n >= 0 && parse_link_speed(n) >= 0) > + eth_link_speed = parse_link_speed(n); > + } > break; > case 'h': > usage(argv[0]); > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c > index caa711d6f3..9434e335a0 100644 > --- a/app/test-pmd/testpmd.c > +++ b/app/test-pmd/testpmd.c > @@ -535,6 +535,8 @@ uint16_t gso_max_segment_size = RTE_ETHER_MAX_LEN - RTE_ETHER_CRC_LEN; > /* Holds the registered mbuf dynamic flags names. */ > char dynf_names[64][RTE_MBUF_DYN_NAMESIZE]; > > +uint32_t eth_link_speed; > + Can you please move this a little up, at the beginning of the 'testpmd.c' there are global config variables, this can go next to them. And can you please add a single line comment to the variable, as done in all other ones. > /* > * Helper function to check if socket is already discovered. > * If yes, return positive value. If not, return zero. > @@ -1484,6 +1486,8 @@ init_config(void) > port->tx_conf[k].offloads = > port->dev_conf.txmode.offloads; > > + port->dev_conf.link_speeds = eth_link_speed; > + This is set even user doesn't provide '--eth-link-speed' at all, in that case I assume it relies on the fact that variable default value (0) is the same as 'AUTONEG' value (0). So it sets speed to autoneg in that case. But what do you think to be more explicit, and set this only if the user provided the device argument, like: if (eth_link_speed) port->dev_conf.link_speeds = eth_link_speed; This sets the same link speed for all ports, do you think may we need capability to set link speed per port? Does it worth the complexity it brings? > /* set flag to initialize port/queue */ > port->need_reconfig = 1; > port->need_reconfig_queues = 1; > diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h > index 60ddeb8f13..a3cd4a0e16 100644 > --- a/app/test-pmd/testpmd.h > +++ b/app/test-pmd/testpmd.h > @@ -351,6 +351,7 @@ extern bool setup_on_probe_event; /**< disabled by port setup-on iterator */ > extern uint8_t hot_plug; /**< enable by "--hot-plug" parameter */ > extern int do_mlockall; /**< set by "--mlockall" or "--no-mlockall" parameter */ > extern uint8_t clear_ptypes; /**< disabled by set ptype cmd */ > +extern uint32_t eth_link_speed; > > #ifdef RTE_LIBRTE_IXGBE_BYPASS > extern uint32_t bypass_timeout; /**< Store the NIC bypass watchdog timeout */ > diff --git a/doc/guides/testpmd_app_ug/run_app.rst b/doc/guides/testpmd_app_ug/run_app.rst > index 6745072329..a856f52736 100644 > --- a/doc/guides/testpmd_app_ug/run_app.rst > +++ b/doc/guides/testpmd_app_ug/run_app.rst > @@ -536,3 +536,14 @@ The command line options are: > bit 1 - two hairpin ports paired > bit 0 - two hairpin ports loop > The default value is 0. Hairpin will use single port mode and implicit Tx flow mode. > + > +* ``--eth-link-speed`` > + Should this document that "10M, 100M" is not supported? > + Set a forced link speed to the ethernet port. > + 1000 - 1Gbps > + 10000 - 10Gbps > + 25000 - 25Gbps > + 40000 - 40Gbps > + 50000 - 50Gbps > + 100000 - 100Gbps > + 200000 - 200Gbps What do you think to put something like "..." to very bottom, to clarify this is not full list, so that we won't need to update this each time we add a new speed?