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

[Andrew Rybchenko]

On 1/19/21 11:59 AM, Xueming(Steven) Li wrote:
> Hi Andrew,
> 
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko at oktetlabs.ru>
>> Sent: Tuesday, January 19, 2021 3:46 PM
>> To: Xueming(Steven) Li <xuemingl at nvidia.com>; NBU-Contact-Thomas
>> Monjalon <thomas at monjalon.net>; Ferruh Yigit <ferruh.yigit at intel.com>;
>> Olivier Matz <olivier.matz at 6wind.com>
>> Cc: dev at dpdk.org; Slava Ovsiienko <viacheslavo at nvidia.com>; Asaf Penso
>> <asafp at 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 at nvidia.com>
>>> Acked-by: Viacheslav Ovsiienko <viacheslavo at 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.

Which maximum? Maximum number of elements in array and
maximum element value are different things.

[snip]