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

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

[Harry van Haaren]

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>

---

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 | 39 ++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index ecebb29..b6b569f 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,27 @@ 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 - ensure -c or -l includes service cores\n");
+	}
+
 	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 +408,10 @@ 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. Ensure -c is before -s or -S.\n");
+
 	if (coremask == NULL)
 		return -1;
 	/* Remove all blank characters ahead and after .
@@ -418,6 +443,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 +475,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 +517,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 +534,11 @@ 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 - ensure -c or -l includes service cores\n");
+	}
+
 	return 0;
 }
 
@@ -516,6 +551,10 @@ 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. Ensure -l is before -s or -S.\n");
+
 	if (corelist == NULL)
 		return -1;
 
-- 
2.7.4

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

[Thomas Monjalon]

15/05/2018 16:52, Harry van Haaren:
> +	if (eal_service_cores_parsed())
> +		RTE_LOG(ERR, EAL,
> +			"Warning: Service cores parsed before dataplane cores. Ensure -c is before -s or -S.\n");
> +
[...]
> +	if (core_parsed && taken_lcore_count != count) {
> +		RTE_LOG(ERR, EAL,
> +			"Warning: not all service cores were in the coremask - ensure -c or -l includes service cores\n");
> +	}

Sometimes, messages are split with a dot, sometimes with a dash.
Please choose only one for consistency (I prefer dot).
I think you can split the string and wrap the line at the end of the sentence.

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

[Harry van Haaren]

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");
+	}
+
 	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

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

[Varghese, Vipin]

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

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

[Thomas Monjalon]

Hi Harry,

What is the status of this patch?


21/05/2018 11:41, Varghese, Vipin:
> 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
> 

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

[Harry van Haaren]

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>

---

v3:
- Use WARNING instead of ERR log level (Vipin)
- Put strings on one line for grep-ability (Harry)


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, RC2 is also OK for this patch in my opinion
---
 lib/librte_eal/common/eal_common_options.c | 39 ++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 80f11d0..bee0f2d 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -321,6 +321,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;
@@ -364,6 +365,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++;
 			}
@@ -380,11 +385,27 @@ eal_parse_service_coremask(const char *coremask)
 	if (count == 0)
 		return -1;
 
+	if (core_parsed && taken_lcore_count != count) {
+		RTE_LOG(WARNING, EAL,
+			"Not all service cores are in the coremask. Please ensure -c or -l includes service cores\n");
+	}
+
 	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();
@@ -393,6 +414,10 @@ eal_parse_coremask(const char *coremask)
 	char c;
 	int val;
 
+	if (eal_service_cores_parsed())
+		RTE_LOG(WARNING, EAL,
+			"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 .
@@ -424,6 +449,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++;
@@ -455,6 +481,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;
@@ -496,6 +523,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++;
@@ -510,6 +540,11 @@ eal_parse_service_corelist(const char *corelist)
 	if (count == 0)
 		return -1;
 
+	if (core_parsed && taken_lcore_count != count) {
+		RTE_LOG(WARNING, EAL,
+			"not all service cores were in the coremask. Please ensure -c or -l includes service cores\n");
+	}
+
 	return 0;
 }
 
@@ -522,6 +557,10 @@ eal_parse_corelist(const char *corelist)
 	char *end = NULL;
 	int min, max;
 
+	if (eal_service_cores_parsed())
+		RTE_LOG(WARNING, EAL,
+			"Service cores parsed before dataplane cores. Please ensure -l is before -s or -S.\n");
+
 	if (corelist == NULL)
 		return -1;
 
-- 
2.7.4

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

[Van Haaren, Harry]

Hi Thomas,

I've sent a v3 for this patch. Twice actually, including dev@dpdk.org the 2nd time :)

Thanks for the ping, -Harry


> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday, July 12, 2018 8:35 AM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>
> Cc: dev@dpdk.org; Varghese, Vipin <vipin.varghese@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2] eal/service: improve error checking of
> coremasks
> 
> Hi Harry,
> 
> What is the status of this patch?
> 
> 
> 21/05/2018 11:41, Varghese, Vipin:
> > 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
> >
> 
> 
> 
> 

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

[Thomas Monjalon]

13/07/2018 19:25, Harry van Haaren:
> v3:
> - Use WARNING instead of ERR log level (Vipin)
> - Put strings on one line for grep-ability (Harry)

We can consider it is OK to have sentences on different lines.
Who is grepping 2 sentences at once?

> v2, thanks for review:
> - Consistency in message endings - vs . (Thomas)

Some dots are missing.

> - Wrap lines as they're very long otherwise (Thomas)

Lines are not wrapped.

> +		RTE_LOG(WARNING, EAL,
> +			"Service cores parsed before dataplane cores Please ensure -c is before -s or -S.\n");

I think this would be better:

		RTE_LOG(WARNING, EAL,
			"Service cores parsed before dataplane cores. "
			"Please ensure -c is before -s or -S.\n");

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

[Varghese, Vipin]

Thanks for the change in log level

Acked-by: Vipin Varghese <vipin.varghese@intel.com>

> -----Original Message-----
> From: Van Haaren, Harry
> Sent: Friday, July 13, 2018 10:56 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 v3] 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>
> 
> ---
> 
> v3:
> - Use WARNING instead of ERR log level (Vipin)
> - Put strings on one line for grep-ability (Harry)
> 
> 
> 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, RC2 is also OK for this patch in my opinion
> ---
>  lib/librte_eal/common/eal_common_options.c | 39
> ++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/lib/librte_eal/common/eal_common_options.c
> b/lib/librte_eal/common/eal_common_options.c
> index 80f11d0..bee0f2d 100644
> --- a/lib/librte_eal/common/eal_common_options.c
> +++ b/lib/librte_eal/common/eal_common_options.c
> @@ -321,6 +321,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;
> @@ -364,6 +365,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++;
>  			}
> @@ -380,11 +385,27 @@ eal_parse_service_coremask(const char
> *coremask)
>  	if (count == 0)
>  		return -1;
> 
> +	if (core_parsed && taken_lcore_count != count) {
> +		RTE_LOG(WARNING, EAL,
> +			"Not all service cores are in the coremask. Please
> ensure -c or -l includes service cores\n");
> +	}
> +
>  	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(); @@ -393,6
> +414,10 @@ eal_parse_coremask(const char *coremask)
>  	char c;
>  	int val;
> 
> +	if (eal_service_cores_parsed())
> +		RTE_LOG(WARNING, EAL,
> +			"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 .
> @@ -424,6 +449,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++;
> @@ -455,6 +481,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;
> @@ -496,6 +523,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++;
> @@ -510,6 +540,11 @@ eal_parse_service_corelist(const char *corelist)
>  	if (count == 0)
>  		return -1;
> 
> +	if (core_parsed && taken_lcore_count != count) {
> +		RTE_LOG(WARNING, EAL,
> +			"not all service cores were in the coremask. Please
> ensure -c or -l includes service cores\n");
> +	}
> +
>  	return 0;
>  }
> 
> @@ -522,6 +557,10 @@ eal_parse_corelist(const char *corelist)
>  	char *end = NULL;
>  	int min, max;
> 
> +	if (eal_service_cores_parsed())
> +		RTE_LOG(WARNING, EAL,
> +			"Service cores parsed before dataplane cores. Please
> ensure -l is
> +before -s or -S.\n");
> +
>  	if (corelist == NULL)
>  		return -1;
> 
> --
> 2.7.4

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

[Thomas Monjalon]

Hi Harry,

Are you OK to make a v4?


13/07/2018 19:33, Thomas Monjalon:
> 13/07/2018 19:25, Harry van Haaren:
> > v3:
> > - Use WARNING instead of ERR log level (Vipin)
> > - Put strings on one line for grep-ability (Harry)
> 
> We can consider it is OK to have sentences on different lines.
> Who is grepping 2 sentences at once?
> 
> > v2, thanks for review:
> > - Consistency in message endings - vs . (Thomas)
> 
> Some dots are missing.
> 
> > - Wrap lines as they're very long otherwise (Thomas)
> 
> Lines are not wrapped.
> 
> > +		RTE_LOG(WARNING, EAL,
> > +			"Service cores parsed before dataplane cores Please ensure -c is before -s or -S.\n");
> 
> I think this would be better:
> 
> 		RTE_LOG(WARNING, EAL,
> 			"Service cores parsed before dataplane cores. "
> 			"Please ensure -c is before -s or -S.\n");

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

[Harry van Haaren]

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>
Acked-by: Vipin Varghese <vipin.varghese@intel.com>

---

v4:
- Re-wrap lines breaking at sentence ends (Thomas)
- Consistency message endings with rest of file (Thomas)
- Including Vipin's Ack from v3

v3:
- Use WARNING instead of ERR log level (Vipin)
- Put strings on one line for grep-ability (Harry)

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
---
 lib/librte_eal/common/eal_common_options.c | 48 ++++++++++++++++++++++++++++--
 1 file changed, 46 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 80f11d0..8fcb6ef 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -321,6 +321,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;
@@ -354,7 +355,7 @@ eal_parse_service_coremask(const char *coremask)
 				if (master_lcore_parsed &&
 						cfg->master_lcore == lcore) {
 					RTE_LOG(ERR, EAL,
-						"Error: lcore %u is master lcore, cannot use as service core\n",
+						"lcore %u is master lcore, cannot use as service core\n",
 						idx);
 					return -1;
 				}
@@ -364,6 +365,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++;
 			}
@@ -380,11 +385,28 @@ eal_parse_service_coremask(const char *coremask)
 	if (count == 0)
 		return -1;
 
+	if (core_parsed && taken_lcore_count != count) {
+		RTE_LOG(WARNING, EAL,
+			"Not all service cores are in the coremask."
+			"Please ensure -c or -l includes service cores\n");
+	}
+
 	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();
@@ -393,6 +415,11 @@ eal_parse_coremask(const char *coremask)
 	char c;
 	int val;
 
+	if (eal_service_cores_parsed())
+		RTE_LOG(WARNING, EAL,
+			"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 .
@@ -424,6 +451,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++;
@@ -455,6 +483,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;
@@ -496,6 +525,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++;
@@ -510,6 +542,12 @@ eal_parse_service_corelist(const char *corelist)
 	if (count == 0)
 		return -1;
 
+	if (core_parsed && taken_lcore_count != count) {
+		RTE_LOG(WARNING, EAL,
+			"Not all service cores were in the coremask."
+			"Please ensure -c or -l includes service cores\n");
+	}
+
 	return 0;
 }
 
@@ -522,6 +560,11 @@ eal_parse_corelist(const char *corelist)
 	char *end = NULL;
 	int min, max;
 
+	if (eal_service_cores_parsed())
+		RTE_LOG(WARNING, EAL,
+			"Service cores parsed before dataplane cores."
+			"Please ensure -l is before -s or -S\n");
+
 	if (corelist == NULL)
 		return -1;
 
@@ -596,7 +639,8 @@ eal_parse_master_lcore(const char *arg)
 
 	/* ensure master core is not used as service core */
 	if (lcore_config[cfg->master_lcore].core_role == ROLE_SERVICE) {
-		RTE_LOG(ERR, EAL, "Error: Master lcore is used as a service core.\n");
+		RTE_LOG(ERR, EAL,
+			"Error: Master lcore is used as a service core\n");
 		return -1;
 	}
 
-- 
2.7.4

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

[Harry van Haaren]

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>
Acked-by: Vipin Varghese <vipin.varghese@intel.com>

---

v5:
- Add spaces after dot on first line (Thomas)

v4:
- Re-wrap lines breaking at sentence ends (Thomas)
- Consistency message endings with rest of file (Thomas)
- Including Vipin's Ack from v3

v3:
- Use WARNING instead of ERR log level (Vipin)
- Put strings on one line for grep-ability (Harry)

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
---
 lib/librte_eal/common/eal_common_options.c | 48 ++++++++++++++++++++++++++++--
 1 file changed, 46 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 80f11d0..dd5f974 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -321,6 +321,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;
@@ -354,7 +355,7 @@ eal_parse_service_coremask(const char *coremask)
 				if (master_lcore_parsed &&
 						cfg->master_lcore == lcore) {
 					RTE_LOG(ERR, EAL,
-						"Error: lcore %u is master lcore, cannot use as service core\n",
+						"lcore %u is master lcore, cannot use as service core\n",
 						idx);
 					return -1;
 				}
@@ -364,6 +365,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++;
 			}
@@ -380,11 +385,28 @@ eal_parse_service_coremask(const char *coremask)
 	if (count == 0)
 		return -1;
 
+	if (core_parsed && taken_lcore_count != count) {
+		RTE_LOG(WARNING, EAL,
+			"Not all service cores are in the coremask. "
+			"Please ensure -c or -l includes service cores\n");
+	}
+
 	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();
@@ -393,6 +415,11 @@ eal_parse_coremask(const char *coremask)
 	char c;
 	int val;
 
+	if (eal_service_cores_parsed())
+		RTE_LOG(WARNING, EAL,
+			"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 .
@@ -424,6 +451,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++;
@@ -455,6 +483,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;
@@ -496,6 +525,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++;
@@ -510,6 +542,12 @@ eal_parse_service_corelist(const char *corelist)
 	if (count == 0)
 		return -1;
 
+	if (core_parsed && taken_lcore_count != count) {
+		RTE_LOG(WARNING, EAL,
+			"Not all service cores were in the coremask. "
+			"Please ensure -c or -l includes service cores\n");
+	}
+
 	return 0;
 }
 
@@ -522,6 +560,11 @@ eal_parse_corelist(const char *corelist)
 	char *end = NULL;
 	int min, max;
 
+	if (eal_service_cores_parsed())
+		RTE_LOG(WARNING, EAL,
+			"Service cores parsed before dataplane cores. "
+			"Please ensure -l is before -s or -S\n");
+
 	if (corelist == NULL)
 		return -1;
 
@@ -596,7 +639,8 @@ eal_parse_master_lcore(const char *arg)
 
 	/* ensure master core is not used as service core */
 	if (lcore_config[cfg->master_lcore].core_role == ROLE_SERVICE) {
-		RTE_LOG(ERR, EAL, "Error: Master lcore is used as a service core.\n");
+		RTE_LOG(ERR, EAL,
+			"Error: Master lcore is used as a service core\n");
 		return -1;
 	}
 
-- 
2.7.4

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

[Thomas Monjalon]

26/07/2018 18:31, Harry van Haaren:
> 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>
> Acked-by: Vipin Varghese <vipin.varghese@intel.com>

Applied, thanks