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

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

On 1/8/2021 9:46 AM, Lijun Ou wrote:
> This patch adds multi-process support for testpmd.
> The test cmd example as follows:
> the primary cmd:
> ./testpmd -w xxx --file-prefix=xx -l 0-1 -n 2 -- -i\
> --rxq=16 --txq=16 --num-procs=2 --proc-id=0
> the secondary cmd:
> ./testpmd -w xxx --file-prefix=xx -l 2-3 -n 2 -- -i\
> --rxq=16 --txq=16 --num-procs=2 --proc-id=1
> 

+1 to add multi-process support to testpmd, missing it reduces multi-process 
testing a lot, but I guess concern is it may cause lots of changes and create 
some corner cases in testpmd.

Can you please explain a little why new 'proc-id' testpmd argument is needed, 
why the regular way of using eal '--proc-type' command and 'RTE_PROC_SECONDARY' 
checks is not enough?

Also why 'MULTIPLE_PROCESS_HANDLE' define is required?

Thanks,
ferruh

> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> ---
>   app/test-pmd/cmdline.c    |   6 ++-
>   app/test-pmd/config.c     |   9 +++-
>   app/test-pmd/parameters.c |   9 ++++
>   app/test-pmd/testpmd.c    | 133 ++++++++++++++++++++++++++++++++--------------
>   app/test-pmd/testpmd.h    |   7 +++
>   5 files changed, 121 insertions(+), 43 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 2ccbaa0..2237c9f 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. *** */
> @@ -17115,6 +17113,10 @@ prompt(void)
>   	if (testpmd_cl == NULL)
>   		return;
>   	cmdline_interact(testpmd_cl);
> +	if (unlikely(f_quit == 1)) {
> +		dup2(testpmd_fd_copy, testpmd_cl->s_in);
> +		close(testpmd_fd_copy);
> +	}
>   	cmdline_stdin_exit(testpmd_cl);
>   }
>   
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 3f6c864..1702e0d 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -3100,6 +3100,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)
> @@ -3117,7 +3119,10 @@ rss_fwd_config_setup(void)
>   	init_fwd_streams();
>   
>   	setup_fwd_config_of_each_lcore(&cur_fwd_config);
> -	rxp = 0; rxq = 0;
> +	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;
>   
> @@ -3134,6 +3139,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 414a006..7807afa 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"
>   
>   static void
>   usage(char* progname)
> @@ -603,6 +605,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 },
>   	};
>   
> @@ -1359,6 +1363,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 (strncmp(lgopts[opt_idx].name, PARAM_NUM_PROCS, 8) == 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 2b60f6c..7ab5f48 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -63,6 +63,8 @@
>   
>   #include "testpmd.h"
>   
> +int testpmd_fd_copy = 500; /* the copy of STDIN_FILENO */
> +
>   #ifndef MAP_HUGETLB
>   /* FreeBSD may not have MAP_HUGETLB (in fact, it probably doesn't) */
>   #define HUGE_FLAG (0x40000)
> @@ -79,6 +81,7 @@
>   
>   #define EXTMEM_HEAP_NAME "extmem"
>   #define EXTBUF_ZONE_SIZE RTE_PGSIZE_2M
> +#define MULTIPLE_PROCESS_HANDLE
>   
>   uint16_t verbose_level = 0; /**< Silent by default. */
>   int testpmd_logtype; /**< Log type for testpmd logs */
> @@ -125,6 +128,9 @@ uint8_t port_numa[RTE_MAX_ETHPORTS];
>    */
>   uint8_t rxring_numa[RTE_MAX_ETHPORTS];
>   
> +int proc_id = 0;
> +unsigned num_procs = 1;
> +
>   /*
>    * Store specified sockets on which TX ring to be used by ports
>    * is allocated.
> @@ -978,16 +984,36 @@ mbuf_pool_create(uint16_t mbuf_seg_size, unsigned nb_mbuf,
>   			/* wrapper to rte_mempool_create() */
>   			TESTPMD_LOG(INFO, "preferred mempool ops selected: %s\n",
>   					rte_mbuf_best_mempool_ops());
> +#ifdef MULTIPLE_PROCESS_HANDLE
> +			if (rte_eal_process_type() == RTE_PROC_PRIMARY)
> +				rte_mp = rte_pktmbuf_pool_create(pool_name,
> +					 nb_mbuf, mb_mempool_cache, 0,
> +					 mbuf_seg_size, socket_id);
> +			else
> +				rte_mp = rte_mempool_lookup(pool_name);
> +#else
>   			rte_mp = rte_pktmbuf_pool_create(pool_name, nb_mbuf,
>   				mb_mempool_cache, 0, mbuf_seg_size, socket_id);
> +#endif
>   			break;
>   		}
>   	case MP_ALLOC_ANON:
>   		{
> +#ifdef MULTIPLE_PROCESS_HANDLE
> +			if (rte_eal_process_type() == RTE_PROC_PRIMARY)
> +				rte_mp = rte_mempool_create_empty(pool_name,
> +					nb_mbuf, mb_size,
> +					(unsigned int)mb_mempool_cache,
> +					sizeof(struct rte_pktmbuf_pool_private),
> +					socket_id, 0);
> +			else
> +				rte_mp = rte_mempool_lookup(pool_name);
> +#else
>   			rte_mp = rte_mempool_create_empty(pool_name, nb_mbuf,
>   				mb_size, (unsigned int) mb_mempool_cache,
>   				sizeof(struct rte_pktmbuf_pool_private),
>   				socket_id, mempool_flags);
> +#endif
>   			if (rte_mp == NULL)
>   				goto err;
>   
> @@ -1017,9 +1043,18 @@ mbuf_pool_create(uint16_t mbuf_seg_size, unsigned nb_mbuf,
>   
>   			TESTPMD_LOG(INFO, "preferred mempool ops selected: %s\n",
>   					rte_mbuf_best_mempool_ops());
> +#ifdef MULTIPLE_PROCESS_HANDLE
> +			if (rte_eal_process_type() == RTE_PROC_PRIMARY)
> +				rte_mp = rte_pktmbuf_pool_create(pool_name,
> +					 nb_mbuf, mb_mempool_cache, 0,
> +					 mbuf_seg_size, heap_socket);
> +			else
> +				rte_mp = rte_mempool_lookup(pool_name);
> +#else
>   			rte_mp = rte_pktmbuf_pool_create(pool_name, nb_mbuf,
>   					mb_mempool_cache, 0, mbuf_seg_size,
>   					heap_socket);
> +#endif
>   			break;
>   		}
>   	case MP_ALLOC_XBUF:
> @@ -2485,21 +2520,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++) {
> @@ -2600,15 +2642,18 @@ start_port(portid_t pid)
>   		cnt_pi++;
>   
>   		/* start port */
> -		if (rte_eth_dev_start(pi) < 0) {
> -			printf("Fail to start port %d\n", pi);
> -
> -			/* 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;
> +		if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> +			diag = rte_eth_dev_start(pi);
> +			if (diag < 0) {
> +				printf("Fail to start port %d\n", pi);
> +
> +				/* 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;
> +			}
>   		}
>   
>   		if (rte_atomic16_cmpset(&(port->port_status),
> @@ -2737,7 +2782,7 @@ 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);
>   
> @@ -2806,8 +2851,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);
>   	}
>   
>   	remove_invalid_ports();
> @@ -3071,7 +3118,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]);
>   	}
>   
> @@ -3519,6 +3566,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));
> @@ -3617,13 +3668,6 @@ init_port(void)
>   }
>   
>   static void
> -force_quit(void)
> -{
> -	pmd_test_exit();
> -	prompt_exit();
> -}
> -
> -static void
>   print_stats(void)
>   {
>   	uint8_t i;
> @@ -3654,12 +3698,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);
> +		}
> +
>   	}
>   }
>   
> @@ -3684,10 +3732,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");
> @@ -3783,8 +3827,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) {
> @@ -3830,6 +3876,8 @@ main(int argc, char** argv)
>   		}
>   		prompt();
>   		pmd_test_exit();
> +		if (unlikely(f_quit == 1))
> +			prompt_exit();
>   	} else
>   #endif
>   	{
> @@ -3865,6 +3913,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();
> +		}
>   		if (rc < 0)
>   			return 1;
>   	}
> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> index 5f23162..8629c57 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
>   
> +uint8_t f_quit;
> +int testpmd_fd_copy;
> +struct cmdline *testpmd_cl;
> +
>   /*
>    * It is used to allocate the memory for hash key.
>    * The hash key size is NIC dependent.
> @@ -421,6 +426,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 num_procs;
>   
>   extern uint8_t dcb_config;
>   extern uint8_t dcb_test;
>