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.