Re: [dpdk-dev] [PATCH v5] app/testpmd: add portlist option to the testpmd

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

On 2/4/2020 4:48 PM, Hariprasad Govindharajan wrote:
>     In current version, we are setting the ports
>     using portmask. With portmask, we can use only
>     upto 64 ports. This portlist option enables the user
>     to use more than 64 ports.
>     Now we can specify the ports in 2 different ways
>     - Using portmask (-p [0x]nnn): mask must be in hex format
>     - Using portlist in the following format
>       --portlist <p1>[-p2][,p3[-p4],...]
> 
>     --portmask 0x2 is same as --portlist 1
>     --portmask 0x3 is same as --portlist 0-1
> 
> Signed-off-by: Hariprasad Govindharajan <hariprasad.govindharajan@intel.com>
> ---
> v5:
> added a check to validate the ports available before
> setting them. also added comments in the testpmd file
> for the new function
> 
> v4:
> the parser is modified so that we don't ues 2 arrays
> to convert the listed port values
> 
> v3:
> squashed the 2 patches and made it 1 patch with
> changes only in testpmd. Also working on optmizing
> the parser
> 
> v2:
> moved the parser function to testpmd
> ---
>  app/test-pmd/config.c                 | 107 +++++++++++++++++++++++++++++++++-
>  app/test-pmd/parameters.c             |   5 ++
>  app/test-pmd/testpmd.h                |   3 +
>  doc/guides/testpmd_app_ug/run_app.rst |   4 ++
>  4 files changed, 118 insertions(+), 1 deletion(-)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 9669cbd..6415022 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -2564,7 +2564,6 @@ set_fwd_ports_list(unsigned int *portlist, unsigned int nb_pt)
>  	unsigned int i;
>  	portid_t port_id;
>  	int record_now;
> -
>  	record_now = 0;
>   again:
>  	for (i = 0; i < nb_pt; i++) {
> @@ -2587,6 +2586,112 @@ set_fwd_ports_list(unsigned int *portlist, unsigned int nb_pt)
>  	}
>  }
>  
> +/**
> + * Parse and obtain the list of forwarding ports
> + * from the user input
> + *
> + * @param[in] list
> + *   String containing the user input. User can specify
> + *   in these formats 1,3,5 or 1-3 or 1-2,5 or 3,5-6.
> + *   For example, if the user wants to use all the available
> + *   4 ports in his system, then the input can be 0-3 or 0,1,2,3.
> + *   If the user wants to use only the ports 1,2 then the input
> + *   is 1,2.
> + *   valid characters are '-' and ','
> + *   invalid chars like '.' or '#' will result in
> + *   EAL: Error - exiting with code: 1
> + *     Cause: Invalid fwd port list
> + * @param[in] values

Isn't values and output parameter, filled by funciton.

> + *   An array pointer, used by this function to set the

Above line is redundant information

> + *   array contents to a positive value if they are listed
> + *   in the input else sets it to -1

Can you please give more detail how the content of this array will be after this
function. Like is the parsed port valued used as index in the array, is the '-1'
list terminator or can be anywhere. Is the "positive value" used that index has
a valid port or that value is the port number etc...


> + * @param[in] maxsize
> + *   This is the maximum value the list string can contain

This is also the size of the 'values' array right? At least code uses it that
way, can you document this expectation?

> + * @return
> + *   -On success, returns 0.
> + *   -On failure, returns -1.
> + */
> +static int
> +parse_port_list(const char *list, int *values, int maxsize)
> +{
> +	unsigned int count = 0;
> +	char *end = NULL;
> +	int min, max;
> +	int idx;
> +
> +	if (list == NULL || values == NULL || maxsize < 0)
> +		return -1;
> +
> +	for (idx = 0; idx < maxsize; idx++)
> +		values[idx] = -1;
> +	/* Remove all blank characters ahead */
> +	while (isblank(*list))
> +		list++;
> +
> +	min = maxsize;
> +
> +	do {
> +		while (isblank(*list))
> +			list++;
> +		if (*list == '\0')
> +			return -1;
> +		errno = 0;
> +		idx = strtol(list, &end, 10);
> +		if (errno || end == NULL)
> +			return -1;
> +		if (idx < 0 || idx >= maxsize)
> +			return -1;
> +		while (isblank(*end))
> +			end++;
> +		if (*end == '-') {
> +			min = idx;
> +		} else if ((*end == ',') || (*end == '\0')) {
> +			max = idx;
> +			if (min == maxsize)
> +				min = idx;
> +			for (idx = min; idx <= max; idx++) {
> +				if (values[count] == -1) {
> +					values[count] = idx;
> +					count++;

Isn't there any check for the 'count', if the user provides something like
"1-30,2-29" won't this overflow and trash the stack?

Also there is no check to prevent the duplication, is it allowed, like
"1,1,1,1,1,1,1,1,1,1" ?

> +				}
> +			}
> +			min = maxsize;
> +		} else
> +			return -1;
> +		list = end + 1;
> +	} while (*end != '\0');
> +
> +	if (count == 0)
> +		return -1;
> +	return 0;
> +}
> +
> +void
> +parse_fwd_portlist(const char *portlist)
> +{
> +	int portcount = 0;
> +	int portindex[RTE_MAX_ETHPORTS];
> +	unsigned int idx;
> +
> +	/*
> +	 * parse_port_list() will mark the portindex array
> +	 * with -1 if the port is not listed and with a positive value
> +	 * for the listed ports. So, the parser is designed in
> +	 * such a way that it will fill the portindex array with the
> +	 * valid ports from the user,and the function set_fwd_ports_list()
> +	 * will set those ports in the forwarding mode
> +	 */
> +
> +	if (parse_port_list(portlist, portindex, RTE_MAX_ETHPORTS) < 0)
> +		rte_exit(EXIT_FAILURE, "Invalid fwd port list\n");
> +
> +	RTE_ETH_FOREACH_DEV(idx) {
> +		if (rte_eth_dev_is_valid_port(portindex[idx]))
> +			portcount++;

This looks long, 'portcount' takes the valid ports in all list, but
'set_fwd_ports_list()' processes only first 'portcount' ports, so a value like
following will fail if there are only two valid ports (0,1):
"5-10,0,1"

> +	}
> +	set_fwd_ports_list((unsigned int *)portindex, portcount);
> +}
> +
>  void
>  set_fwd_ports_mask(uint64_t portmask)
>  {
> diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
> index 6340104..404dba2 100644
> --- a/app/test-pmd/parameters.c
> +++ b/app/test-pmd/parameters.c
> @@ -57,6 +57,7 @@ usage(char* progname)
>  	       "[--help|-h] | [--auto-start|-a] | ["
>  	       "--tx-first | --stats-period=PERIOD | "
>  	       "--coremask=COREMASK --portmask=PORTMASK --numa "
> +	       "--portlist=PORTLIST "
>  	       "--mbuf-size= | --total-num-mbufs= | "
>  	       "--nb-cores= | --nb-ports= | "
>  #ifdef RTE_LIBRTE_CMDLINE
> @@ -92,6 +93,7 @@ usage(char* progname)
>  	       "packet forwarding.\n");
>  	printf("  --portmask=PORTMASK: hexadecimal bitmask of ports used "
>  	       "by the packet forwarding test.\n");
> +	printf("  --portlist=PORTLIST: list of forwarding ports\n");
>  	printf("  --numa: enable NUMA-aware allocation of RX/TX rings and of "
>  	       "RX memory buffers (mbufs).\n");
>  	printf("  --port-numa-config=(port,socket)[,(port,socket)]: "
> @@ -587,6 +589,7 @@ launch_args_parse(int argc, char** argv)
>  		{ "nb-ports",			1, 0, 0 },
>  		{ "coremask",			1, 0, 0 },
>  		{ "portmask",			1, 0, 0 },
> +		{ "portlist",			1, 0, 0 },
>  		{ "numa",			0, 0, 0 },
>  		{ "no-numa",			0, 0, 0 },
>  		{ "mp-anon",			0, 0, 0 },
> @@ -825,6 +828,8 @@ launch_args_parse(int argc, char** argv)
>  				parse_fwd_coremask(optarg);
>  			if (!strcmp(lgopts[opt_idx].name, "portmask"))
>  				parse_fwd_portmask(optarg);
> +			if (!strcmp(lgopts[opt_idx].name, "portlist"))
> +				parse_fwd_portlist(optarg);
>  			if (!strcmp(lgopts[opt_idx].name, "no-numa"))
>  				numa_support = 0;
>  			if (!strcmp(lgopts[opt_idx].name, "numa"))
> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> index 3dd5fc7..33ef3e2 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -614,6 +614,9 @@ lcore_num(void)
>  	rte_panic("lcore_id of current thread not found in fwd_lcores_cpuids\n");
>  }
>  
> +void
> +parse_fwd_portlist(const char *port);
> +
>  static inline struct fwd_lcore *
>  current_fwd_lcore(void)
>  {
> diff --git a/doc/guides/testpmd_app_ug/run_app.rst b/doc/guides/testpmd_app_ug/run_app.rst
> index 9ab4d70..5e8d379 100644
> --- a/doc/guides/testpmd_app_ug/run_app.rst
> +++ b/doc/guides/testpmd_app_ug/run_app.rst
> @@ -77,6 +77,10 @@ The command line options are:
>  
>      Set the hexadecimal bitmask of the ports used by the packet forwarding test.
>  
> +*   ``--portlist=X``
> +
> +      Set the forwarding ports based on the user input used by the packet forwarding test.

Can you please document the expected syntax of the "X" briefly, like in a
sentences, here?

> +
>  *   ``--numa``
>  
>      Enable NUMA-aware allocation of RX/TX rings and of RX memory buffers
>