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 48243A0561; Fri, 5 Mar 2021 05:15:28 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0913440A4B; Fri, 5 Mar 2021 05:15:28 +0100 (CET) Received: from mail-qt1-f174.google.com (mail-qt1-f174.google.com [209.85.160.174]) by mails.dpdk.org (Postfix) with ESMTP id 531DE4069F for ; Fri, 5 Mar 2021 05:15:26 +0100 (CET) Received: by mail-qt1-f174.google.com with SMTP id b19so764457qto.11 for ; Thu, 04 Mar 2021 20:15:26 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=broadcom.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=1DQlZHcjBuU+qMUzyfuHb6DMsJV0fS+xS22gdEaDnok=; b=D0cCbV6y/bxG4R/eHuBAkHh0CvqGEC6TGoJzm+xKxf/qNbow8coPm8Zt78BzxWMBN4 zLdMOZzZCbW/wZ3YW1bJm87CXJsAY4OU206y8accxethlvjwUmt+g1OFOpimbjsczvJv tOiQYhZtIltGzXZ7w5dm4H9JURItSggTtUO+o= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=1DQlZHcjBuU+qMUzyfuHb6DMsJV0fS+xS22gdEaDnok=; b=FM3qwozu9MqohqHxUlYFsXyVsNVOopQAKaGOp5C7URAqaW9Kz9xwrKLdIfZBWc0OFj qeolhycIQdL9QIoTt0AG5K96+V4xVk+g8o7B7Xuc2vTqpq+LAbjYUJWwi3Vc0dhtaNa4 fbbCGulgh0+hgwsi+jd1C1Bqti6l8PUzYwFP0rAVCYlGRJaLRH3RSxUBMebSHl1k07tZ H8YAOz09Wlo8tgkYQMYvHWIKDEB66z1cLUt3Go5mdPTp19pDO8xujwQvYGMaChqxyCIB Xw0qFtWgleYI5eD9lalM8H3qHPt4kW66S33WWIjsqh5mQCnVo8OHyCrFwu6qpOPC5IFb bR4w== X-Gm-Message-State: AOAM531gJJqW7EMNYeeEcJxqgdr25RxMbLza/a/J3I+IfuiF5he9XK6P SVNwmChCDjwSYTAPPf9t0/6FLGEkJ0FAHRXvZhERLTFSdSVQqQ== X-Google-Smtp-Source: ABdhPJx7HY+pWhaxbOtrgdP3039craEastFiAuMKsdw2fHi0bTzqDA3H6XmXWcZ/xMO4yUQ99l/zl44+CXaRYQl4uTA= X-Received: by 2002:ac8:6798:: with SMTP id b24mr7153063qtp.371.1614917725556; Thu, 04 Mar 2021 20:15:25 -0800 (PST) MIME-Version: 1.0 References: <20210222191824.40230-1-ajit.khaparde@broadcom.com> <3a90da98-da41-70fb-2b6b-99e68c260b62@intel.com> In-Reply-To: <3a90da98-da41-70fb-2b6b-99e68c260b62@intel.com> From: Ajit Khaparde Date: Thu, 4 Mar 2021 20:15:09 -0800 Message-ID: To: Ferruh Yigit Cc: dpdk-dev Content-Type: multipart/signed; protocol="application/pkcs7-signature"; micalg=sha-256; boundary="00000000000065b1d105bcc255c9" X-Content-Filtered-By: Mailman/MimeDel 2.1.29 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" --00000000000065b1d105bcc255c9 Content-Type: text/plain; charset="UTF-8" On Thu, Feb 25, 2021 at 10:25 AM Ferruh Yigit wrote: > 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? > Done > > > > > 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? > Ok. Added this. > > > + 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? > Good point. I removed it. > > Also shouldn't this function set 'ETH_LINK_SPEED_FIXED' too? > Yes. Done in v2. > > > + } > > + > > + 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. > Done in v2. > > And can you please add a single line comment to the variable, as done in > all > other ones. > Sure. Done in v2. > > > /* > > * 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; > Done in v2. > > > 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? > It is turning out to be a bit complex. For this round I think let's keep it simple. > > > /* 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? > Done in v2. > > > + 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? > Done. --00000000000065b1d105bcc255c9--