On 3/30/2021 2:48 AM, Min Hu (Connor) wrote:
From: Lijun Ou <ouli...@huawei.com>
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,
Please find a few minor comments below, since they are minor issues please feel
free to keep the existing acks in next version.
Signed-off-by: Min Hu (Connor) <humi...@huawei.com>
Signed-off-by: Lijun Ou <ouli...@huawei.com>
---
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 | 12 +++-
app/test-pmd/config.c | 14 +++-
app/test-pmd/parameters.c | 11 +++
app/test-pmd/testpmd.c | 127 ++++++++++++++++++++++------------
app/test-pmd/testpmd.h | 7 ++
doc/guides/testpmd_app_ug/run_app.rst | 101 +++++++++++++++++++++++++++
Can you please add a release notes update too?
6 files changed, 226 insertions(+), 46 deletions(-)
diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index f44116b..38c97e3 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -71,8 +71,6 @@
#include "cmdline_tm.h"
#include "bpf_cmd.h"
-static struct cmdline *testpmd_cl;
-
static void cmd_reconfig_device_queue(portid_t id, uint8_t dev, uint8_t
queue);
/* *** Help command with introduction. *** */
@@ -5351,6 +5349,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);
}
@@ -17229,6 +17233,10 @@ prompt(void)
printf("Cannot set exit function for cmdline\n");
cmdline_interact(testpmd_cl);
+ if (unlikely(f_quit == 1)) {
+ dup2(testpmd_fd_copy, testpmd_cl->s_in);
+ close(testpmd_fd_copy);
+ }
Why this is needed? Can you put some comment on the code to explain why this is
needed?
if (ret != 0)
cmdline_stdin_exit(testpmd_cl);
}
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index ef0b978..ec7d030 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -2827,6 +2827,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)
@@ -2844,7 +2846,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;
for (sm_id = 0; sm_id < cur_fwd_config.nb_fwd_streams; sm_id++) {
struct fwd_stream *fs;
@@ -2861,6 +2871,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 a326c8c..ec3bc62 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -45,6 +45,8 @@
#include <rte_flow.h>
#include "testpmd.h"
+#define PARAM_PROC_ID "proc-id"
+#define PARAM_NUM_PROCS "num-procs"
Macros just after include is not good, can you please move these just after
'launch_args_parse()' to keep locality.
static void
usage(char* progname)
@@ -644,6 +646,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 },
};
@@ -1410,6 +1414,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;
+
+ if (strncmp(lgopts[opt_idx].name,
+ PARAM_NUM_PROCS, 8) == 0)
9?
"num-procs"
123456789
+ 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..c31234e 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -63,6 +63,9 @@
#include "testpmd.h"
+int testpmd_fd_copy; /* the copy of STDIN_FILENO */
+struct cmdline *testpmd_cl;
+
Again the global variables just after include is not good place.
What do you think to keep 'testpmd_cl' in the 'cmdline.c', since the object
allocated there and the variable seems match more to that file. 'extern' already
added to header, so it can be used here.
'testpmd_fd_copy' can be moved at the bottom of the block that has global
variables, and please add more comment on what this variable is for.
#ifndef MAP_HUGETLB
/* FreeBSD may not have MAP_HUGETLB (in fact, it probably doesn't) */
#define HUGE_FLAG (0x40000)
@@ -125,6 +128,9 @@ uint8_t port_numa[RTE_MAX_ETHPORTS];
*/
uint8_t rxring_numa[RTE_MAX_ETHPORTS];
+int proc_id;
+unsigned int num_procs = 1;
+
Similar comments for these global variables.
The testpmd is already complex, it is good to have multi process support for the
testpmd but the concern is additional complexity it bring, it may lead very
tangled senarious. Please add much more comment on the multi process related
additions.
There is a block at the beggining of the this file, that has all global
variables with enough comments on them, can we follow same for these new global
variables.
Group all multiprocess related variables together, appent to the global
variables block, and comment all the variables in detail?
/*
* Store specified sockets on which TX ring to be used by ports
* is allocated.
@@ -977,6 +983,11 @@ mbuf_pool_create(uint16_t mbuf_seg_size, unsigned nb_mbuf,
mb_size = sizeof(struct rte_mbuf) + mbuf_seg_size;
mbuf_poolname_build(socket_id, pool_name, sizeof(pool_name), size_idx);
+ if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
+ rte_mp = rte_mempool_lookup(pool_name);
+ goto err;
+ }
+
TESTPMD_LOG(INFO,
"create a new mbuf pool <%s>: n=%u, size=%u, socket=%u\n",
pool_name, nb_mbuf, mbuf_seg_size, socket_id);
@@ -1059,9 +1070,14 @@ mbuf_pool_create(uint16_t mbuf_seg_size, unsigned
nb_mbuf,
err:
if (rte_mp == NULL) {
- rte_exit(EXIT_FAILURE,
- "Creation of mbuf pool for socket %u failed: %s\n",
- socket_id, rte_strerror(rte_errno));
+ if (rte_eal_process_type() == RTE_PROC_PRIMARY)
+ rte_exit(EXIT_FAILURE,
+ "Creation of mbuf pool for socket %u failed:
%s\n",
+ socket_id, rte_strerror(rte_errno));
+ else
+ rte_exit(EXIT_FAILURE,
+ "Get mbuf pool for socket %u failed: %s\n",
+ socket_id, rte_strerror(rte_errno));
} else if (verbose_level > 0) {
rte_mempool_dump(stdout, rte_mp);
}
@@ -2002,6 +2018,12 @@ flush_fwd_rx_queues(void)
uint64_t prev_tsc = 0, diff_tsc, cur_tsc, timer_tsc = 0;
uint64_t timer_period;
+ if (num_procs > 1) {
+ printf("multi-process not support for flushing fwd rx "
+ "queues, skip the below lines and return.\n");
+ return;
+ }
+
/* convert to number of cycles */
timer_period = rte_get_timer_hz(); /* 1 second timeout */
@@ -2511,21 +2533,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;
+ }
}
}
- if (port->need_reconfig_queues > 0) {
+ if (port->need_reconfig_queues > 0 &&
+ rte_eal_process_type() == RTE_PROC_PRIMARY) {
port->need_reconfig_queues = 0;
/* setup tx queues */
for (qi = 0; qi < nb_txq; qi++) {
@@ -2626,17 +2655,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)
- printf("Port %d can not be set back to "
- "stopped\n", pi);
- continue;
+ printf("Port %d can not be set back to "
+ "stopped\n",
+ pi);
+ continue;
+ }
}
if (rte_atomic16_cmpset(&(port->port_status),
@@ -2765,7 +2797,8 @@ stop_port(portid_t pid)
if (port->flow_list)
port_flow_flush(pi);
- if (rte_eth_dev_stop(pi) != 0)
+ if (rte_eal_process_type() == RTE_PROC_PRIMARY &&
+ rte_eth_dev_stop(pi) != 0)
RTE_LOG(ERR, EAL, "rte_eth_dev_stop failed for port
%u\n",
pi);
@@ -2834,8 +2867,10 @@ close_port(portid_t pid)
continue;
}
- port_flow_flush(pi);
- rte_eth_dev_close(pi);
+ if (rte_eal_process_type() == RTE_PROC_PRIMARY)
+ port_flow_flush(pi);
+ if (rte_eal_process_type() == RTE_PROC_PRIMARY)
+ rte_eth_dev_close(pi);
Can combine two calls to prevent redundant check.
}
remove_invalid_ports();
@@ -3099,7 +3134,7 @@ pmd_test_exit(void)
}
}
for (i = 0 ; i < RTE_DIM(mempools) ; i++) {
- if (mempools[i])
+ if (rte_eal_process_type() == RTE_PROC_PRIMARY && mempools[i])
rte_mempool_free(mempools[i]);
}
@@ -3621,6 +3656,10 @@ init_port_dcb_config(portid_t pid,
int retval;
uint16_t i;
+ if (num_procs > 1) {
+ printf("The multi-process feature doesn't support dcb.\n");
+ return -ENOTSUP;
+ }
rte_port = &ports[pid];
memset(&port_conf, 0, sizeof(struct rte_eth_conf));
@@ -3719,13 +3758,6 @@ init_port(void)
}
static void
-force_quit(void)
-{
- pmd_test_exit();
- prompt_exit();
-}
-
-static void
print_stats(void)
{
uint8_t i;
@@ -3756,12 +3788,16 @@ signal_handler(int signum)
if (latencystats_enabled != 0)
rte_latencystats_uninit();
#endif
- force_quit();
/* Set flag to indicate the force termination. */
f_quit = 1;
- /* exit with the expected status */
- signal(signum, SIG_DFL);
- kill(getpid(), signum);
+ if (interactive == 1) {
+ dup2(testpmd_cl->s_in, testpmd_fd_copy);
+ close(testpmd_cl->s_in);
+ } else {
+ dup2(0, testpmd_fd_copy);
+ close(0);
+ }
+
Similarly can you please comment the code why this is needed.
}
}
@@ -3786,10 +3822,6 @@ main(int argc, char** argv)
rte_exit(EXIT_FAILURE, "Cannot init EAL: %s\n",
rte_strerror(rte_errno));
- if (rte_eal_process_type() == RTE_PROC_SECONDARY)
- rte_exit(EXIT_FAILURE,
- "Secondary process type not supported.\n");
-
ret = register_eth_event_callback();
if (ret != 0)
rte_exit(EXIT_FAILURE, "Cannot register for ethdev events");
@@ -3885,8 +3917,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();
rte_exit(EXIT_FAILURE, "Start ports failed\n");
+ }
/* set all ports to promiscuous mode by default */
RTE_ETH_FOREACH_DEV(port_id) {
@@ -3932,6 +3966,8 @@ main(int argc, char** argv)
}
prompt();
pmd_test_exit();
+ if (unlikely(f_quit == 1))
+ prompt_exit();
Why additional prompt_exit() is required?
} else
#endif
{
@@ -3967,6 +4003,11 @@ main(int argc, char** argv)
printf("Press enter to exit\n");
rc = read(0, &c, 1);
pmd_test_exit();
+ if (unlikely(f_quit == 1)) {
+ dup2(testpmd_fd_copy, 0);
+ close(testpmd_fd_copy);
+ prompt_exit();
+ }
By definition 'f_quit', force quit, is added to support exiting in container
environment, since they can't exit with SIGINT/SIGTERM, so main thing with this
global variable should be breaking the loop in main function.
I wonder why this variable used extensively in this patch, is it used in
different purpose here? will quit/Ctrl-C behaviour differ in multi process?
if (rc < 0)
return 1;
}
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index a87ccb0..640a377 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -13,6 +13,7 @@
#include <rte_gso.h>
#include <cmdline.h>
#include <sys/queue.h>
+#include "cmdline.h"
#define RTE_PORT_ALL (~(portid_t)0x0)
@@ -24,6 +25,10 @@
#define RTE_PORT_CLOSED (uint16_t)2
#define RTE_PORT_HANDLING (uint16_t)3
+extern uint8_t f_quit;
+extern int testpmd_fd_copy;
+extern struct cmdline *testpmd_cl;
+
Again can you please move them in the file, there is already a block in below
for extern variables, please place there with sufficient comments.
/*
* It is used to allocate the memory for hash key.
* The hash key size is NIC dependent.
@@ -423,6 +428,8 @@ extern uint64_t noisy_lkup_mem_sz;
extern uint64_t noisy_lkup_num_writes;
extern uint64_t noisy_lkup_num_reads;
extern uint64_t noisy_lkup_num_reads_writes;
+extern int proc_id;
+extern unsigned int num_procs;
Can you pleaes group multi process related changes together?
extern uint8_t dcb_config;
extern uint8_t dcb_test;
diff --git a/doc/guides/testpmd_app_ug/run_app.rst
b/doc/guides/testpmd_app_ug/run_app.rst
index ec1dc7d..7e60e80 100644
--- a/doc/guides/testpmd_app_ug/run_app.rst
+++ b/doc/guides/testpmd_app_ug/run_app.rst
@@ -551,3 +551,104 @@ 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.
+
+
+Testpmd Support Multi Process Command-line Options
+--------------------------------------------------
+
+The following are the command-line options for the testpmd applications(support
+multi process).They must be separated from the EAL options, shown in the
previous
+section, with a ``--`` separator:
+
+.. code-block:: console
+
+ primary process:
+ sudo ./dpdk-testpmd -a xxx --proc-type=auto -l 0-1 -- -i --rxq=4
--txq=4 \
+ --num-procs=2 --proc-id=0
+
+ secondary process:
+ sudo ./dpdk-testpmd -a xxx --proc-type=auto -l 2-3 -- -i --rxq=4
--txq=4 \
+ --num-procs=2 --proc-id=1
+
+The command line options are:
+
+* ``-a, --allow``
+
+ Add a device to the allow list. ``xxx`` means device used which should be
the
+ same in primary process and secondary process.
+
+* ``--proc-type``
+
+ Specify a given process instance as the primary or secondary DPDK instance.
+ ``auto`` set here is OK.
+
+* ``-l CORELIST``
+
+ List of cores to run on. the corelist should be different in primary
process and
+ secondary process.
+
+1 for documentation, but above are eal parameters, what about not repeting them
here but referencing eal documentation,
'doc/guides/linux_gsg/eal_args.include.rst', for it?
+* ``--rxq=N``
+
+ Set the number of Rx queues per port to N. N is the sum of queues used by
primary
+ and secondary process. Primary process and secondary process should have
separate
+ queues, and each should occupy at least one queue. Where N should be the
multiple
+ of number of processes.
+
+* ``--txq=N``
+
+ Set the number of Tx queues per port to N. N is the sum of queues used by
primary
+ and secondary process. Primary process and secondary process should have
separate
+ queues, and each should occupy at least one queue. Where N should be the
multiple
+ of number of processes.
+
+* ``--num-procs=N``
+
+ The number of processes which will be used.
+
+* ``--proc-id=id``
+
+ The id of the current process (id < num-procs). id should be different in
primary
+ process and secondary process.
+
+Calculation rule for queue:
+All queues are allocated to different processes based on proc_num and proc_id.
+Calculation rule for the Testpmd to allocate queues to each process:
+start(queue start id) = proc_id * nb_q / num_procs；
+end(queue end id) = start + nb_q / num_procs；
+
+For example, if supports 4 txq and rxq
+the 0~1 for primary process
+the 2~3 for secondary process
+
+The number of rings should be a multiple of the number of processes. If not,
+redundant queues will exist after queues are allocated to processes. After RSS
is
+enabled, packet loss occurs when traffic is sent to all processes at the same
time.
+Some traffic enters redundant queues and cannot be forwarded.
+
+Most dev ops is supported in primary and secondary process. While secondary
process
+is not permitted to allocate or release shared memory, so some ops are not
supported
+as follows:
+``dev_configure``
+``dev_start``
+``dev_stop``
+``rx_queue_setup``
+``tx_queue_setup``
+``rx_queue_release``
+``tx_queue_release``
+
+So, any command from testpmd which calls those APIs will not be supported in
secondary
+process, like:
+``port config all rxq|txq|rxd|txd <value>``
+``port config <port_id> rx_offload xxx on/off ``
+``port config <port_id> tx_offload xxx on/off``
+etc.
+
+RTE_FLOW supported, it applies only on its own process on SW side, but all on
HW size.
+stats supported, stats will not change when one quit and start, As they share
the same
+buffer to store the stats. Flow rules are maintained in process level: primary
and secondary
+has its own flow list(but one flow list in HW). The two can see all the
queues, so setting
+the flow rules for the other is OK. Of course, io(receive or transmit packets)
in the queue
+from others is not permitted.
+
+RSS supported, Primary process and secondary process has separate queues to
use, RSS
+will work in their own queues whether primary and secondary process.
