diff mbox series

[v4,2/9] ethdev: support representor port list

Message ID 1610968623-31345-3-git-send-email-xuemingl@nvidia.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers show
Series ethdev: support SubFunction representor | expand

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Xueming(Steven) Li Jan. 18, 2021, 11:16 a.m. UTC 
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>
---
 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(-)

Comments

Thomas Monjalon Jan. 18, 2021, 4:18 p.m. UTC | #1 
18/01/2021 12:16, Xueming Li:
> 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>

Parsing functions are usually difficult to read.
If you need to do another version, I would recommend adding
an oneline comment on top of parsing functions to give a hint
about what it is really doing.

Here,

> +static int
> +rte_eth_devargs_enlist(uint16_t *list, uint16_t *len_list,
> +		       const uint16_t max_list, uint16_t val)

here,

> +static char *
>  rte_eth_devargs_process_range(char *str, uint16_t *list, uint16_t *len_list,
>  	const uint16_t max_list)

here,

> +static char *
> +rte_eth_devargs_process_list(char *str, uint16_t *list, uint16_t *len_list,
> +	const uint16_t max_list)

and also for this old one:

> @@ -121,6 +115,9 @@ rte_eth_devargs_parse_representor_ports(char *str, void *data)
Xueming(Steven) Li Jan. 18, 2021, 11:23 p.m. UTC | #2 
Hi Thomas,

>-----Original Message-----
>From: Thomas Monjalon <thomas@monjalon.net>
>Sent: Tuesday, January 19, 2021 12:18 AM
>To: Xueming(Steven) Li <xuemingl@nvidia.com>
>Cc: Ferruh Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko
><andrew.rybchenko@oktetlabs.ru>; Olivier Matz <olivier.matz@6wind.com>;
>dev@dpdk.org; Slava Ovsiienko <viacheslavo@nvidia.com>; Asaf Penso
><asafp@nvidia.com>
>Subject: Re: [dpdk-dev] [PATCH v4 2/9] ethdev: support representor port list
>
>18/01/2021 12:16, Xueming Li:
>> 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>
>
>Parsing functions are usually difficult to read.
>If you need to do another version, I would recommend adding an oneline
>comment on top of parsing functions to give a hint about what it is really
>doing.
>
>Here,
>
>> +static int
>> +rte_eth_devargs_enlist(uint16_t *list, uint16_t *len_list,
>> +		       const uint16_t max_list, uint16_t val)
>
>here,
>
>> +static char *
>>  rte_eth_devargs_process_range(char *str, uint16_t *list, uint16_t *len_list,
>>  	const uint16_t max_list)
>
>here,
>
>> +static char *
>> +rte_eth_devargs_process_list(char *str, uint16_t *list, uint16_t *len_list,
>> +	const uint16_t max_list)
>
>and also for this old one:
>
>> @@ -121,6 +115,9 @@ rte_eth_devargs_parse_representor_ports(char *str,
>> void *data)
>
>

Will add them in next version, thanks.
Andrew Rybchenko Jan. 19, 2021, 7:45 a.m. UTC | #3 
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.

> -			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.

> +	if (str == NULL)
> +		RTE_LOG(ERR, EAL, "wrong representor format: %s\n", str);
> +	return str == NULL ? -1 : 0;
>  }


[snip]
Xueming(Steven) Li Jan. 19, 2021, 8:59 a.m. UTC | #4 
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!
Andrew Rybchenko Jan. 19, 2021, 9:03 a.m. UTC | #5 
On 1/19/21 11:59 AM, Xueming(Steven) Li wrote:
> 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.

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

[snip]
Xueming(Steven) Li Jan. 19, 2021, 10:19 a.m. UTC | #6 
>-----Original Message-----
>From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>Sent: Tuesday, January 19, 2021 5:04 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/19/21 11:59 AM, Xueming(Steven) Li wrote:
>> 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.
>
>Which maximum? Maximum number of elements in array and maximum
>element value are different things.

My bad, after another check, it should be max number of elements in array,
not max element value. Will update, thanks! 
>
>[snip]
diff mbox series

Patch

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;
+	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)
-			return -EINVAL;
+		if (lo >= hi)
+			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 == '-'))
+		pos++;
+	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);
+	if (str == NULL)
+		RTE_LOG(ERR, EAL, "wrong representor format: %s\n", str);
+	return str == NULL ? -1 : 0;
 }
diff --git a/lib/librte_ethdev/ethdev_private.h b/lib/librte_ethdev/ethdev_private.h
index 905a45c337..220ddd4408 100644
--- a/lib/librte_ethdev/ethdev_private.h
+++ b/lib/librte_ethdev/ethdev_private.h
@@ -26,9 +26,6 @@  eth_find_device(const struct rte_eth_dev *_start, rte_eth_cmp_t cmp,
 		const void *data);
 
 /* Parse devargs value for representor parameter. */
-typedef int (*rte_eth_devargs_callback_t)(char *str, void *data);
-int rte_eth_devargs_parse_list(char *str, rte_eth_devargs_callback_t callback,
-	void *data);
 int rte_eth_devargs_parse_representor_ports(char *str, void *data);
 
 #ifdef __cplusplus
diff --git a/lib/librte_ethdev/rte_class_eth.c b/lib/librte_ethdev/rte_class_eth.c
index 6338355e25..efe6149df5 100644
--- a/lib/librte_ethdev/rte_class_eth.c
+++ b/lib/librte_ethdev/rte_class_eth.c
@@ -77,9 +77,7 @@  eth_representor_cmp(const char *key __rte_unused,
 	if (values == NULL)
 		return -1;
 	memset(&representors, 0, sizeof(representors));
-	ret = rte_eth_devargs_parse_list(values,
-			rte_eth_devargs_parse_representor_ports,
-			&representors);
+	ret = rte_eth_devargs_parse_representor_ports(values, &representors);
 	free(values);
 	if (ret != 0)
 		return -1; /* invalid devargs value */
diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 17ddacc78d..2ac51ac149 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -5542,9 +5542,8 @@  rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_da)
 	for (i = 0; i < args.count; i++) {
 		pair = &args.pairs[i];
 		if (strcmp("representor", pair->key) == 0) {
-			result = rte_eth_devargs_parse_list(pair->value,
-				rte_eth_devargs_parse_representor_ports,
-				eth_da);
+			result = rte_eth_devargs_parse_representor_ports(
+					pair->value, eth_da);
 			if (result < 0)
 				goto parse_cleanup;
 		}