Re: [dpdk-dev] [PATCH v8] app/testpmd: support multi-process

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 3/30/2021 2:48 AM, Min Hu (Connor) wrote:
> From: Lijun Ou <oulijun@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) <humin29@huawei.com>
> Signed-off-by: Lijun Ou <oulijun@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.
>