Re: [dpdk-dev] [PATCH v4 2/9] ethdev: support representor port list

["Xueming(Steven) Li" <xuemingl@nvidia.com>]

Hi Andrew,

>-----Original Message-----
>From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>Sent: Tuesday, January 19, 2021 3:46 PM
>To: Xueming(Steven) Li <xuemingl@nvidia.com>; NBU-Contact-Thomas
>Monjalon <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>;
>Olivier Matz <olivier.matz@6wind.com>
>Cc: dev@dpdk.org; Slava Ovsiienko <viacheslavo@nvidia.com>; Asaf Penso
><asafp@nvidia.com>
>Subject: Re: [PATCH v4 2/9] ethdev: support representor port list
>
>On 1/18/21 2:16 PM, Xueming Li wrote:
>> To support extended representor syntax, this patch extends the
>> representor list parsing to support for representor port range in
>> devargs, examples:
>>    representor=[1,2,3]         - single list
>>    representor=[1,3-5,7,9-11]  - list with singles and ranges
>>
>> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
>> Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
>
>See below
>
>> ---
>>  lib/librte_ethdev/ethdev_private.c | 105 ++++++++++++++---------------
>>  lib/librte_ethdev/ethdev_private.h |   3 -
>>  lib/librte_ethdev/rte_class_eth.c  |   4 +-
>>  lib/librte_ethdev/rte_ethdev.c     |   5 +-
>>  4 files changed, 54 insertions(+), 63 deletions(-)
>>
>> diff --git a/lib/librte_ethdev/ethdev_private.c
>> b/lib/librte_ethdev/ethdev_private.c
>> index c1a411dba4..12bcc7e98d 100644
>> --- a/lib/librte_ethdev/ethdev_private.c
>> +++ b/lib/librte_ethdev/ethdev_private.c
>> @@ -38,77 +38,71 @@ eth_find_device(const struct rte_eth_dev *start,
>rte_eth_cmp_t cmp,
>>  	return NULL;
>>  }
>>
>> -int
>> -rte_eth_devargs_parse_list(char *str, rte_eth_devargs_callback_t callback,
>> -	void *data)
>> +static int
>> +rte_eth_devargs_enlist(uint16_t *list, uint16_t *len_list,
>> +		       const uint16_t max_list, uint16_t val)
>>  {
>> -	char *str_start;
>> -	int state;
>> -	int result;
>> -
>> -	if (*str != '[')
>> -		/* Single element, not a list */
>> -		return callback(str, data);
>> -
>> -	/* Sanity check, then strip the brackets */
>> -	str_start = &str[strlen(str) - 1];
>> -	if (*str_start != ']') {
>> -		RTE_LOG(ERR, EAL, "(%s): List does not end with ']'\n", str);
>> -		return -EINVAL;
>> -	}
>> -	str++;
>> -	*str_start = '\0';
>> +	uint16_t i;
>>
>> -	/* Process list elements */
>> -	state = 0;
>> -	while (1) {
>> -		if (state == 0) {
>> -			if (*str == '\0')
>> -				break;
>> -			if (*str != ',') {
>> -				str_start = str;
>> -				state = 1;
>> -			}
>> -		} else if (state == 1) {
>> -			if (*str == ',' || *str == '\0') {
>> -				if (str > str_start) {
>> -					/* Non-empty string fragment */
>> -					*str = '\0';
>> -					result = callback(str_start, data);
>> -					if (result < 0)
>> -						return result;
>> -				}
>> -				state = 0;
>> -			}
>> -		}
>> -		str++;
>> +	if (*len_list >= max_list)
>> +		return -1;
>
>If current length is equal to max, but added value is already is in the list, it
>should not be an error. So, these two lines should be moved after below for
>loop.
>
>> +	for (i = 0; i < *len_list; i++) {
>> +		if (list[i] == val)
>> +			return 0;
>>  	}
>> +	list[(*len_list)++] = val;
>>  	return 0;
>>  }
>>
>> -static int
>> +static char *
>>  rte_eth_devargs_process_range(char *str, uint16_t *list, uint16_t *len_list,
>>  	const uint16_t max_list)
>>  {
>>  	uint16_t lo, hi, val;
>>  	int result;
>> +	char *pos = str;
>>
>>  	result = sscanf(str, "%hu-%hu", &lo, &hi);
>>  	if (result == 1) {
>> -		if (*len_list >= max_list)
>> -			return -ENOMEM;
>> -		list[(*len_list)++] = lo;
>> +		if (rte_eth_devargs_enlist(list, len_list, max_list, lo) != 0)
>> +			return NULL;
>>  	} else if (result == 2) {
>> -		if (lo >= hi || lo > RTE_MAX_ETHPORTS || hi >
>RTE_MAX_ETHPORTS)
>
>Strictly speaking removal of comparision vs RTE_MAX_ETHPORTS is a separate
>logical change with a separate motivation.

To make this function comment for controller/pf/vf/sf parsing, the max value is
passed in as parameter and checked in rte_eth_devargs_enlist().
Caller code in rte_eth_devargs_process_list() decide the max value.
>
>> -			return -EINVAL;
>> +		if (lo >= hi)
>
>I'd remove '=' here. It should not be a problem and handed perfectly by below
>code. I see no point to deny 3-3 range which is an equivalent for just 3. It
>could be convenient in some cases.
>
>> +			return NULL;
>>  		for (val = lo; val <= hi; val++) {
>> -			if (*len_list >= max_list)
>> -				return -ENOMEM;
>> -			list[(*len_list)++] = val;
>> +			if (rte_eth_devargs_enlist(list, len_list, max_list,
>> +						   val) != 0)
>> +				return NULL;
>>  		}
>>  	} else
>> -		return -EINVAL;
>> -	return 0;
>> +		return NULL;
>> +	while (*pos != 0 && ((*pos >= '0' && *pos <= '9') || *pos == '-'))
>
>*post != '\0' is a bit better looking at subsequent comparisons. Yes, it is just
>style. Up to you.
>
>> +		pos++;
>
>It looks too fragile. May I suggest to use %n in above scanf to be able to skip
>only parsed characters.
>
>> +	return pos;
>> +}
>> +
>> +static char *
>> +rte_eth_devargs_process_list(char *str, uint16_t *list, uint16_t *len_list,
>> +	const uint16_t max_list)
>> +{
>> +	char *pos = str;
>> +
>> +	if (*pos == '[')
>> +		pos++;
>> +	while (1) {
>> +		pos = rte_eth_devargs_process_range(pos, list, len_list,
>> +						    max_list);
>> +		if (pos == NULL)
>> +			return NULL;
>> +		if (*pos != ',') /* end of list */
>> +			break;
>> +		pos++;
>> +	}
>> +	if (*str == '[' && *pos != ']')
>> +		return NULL;
>> +	if (*pos == ']')
>> +		pos++;
>> +	return pos;
>>  }
>>
>>  /*
>> @@ -121,6 +115,9 @@ rte_eth_devargs_parse_representor_ports(char *str,
>void *data)
>>  	struct rte_eth_devargs *eth_da = data;
>>
>>  	eth_da->type = RTE_ETH_REPRESENTOR_VF;
>> -	return rte_eth_devargs_process_range(str, eth_da-
>>representor_ports,
>> +	str = rte_eth_devargs_process_list(str, eth_da->representor_ports,
>>  		&eth_da->nb_representor_ports, RTE_MAX_ETHPORTS);
>
>Not directly related to the patch, but I dislike RTE_MAX_ETHPORTS above.
>RTE_DIM(eth_da->representor_ports) would be more readable.

The array dim could be different than RTE_MAX_ETHPORTS, although they are
same today 😊
>
>> +	if (str == NULL)
>> +		RTE_LOG(ERR, EAL, "wrong representor format: %s\n", str);
>> +	return str == NULL ? -1 : 0;
>>  }
>
>
>[snip]

Other comments looks great, will update, thanks!