Re: [dpdk-dev] [PATCH v2] eal/service: improve error checking of coremasks

["Varghese, Vipin" <vipin.varghese@intel.com>]

Hi Harry,

This look ok to me, except for one warning rewrite else its ACK from my end.

> -----Original Message-----
> From: Van Haaren, Harry
> Sent: Tuesday, May 15, 2018 9:26 PM
> To: dev@dpdk.org
> Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; thomas@monjalon.net;
> Varghese, Vipin <vipin.varghese@intel.com>
> Subject: [PATCH v2] eal/service: improve error checking of coremasks
> 
> This commit improves the error checking performed on the core masks (or lists)
> of the service cores, in particular with respect to the data-plane (RTE) cores of
> DPDK.
> 
> With this commit, invalid configurations are detected at runtime, and warning
> messages are printed to inform the user.
> 
> For example specifying the coremask as 0xf, and the service coremask as 0xff00
> is invalid as not all service-cores are contained within the coremask. A warning is
> now printed to inform the user.
> 
> Reported-by: Vipin Varghese <vipin.varghese@intel.com>
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> 
> ---
> 
> v2, thanks for review:
> - Consistency in message endings - vs . (Thomas)
> - Wrap lines as they're very long otherwise (Thomas)
> 
> Cc: thomas@monjalon.net
> Cc: vipin.varghese@intel.com
> 
> @Thomas, please consider this patch for RC4, it adds checks and prints
> warnings, better usability, no functional changes.
> ---
>  lib/librte_eal/common/eal_common_options.c | 43
> ++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/lib/librte_eal/common/eal_common_options.c
> b/lib/librte_eal/common/eal_common_options.c
> index ecebb29..9f3a484 100644
> --- a/lib/librte_eal/common/eal_common_options.c
> +++ b/lib/librte_eal/common/eal_common_options.c
> @@ -315,6 +315,7 @@ eal_parse_service_coremask(const char *coremask)
>  	unsigned int count = 0;
>  	char c;
>  	int val;
> +	uint32_t taken_lcore_count = 0;
> 
>  	if (coremask == NULL)
>  		return -1;
> @@ -358,6 +359,10 @@ eal_parse_service_coremask(const char *coremask)
>  						"lcore %u unavailable\n", idx);
>  					return -1;
>  				}
> +
> +				if (cfg->lcore_role[idx] == ROLE_RTE)
> +					taken_lcore_count++;
> +
>  				lcore_config[idx].core_role = ROLE_SERVICE;
>  				count++;
>  			}
> @@ -374,11 +379,28 @@ eal_parse_service_coremask(const char *coremask)
>  	if (count == 0)
>  		return -1;
> 
> +	if (core_parsed && taken_lcore_count != count) {
> +		RTE_LOG(ERR, EAL,
> +			"Warning: not all service cores were in the coremask. "
> +			"Please ensure -c or -l includes service cores\n");

Current execution will throw warning message as 'Warning: not all service cores were in the coremask. Please ensure -c or -l includes service cores'. 

1) Should we re-write this with ' RTE_LOG(WARN, EAL,' and removing 'Warning: '
2) Warning message as "service cores not in data plane core mask ".
3) If we share information "Please ensure -c or -l includes service cores\n" is not it expected to rte_panic? So should we remove this line?

> +	}
> +
>  	cfg->service_lcore_count = count;
>  	return 0;
>  }
> 
>  static int
> +eal_service_cores_parsed(void)
> +{
> +	int idx;
> +	for (idx = 0; idx < RTE_MAX_LCORE; idx++) {
> +		if (lcore_config[idx].core_role == ROLE_SERVICE)
> +			return 1;
> +	}
> +	return 0;
> +}
> +
> +static int
>  eal_parse_coremask(const char *coremask)  {
>  	struct rte_config *cfg = rte_eal_get_configuration(); @@ -387,6
> +409,11 @@ eal_parse_coremask(const char *coremask)
>  	char c;
>  	int val;
> 
> +	if (eal_service_cores_parsed())
> +		RTE_LOG(ERR, EAL,
> +			"Warning: Service cores parsed before dataplane cores.
> "
> +			"Please ensure -c is before -s or -S.\n");
> +
>  	if (coremask == NULL)
>  		return -1;
>  	/* Remove all blank characters ahead and after .
> @@ -418,6 +445,7 @@ eal_parse_coremask(const char *coremask)
>  					        "unavailable\n", idx);
>  					return -1;
>  				}
> +
>  				cfg->lcore_role[idx] = ROLE_RTE;
>  				lcore_config[idx].core_index = count;
>  				count++;
> @@ -449,6 +477,7 @@ eal_parse_service_corelist(const char *corelist)
>  	unsigned count = 0;
>  	char *end = NULL;
>  	int min, max;
> +	uint32_t taken_lcore_count = 0;
> 
>  	if (corelist == NULL)
>  		return -1;
> @@ -490,6 +519,9 @@ eal_parse_service_corelist(const char *corelist)
>  							idx);
>  						return -1;
>  					}
> +					if (cfg->lcore_role[idx] == ROLE_RTE)
> +						taken_lcore_count++;
> +
>  					lcore_config[idx].core_role =
>  							ROLE_SERVICE;
>  					count++;
> @@ -504,6 +536,12 @@ eal_parse_service_corelist(const char *corelist)
>  	if (count == 0)
>  		return -1;
> 
> +	if (core_parsed && taken_lcore_count != count) {
> +		RTE_LOG(ERR, EAL,
> +			"Warning: not all service cores were in the coremask. "
> +			"Please ensure -c or -l includes service cores\n");
> +	}
> +
>  	return 0;
>  }
> 
> @@ -516,6 +554,11 @@ eal_parse_corelist(const char *corelist)
>  	char *end = NULL;
>  	int min, max;
> 
> +	if (eal_service_cores_parsed())
> +		RTE_LOG(ERR, EAL,
> +			"Warning: Service cores parsed before dataplane cores.
> "
> +			"Please ensure -l is before -s or -S.\n");
> +
>  	if (corelist == NULL)
>  		return -1;
> 
> --
> 2.7.4