On 4/16/2021 2:52 AM, Min Hu (Connor) wrote:
This patch adds multi-process support for testpmd.
The test cmd example as follows:
the primary cmd:
./dpdk-testpmd -a xxx --proc-type=auto -l 0-1 -- -i \
--rxq=4 --txq=4 --num-procs=2 --proc-id=0
the secondary cmd:
./dpdk-testpmd -a xxx --proc-type=auto -l 2-3 -- -i \
--rxq=4 --txq=4 --num-procs=2 --proc-id=1
Hi Connor,
Thanks for the update. +Anatoly as multi-process maintainer, and ethdev
maintainers.
Signed-off-by: Min Hu (Connor) <humi...@huawei.com>
Signed-off-by: Lijun Ou <ouli...@huawei.com>
Acked-by: Xiaoyun Li <xiaoyun...@intel.com>
Acked-by: Ajit Khaparde <ajit.khapa...@broadcom.com>
---
v9:
* Updated release notes and rst doc.
* Deleted deprecated codes.
* move macro and variable.
v8:
* Added warning info about queue numbers and process numbers.
v7:
* Fixed compiling error for unexpected unindent.
v6:
* Add rte flow description for multiple process.
v5:
* Fixed run_app.rst for multiple process description.
* Fix compiling error.
v4:
* Fixed minimum vlaue of Rxq or Txq in doc.
v3:
* Fixed compiling error using gcc10.0.
v2:
* Added document for this patch.
---
app/test-pmd/cmdline.c | 6 ++
app/test-pmd/config.c | 14 ++++-
app/test-pmd/parameters.c | 12 ++++
app/test-pmd/testpmd.c | 108 +++++++++++++++++++++++----------
app/test-pmd/testpmd.h | 3 +
doc/guides/rel_notes/release_21_05.rst | 1 +
doc/guides/testpmd_app_ug/run_app.rst | 86 ++++++++++++++++++++++++++
7 files changed, 196 insertions(+), 34 deletions(-)
diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 5bf1497..e465824 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -5354,6 +5354,12 @@ cmd_set_flush_rx_parsed(void *parsed_result,
__rte_unused void *data)
{
struct cmd_set_flush_rx *res = parsed_result;
+
+ if (num_procs > 1 && (strcmp(res->mode, "on") == 0)) {
+ printf("multi-process doesn't support to flush rx queues.\n");
+ return;
+ }
+
no_flush_rx = (uint8_t)((strcmp(res->mode, "on") == 0) ? 0 : 1);
}
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 40b2b29..c982c87 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -2860,6 +2860,8 @@ rss_fwd_config_setup(void)
queueid_t rxq;
queueid_t nb_q;
streamid_t sm_id;
+ int start;
+ int end;
nb_q = nb_rxq;
if (nb_q > nb_txq)
@@ -2877,7 +2879,15 @@ rss_fwd_config_setup(void)
init_fwd_streams();
setup_fwd_config_of_each_lcore(&cur_fwd_config);
- rxp = 0; rxq = 0;
+
+ if (proc_id > 0 && nb_q % num_procs)
+ printf("Warning! queue numbers should be multiple of "
+ "processes, or packet loss will happen.\n");
+
+ start = proc_id * nb_q / num_procs;
+ end = start + nb_q / num_procs;
+ rxp = 0;
+ rxq = start;
Can you put some comment above, on what/why is done, something similar to you
already put into the documentation?
for (sm_id = 0; sm_id < cur_fwd_config.nb_fwd_streams; sm_id++) {
struct fwd_stream *fs;
@@ -2894,6 +2904,8 @@ rss_fwd_config_setup(void)
continue;
rxp = 0;
rxq++;
+ if (rxq >= end)
+ rxq = start;
}
}
diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index f3954c1..d86cc21 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -508,6 +508,9 @@ parse_link_speed(int n)
void
launch_args_parse(int argc, char** argv)
{
+#define PARAM_PROC_ID "proc-id"
+#define PARAM_NUM_PROCS "num-procs"
+
int n, opt;
char **argvopt;
int opt_idx;
@@ -625,6 +628,8 @@ 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 },
+ { PARAM_NUM_PROCS, 1, 0, 0 },
+ { PARAM_PROC_ID, 1, 0, 0 },
{ 0, 0, 0, 0 },
};
@@ -1391,6 +1396,13 @@ launch_args_parse(int argc, char** argv)
record_core_cycles = 1;
if (!strcmp(lgopts[opt_idx].name, "record-burst-stats"))
record_burst_stats = 1;
+
To be consistent for rest, this empty line can be removed.
+ if (strncmp(lgopts[opt_idx].name,
+ PARAM_NUM_PROCS, 9) == 0)
+ num_procs = atoi(optarg);
+ if (strncmp(lgopts[opt_idx].name,
+ PARAM_PROC_ID, 7) == 0)
+ proc_id = atoi(optarg);
break;
case 'h':
usage(argv[0]);
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 96d2e0f..01d0d82 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -518,6 +518,16 @@ enum rte_eth_rx_mq_mode rx_mq_mode =
ETH_MQ_RX_VMDQ_DCB_RSS;
*/
uint32_t eth_link_speed;
+/*
+ * Id of the current process.
+ */
Can you please add more info, like this being related to the primary/secondary
process support and used to configure the queues to be polled.
+int proc_id;
+
+/*
+ * Number of processes.
+ */
+unsigned int num_procs = 1;
+
Ditto.
<...>
@@ -2511,21 +2537,28 @@ start_port(portid_t pid)
return -1;
}
/* configure port */
- diag = rte_eth_dev_configure(pi, nb_rxq + nb_hairpinq,
+ if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+ diag = rte_eth_dev_configure(pi,
+ nb_rxq + nb_hairpinq,
nb_txq + nb_hairpinq,
&(port->dev_conf));
- if (diag != 0) {
- if (rte_atomic16_cmpset(&(port->port_status),
- RTE_PORT_HANDLING, RTE_PORT_STOPPED) == 0)
- printf("Port %d can not be set back "
- "to stopped\n", pi);
- printf("Fail to configure port %d\n", pi);
- /* try to reconfigure port next time */
- port->need_reconfig = 1;
- return -1;
+ if (diag != 0) {
+ if (rte_atomic16_cmpset(
+ &(port->port_status),
+ RTE_PORT_HANDLING,
+ RTE_PORT_STOPPED) == 0)
+ printf("Port %d can not be set "
+ "back to stopped\n", pi);
+ printf("Fail to configure port %d\n",
+ pi);
+ /* try to reconfigure port next time */
+ port->need_reconfig = 1;
+ return -1;
+ }
Just an idea,
I am a little worried about the complexity this new process type checks are
added to the multiple locations. What do you think hiding process type checks
behind new functions, like:
eth_dev_configure_mp(...) {
if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
return rte_eth_dev_configure(..)
}
return 0;
}
and replace it with 'rte_eth_dev_configure()':
-diag = rte_eth_dev_configure(...)
+diag = eth_dev_configure_mp(...)
Do you think does this help reducing the complexity added by the multi-process
support?
}
}
- if (port->need_reconfig_queues > 0) {
+ if (port->need_reconfig_queues > 0 &&
+ rte_eal_process_type() == RTE_PROC_PRIMARY) {
According our coding convention, we don't allign with paranthesis but put double
tabs.
And what about creating an inline function for primary process check, like
is_proc_primary(), that may help.
port->need_reconfig_queues = 0;
/* setup tx queues */
for (qi = 0; qi < nb_txq; qi++) {
@@ -2626,17 +2659,20 @@ start_port(portid_t pid)
cnt_pi++;
/* start port */
- diag = rte_eth_dev_start(pi);
- if (diag < 0) {
- printf("Fail to start port %d: %s\n", pi,
- rte_strerror(-diag));
+ if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+ diag = rte_eth_dev_start(pi);
+ if (diag < 0) {
+ printf("Fail to start port %d: %s\n", pi,
+ rte_strerror(-diag));
- /* Fail to setup rx queue, return */
- if (rte_atomic16_cmpset(&(port->port_status),
+ /* Fail to setup rx queue, return */
+ if (rte_atomic16_cmpset(&(port->port_status),
RTE_PORT_HANDLING, RTE_PORT_STOPPED) == 0)
Syntax is wrong here after change, but if you go with above idea to hide process
type check within functions, you won't need to change these lines at all.
<...>
@@ -3885,8 +3925,10 @@ main(int argc, char** argv)
}
}
- if (!no_device_start && start_port(RTE_PORT_ALL) != 0)
+ if (!no_device_start && start_port(RTE_PORT_ALL) != 0) {
+ pmd_test_exit();
Why need to add 'pmd_test_exit()' for the multi-process support, if there is a
special case, please add comment for it.
