[dpdk-dev] [PATCH v3] examples/qos_sched: fix bad bit shift operation

[dpdk-dev] [PATCH v3] examples/qos_sched: fix bad bit shift operation

[Slawomir Mrozowicz]

Fix issue reported by Coverity.

Coverity ID 30690: Bad bit shift operation
large_shift: In expression 1ULL << i, left shifting by more than 63 bits
has undefined behavior. The shift amount, i, is as much as 127.

Fixes: de3cfa2c9823 ("sched: initial import")

Signed-off-by: Slawomir Mrozowicz <slawomirx.mrozowicz@intel.com>
---
 examples/qos_sched/args.c | 4 ++--
 examples/qos_sched/main.h | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/examples/qos_sched/args.c b/examples/qos_sched/args.c
index 3e7fd08..354372d 100644
--- a/examples/qos_sched/args.c
+++ b/examples/qos_sched/args.c
@@ -123,7 +123,7 @@ app_eal_core_mask(void)
 	uint64_t cm = 0;
 	struct rte_config *cfg = rte_eal_get_configuration();
 
-	for (i = 0; i < RTE_MAX_LCORE; i++) {
+	for (i = 0; i < APP_MAX_LCORE; i++) {
 		if (cfg->lcore_role[i] == ROLE_RTE)
 			cm |= (1ULL << i);
 	}
@@ -142,7 +142,7 @@ app_cpu_core_count(void)
 	char path[PATH_MAX];
 	uint32_t ncores = 0;
 
-	for(i = 0; i < RTE_MAX_LCORE; i++) {
+	for (i = 0; i < APP_MAX_LCORE; i++) {
 		len = snprintf(path, sizeof(path), SYS_CPU_DIR, i);
 		if (len <= 0 || (unsigned)len >= sizeof(path))
 			continue;
diff --git a/examples/qos_sched/main.h b/examples/qos_sched/main.h
index 82aa0fa..e0517d1 100644
--- a/examples/qos_sched/main.h
+++ b/examples/qos_sched/main.h
@@ -68,7 +68,8 @@ extern "C" {
 
 #define BURST_TX_DRAIN_US 100
 
-#define MAX_DATA_STREAMS (RTE_MAX_LCORE/2)
+#define APP_MAX_LCORE 64
+#define MAX_DATA_STREAMS (APP_MAX_LCORE/2)
 #define MAX_SCHED_SUBPORTS		8
 #define MAX_SCHED_PIPES		4096
 
-- 
1.9.1

Re: [dpdk-dev] [PATCH v3] examples/qos_sched: fix bad bit shift operation

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Mrozowicz, SlawomirX
> Sent: Tuesday, May 10, 2016 1:21 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: dev@dpdk.org; Singh, Jasvinder <jasvinder.singh@intel.com>;
> Mrozowicz, SlawomirX <slawomirx.mrozowicz@intel.com>
> Subject: [PATCH v3] examples/qos_sched: fix bad bit shift operation
> 
> Fix issue reported by Coverity.
> 
> Coverity ID 30690: Bad bit shift operation
> large_shift: In expression 1ULL << i, left shifting by more than 63 bits
> has undefined behavior. The shift amount, i, is as much as 127.
> 
> Fixes: de3cfa2c9823 ("sched: initial import")
> 
> Signed-off-by: Slawomir Mrozowicz <slawomirx.mrozowicz@intel.com>
> ---
>  examples/qos_sched/args.c | 4 ++--
>  examples/qos_sched/main.h | 3 ++-
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/examples/qos_sched/args.c b/examples/qos_sched/args.c
> index 3e7fd08..354372d 100644
> --- a/examples/qos_sched/args.c
> +++ b/examples/qos_sched/args.c
> @@ -123,7 +123,7 @@ app_eal_core_mask(void)
>  	uint64_t cm = 0;
>  	struct rte_config *cfg = rte_eal_get_configuration();
> 
> -	for (i = 0; i < RTE_MAX_LCORE; i++) {
> +	for (i = 0; i < APP_MAX_LCORE; i++) {
>  		if (cfg->lcore_role[i] == ROLE_RTE)
>  			cm |= (1ULL << i);
>  	}
> @@ -142,7 +142,7 @@ app_cpu_core_count(void)
>  	char path[PATH_MAX];
>  	uint32_t ncores = 0;
> 
> -	for(i = 0; i < RTE_MAX_LCORE; i++) {
> +	for (i = 0; i < APP_MAX_LCORE; i++) {
>  		len = snprintf(path, sizeof(path), SYS_CPU_DIR, i);
>  		if (len <= 0 || (unsigned)len >= sizeof(path))
>  			continue;
> diff --git a/examples/qos_sched/main.h b/examples/qos_sched/main.h
> index 82aa0fa..e0517d1 100644
> --- a/examples/qos_sched/main.h
> +++ b/examples/qos_sched/main.h
> @@ -68,7 +68,8 @@ extern "C" {
> 
>  #define BURST_TX_DRAIN_US 100
> 
> -#define MAX_DATA_STREAMS (RTE_MAX_LCORE/2)
> +#define APP_MAX_LCORE 64

Please allow this parameter to be configured from Makefile:

#ifndef APP_MAX_LCORE
#define APP_MAX_LCORE 64
#endif

> +#define MAX_DATA_STREAMS (APP_MAX_LCORE/2)
>  #define MAX_SCHED_SUBPORTS		8
>  #define MAX_SCHED_PIPES		4096
> 
> --
> 1.9.1

Yep, looks good. Thanks, Slawomir!

Regards,
Cristian

[dpdk-dev] [PATCH v3] examples/qos_sched: fix negative loop bound

[Michal Jastrzebski]

From: Slawomir Mrozowicz <slawomirx.mrozowicz@intel.com>

Fix issue reported by Coverity.

Coverity ID 30704: Negative loop bound
negative_returns: Using unsigned variable n_tokens in a loop exit condition.

Fixes: de3cfa2c9823 ("sched: initial import")

Signed-off-by: Slawomir Mrozowicz <slawomirx.mrozowicz@intel.com>
---
 examples/qos_sched/args.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/examples/qos_sched/args.c b/examples/qos_sched/args.c
index 3e7fd08..7a98e5c 100644
--- a/examples/qos_sched/args.c
+++ b/examples/qos_sched/args.c
@@ -162,7 +162,7 @@ static int
 app_parse_opt_vals(const char *conf_str, char separator, uint32_t n_vals, uint32_t *opt_vals)
 {
 	char *string;
-	uint32_t i, n_tokens;
+	int i, n_tokens;
 	char *tokens[MAX_OPT_VALUES];
 
 	if (conf_str == NULL || opt_vals == NULL || n_vals == 0 || n_vals > MAX_OPT_VALUES)
-- 
1.9.1

[dpdk-dev] [PATCH v3] examples/qos_sched: fix bad bit shift operation

[Michal Jastrzebski]

From: Slawomir Mrozowicz <slawomirx.mrozowicz@intel.com>

Fix issue reported by Coverity.

Coverity ID 30690: Bad bit shift operation
large_shift: In expression 1ULL << i, left shifting by more than 63 bits
has undefined behavior. The shift amount, i, is as much as 127.

Fixes: de3cfa2c9823 ("sched: initial import")

Signed-off-by: Slawomir Mrozowicz <slawomirx.mrozowicz@intel.com>
---
 examples/qos_sched/args.c | 84 +++++++++++++++++++++++++++++------------------
 1 file changed, 52 insertions(+), 32 deletions(-)

diff --git a/examples/qos_sched/args.c b/examples/qos_sched/args.c
index 3e7fd08..cd077ba 100644
--- a/examples/qos_sched/args.c
+++ b/examples/qos_sched/args.c
@@ -53,7 +53,7 @@
 
 static uint32_t app_master_core = 1;
 static uint32_t app_numa_mask;
-static uint64_t app_used_core_mask = 0;
+static int app_used_core_mask[RTE_MAX_LCORE];
 static uint64_t app_used_port_mask = 0;
 static uint64_t app_used_rx_port_mask = 0;
 static uint64_t app_used_tx_port_mask = 0;
@@ -115,22 +115,23 @@ static inline int str_is(const char *str, const char *is)
 	return strcmp(str, is) == 0;
 }
 
-/* returns core mask used by DPDK */
-static uint64_t
-app_eal_core_mask(void)
+/* compare used core with eal configuration,
+	returns:
+		1 if equal
+		0 if differ */
+static int
+app_eal_core_check(void)
 {
-	uint32_t i;
-	uint64_t cm = 0;
+	uint16_t i;
+	int ret = 1;
 	struct rte_config *cfg = rte_eal_get_configuration();
 
-	for (i = 0; i < RTE_MAX_LCORE; i++) {
-		if (cfg->lcore_role[i] == ROLE_RTE)
-			cm |= (1ULL << i);
+	for (i = 0; i < RTE_MAX_LCORE && ret; i++) {
+		if ((cfg->lcore_role[i] == ROLE_RTE) != app_used_core_mask[i])
+			ret = 0;
 	}
 
-	cm |= (1ULL << cfg->master_lcore);
-
-	return cm;
+	return ret;
 }
 
 
@@ -292,14 +293,9 @@ app_parse_flow_conf(const char *conf_str)
 	app_used_tx_port_mask |= mask;
 	app_used_port_mask |= mask;
 
-	mask = 1lu << pconf->rx_core;
-	app_used_core_mask |= mask;
-
-	mask = 1lu << pconf->wt_core;
-	app_used_core_mask |= mask;
-
-	mask = 1lu << pconf->tx_core;
-	app_used_core_mask |= mask;
+	app_used_core_mask[pconf->rx_core] = 1;
+	app_used_core_mask[pconf->wt_core] = 1;
+	app_used_core_mask[pconf->tx_core] = 1;
 
 	nb_pfc++;
 
@@ -335,7 +331,7 @@ app_parse_args(int argc, char **argv)
 	int option_index;
 	const char *optname;
 	char *prgname = argv[0];
-	uint32_t i, nb_lcores;
+	uint16_t i, j, k, nb_lcores;
 
 	static struct option lgopts[] = {
 		{ "pfc", 1, 0, 0 },
@@ -349,6 +345,9 @@ app_parse_args(int argc, char **argv)
 		{ NULL,  0, 0, 0 }
 	};
 
+	for (i = 0; i < RTE_MAX_LCORE; i++)
+		app_used_core_mask[i] = 0;
+
 	/* initialize EAL first */
 	ret = rte_eal_init(argc, argv);
 	if (ret < 0)
@@ -436,19 +435,40 @@ app_parse_args(int argc, char **argv)
 	}
 
 	/* check master core index validity */
-	for(i = 0; i <= app_master_core; i++) {
-		if (app_used_core_mask & (1u << app_master_core)) {
-			RTE_LOG(ERR, APP, "Master core index is not configured properly\n");
-			app_usage(prgname);
-			return -1;
-		}
+	if (app_used_core_mask[app_master_core] == 1) {
+		RTE_LOG(ERR, APP,
+			"Master core index is not configured properly\n");
+		app_usage(prgname);
+		return -1;
 	}
-	app_used_core_mask |= 1u << app_master_core;
+	app_used_core_mask[app_master_core] = 1;
+
+	if ((app_eal_core_check() == 0) ||
+		(app_master_core != rte_get_master_lcore())) {
+
+		char used_hexstr[RTE_MAX_LCORE/4+1];
+		char conf_hexstr[RTE_MAX_LCORE/4+1];
+		int used_byte, conf_byte;
+		struct rte_config *cfg = rte_eal_get_configuration();
+
+		for (i = 0; i < RTE_MAX_LCORE/4; i++) {
+			used_byte = 0;
+			conf_byte = 0;
+			for (j = 0; j < 3; j++) {
+				k = 4 * (RTE_MAX_LCORE/4 - i - 1) + j;
+				used_byte += app_used_core_mask[k] << j;
+				conf_byte +=
+					((cfg->lcore_role[k] ==
+					ROLE_RTE)?1:0) << j;
+			}
+			sprintf(&used_hexstr[i], "%1x", used_byte);
+			sprintf(&conf_hexstr[i], "%1x", used_byte);
+		}
+
+		RTE_LOG(ERR, APP, "EAL core mask not configured properly\n");
+		RTE_LOG(ERR, APP, "  must be   : %s\n", used_hexstr);
+		RTE_LOG(ERR, APP, "  instead of: %s\n", conf_hexstr);
 
-	if ((app_used_core_mask != app_eal_core_mask()) ||
-			(app_master_core != rte_get_master_lcore())) {
-		RTE_LOG(ERR, APP, "EAL core mask not configured properly, must be %" PRIx64
-				" instead of %" PRIx64 "\n" , app_used_core_mask, app_eal_core_mask());
 		return -1;
 	}
 
-- 
1.9.1

Re: [dpdk-dev] [PATCH v3] examples/qos_sched: fix bad bit shift operation

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Jastrzebski, MichalX K
> Sent: Thursday, April 21, 2016 2:08 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Zhang, Roy Fan
> <roy.fan.zhang@intel.com>; Singh, Jasvinder <jasvinder.singh@intel.com>
> Cc: dev@dpdk.org; Mrozowicz, SlawomirX
> <slawomirx.mrozowicz@intel.com>
> Subject: [PATCH v3] examples/qos_sched: fix bad bit shift operation
> 
> From: Slawomir Mrozowicz <slawomirx.mrozowicz@intel.com>
> 
> Fix issue reported by Coverity.
> 
> Coverity ID 30690: Bad bit shift operation
> large_shift: In expression 1ULL << i, left shifting by more than 63 bits
> has undefined behavior. The shift amount, i, is as much as 127.
> 
> Fixes: de3cfa2c9823 ("sched: initial import")
> 
> Signed-off-by: Slawomir Mrozowicz <slawomirx.mrozowicz@intel.com>
> ---
>  examples/qos_sched/args.c | 84 +++++++++++++++++++++++++++++------
> ------------
>  1 file changed, 52 insertions(+), 32 deletions(-)
> 
> diff --git a/examples/qos_sched/args.c b/examples/qos_sched/args.c
> index 3e7fd08..cd077ba 100644
> --- a/examples/qos_sched/args.c
> +++ b/examples/qos_sched/args.c
> @@ -53,7 +53,7 @@
> 
>  static uint32_t app_master_core = 1;
>  static uint32_t app_numa_mask;
> -static uint64_t app_used_core_mask = 0;
> +static int app_used_core_mask[RTE_MAX_LCORE];
>  static uint64_t app_used_port_mask = 0;
>  static uint64_t app_used_rx_port_mask = 0;
>  static uint64_t app_used_tx_port_mask = 0;
> @@ -115,22 +115,23 @@ static inline int str_is(const char *str, const char *is)
>  	return strcmp(str, is) == 0;
>  }
> 
> -/* returns core mask used by DPDK */
> -static uint64_t
> -app_eal_core_mask(void)
> +/* compare used core with eal configuration,
> +	returns:
> +		1 if equal
> +		0 if differ */
> +static int
> +app_eal_core_check(void)
>  {
> -	uint32_t i;
> -	uint64_t cm = 0;
> +	uint16_t i;
> +	int ret = 1;
>  	struct rte_config *cfg = rte_eal_get_configuration();
> 
> -	for (i = 0; i < RTE_MAX_LCORE; i++) {
> -		if (cfg->lcore_role[i] == ROLE_RTE)
> -			cm |= (1ULL << i);
> +	for (i = 0; i < RTE_MAX_LCORE && ret; i++) {
> +		if ((cfg->lcore_role[i] == ROLE_RTE) !=
> app_used_core_mask[i])
> +			ret = 0;
>  	}
> 
> -	cm |= (1ULL << cfg->master_lcore);
> -
> -	return cm;
> +	return ret;
>  }
> 
> 
> @@ -292,14 +293,9 @@ app_parse_flow_conf(const char *conf_str)
>  	app_used_tx_port_mask |= mask;
>  	app_used_port_mask |= mask;
> 
> -	mask = 1lu << pconf->rx_core;
> -	app_used_core_mask |= mask;
> -
> -	mask = 1lu << pconf->wt_core;
> -	app_used_core_mask |= mask;
> -
> -	mask = 1lu << pconf->tx_core;
> -	app_used_core_mask |= mask;
> +	app_used_core_mask[pconf->rx_core] = 1;
> +	app_used_core_mask[pconf->wt_core] = 1;
> +	app_used_core_mask[pconf->tx_core] = 1;
> 
>  	nb_pfc++;
> 
> @@ -335,7 +331,7 @@ app_parse_args(int argc, char **argv)
>  	int option_index;
>  	const char *optname;
>  	char *prgname = argv[0];
> -	uint32_t i, nb_lcores;
> +	uint16_t i, j, k, nb_lcores;
> 
>  	static struct option lgopts[] = {
>  		{ "pfc", 1, 0, 0 },
> @@ -349,6 +345,9 @@ app_parse_args(int argc, char **argv)
>  		{ NULL,  0, 0, 0 }
>  	};
> 
> +	for (i = 0; i < RTE_MAX_LCORE; i++)
> +		app_used_core_mask[i] = 0;
> +
>  	/* initialize EAL first */
>  	ret = rte_eal_init(argc, argv);
>  	if (ret < 0)
> @@ -436,19 +435,40 @@ app_parse_args(int argc, char **argv)
>  	}
> 
>  	/* check master core index validity */
> -	for(i = 0; i <= app_master_core; i++) {
> -		if (app_used_core_mask & (1u << app_master_core)) {
> -			RTE_LOG(ERR, APP, "Master core index is not
> configured properly\n");
> -			app_usage(prgname);
> -			return -1;
> -		}
> +	if (app_used_core_mask[app_master_core] == 1) {
> +		RTE_LOG(ERR, APP,
> +			"Master core index is not configured properly\n");
> +		app_usage(prgname);
> +		return -1;
>  	}
> -	app_used_core_mask |= 1u << app_master_core;
> +	app_used_core_mask[app_master_core] = 1;
> +
> +	if ((app_eal_core_check() == 0) ||
> +		(app_master_core != rte_get_master_lcore())) {
> +
> +		char used_hexstr[RTE_MAX_LCORE/4+1];
> +		char conf_hexstr[RTE_MAX_LCORE/4+1];
> +		int used_byte, conf_byte;
> +		struct rte_config *cfg = rte_eal_get_configuration();
> +
> +		for (i = 0; i < RTE_MAX_LCORE/4; i++) {
> +			used_byte = 0;
> +			conf_byte = 0;
> +			for (j = 0; j < 3; j++) {
> +				k = 4 * (RTE_MAX_LCORE/4 - i - 1) + j;
> +				used_byte += app_used_core_mask[k] << j;
> +				conf_byte +=
> +					((cfg->lcore_role[k] ==
> +					ROLE_RTE)?1:0) << j;
> +			}
> +			sprintf(&used_hexstr[i], "%1x", used_byte);
> +			sprintf(&conf_hexstr[i], "%1x", used_byte);
> +		}
> +
> +		RTE_LOG(ERR, APP, "EAL core mask not configured
> properly\n");
> +		RTE_LOG(ERR, APP, "  must be   : %s\n", used_hexstr);
> +		RTE_LOG(ERR, APP, "  instead of: %s\n", conf_hexstr);
> 
> -	if ((app_used_core_mask != app_eal_core_mask()) ||
> -			(app_master_core != rte_get_master_lcore())) {
> -		RTE_LOG(ERR, APP, "EAL core mask not configured properly,
> must be %" PRIx64
> -				" instead of %" PRIx64 "\n" ,
> app_used_core_mask, app_eal_core_mask());
>  		return -1;
>  	}
> 
> --
> 1.9.1


Can you please explain the root issue?

This patch contains way too much code for fixing a shift overflow issue, it is basically a rework without explaining the issue or reason/benefit for the rework.

This approach does not look right to me, I am sure there is a better and quicker way to fix the potential issue once we all understand it.

Re: [dpdk-dev] [PATCH v3] examples/qos_sched: fix bad bit shift operation

[Mrozowicz, SlawomirX]

>-----Original Message-----
>From: Dumitrescu, Cristian
>Sent: Thursday, April 28, 2016 1:16 PM
>To: Jastrzebski, MichalX K <michalx.k.jastrzebski@intel.com>; Zhang, Roy Fan
><roy.fan.zhang@intel.com>; Singh, Jasvinder <jasvinder.singh@intel.com>
>Cc: dev@dpdk.org; Mrozowicz, SlawomirX <slawomirx.mrozowicz@intel.com>
>Subject: RE: [PATCH v3] examples/qos_sched: fix bad bit shift operation
>
>
>
>> -----Original Message-----
>> From: Jastrzebski, MichalX K
>> Sent: Thursday, April 21, 2016 2:08 PM
>> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Zhang, Roy
>> Fan <roy.fan.zhang@intel.com>; Singh, Jasvinder
>> <jasvinder.singh@intel.com>
>> Cc: dev@dpdk.org; Mrozowicz, SlawomirX
><slawomirx.mrozowicz@intel.com>
>> Subject: [PATCH v3] examples/qos_sched: fix bad bit shift operation
>>
>> From: Slawomir Mrozowicz <slawomirx.mrozowicz@intel.com>
>>
>> Fix issue reported by Coverity.
>>
>> Coverity ID 30690: Bad bit shift operation
>> large_shift: In expression 1ULL << i, left shifting by more than 63
>> bits has undefined behavior. The shift amount, i, is as much as 127.
>>
>> Fixes: de3cfa2c9823 ("sched: initial import")
>>
>> Signed-off-by: Slawomir Mrozowicz <slawomirx.mrozowicz@intel.com>
>> ---
>>  examples/qos_sched/args.c | 84 +++++++++++++++++++++++++++++-----
>-
>> ------------
>>  1 file changed, 52 insertions(+), 32 deletions(-)
>>
>> diff --git a/examples/qos_sched/args.c b/examples/qos_sched/args.c
>> index 3e7fd08..cd077ba 100644
>> --- a/examples/qos_sched/args.c
>> +++ b/examples/qos_sched/args.c
>> @@ -53,7 +53,7 @@
>>
>>  static uint32_t app_master_core = 1;
>>  static uint32_t app_numa_mask;
>> -static uint64_t app_used_core_mask = 0;
>> +static int app_used_core_mask[RTE_MAX_LCORE];

Changed type of the app_used_core_mask variable to store up to RTE_MAX_LCORE cores information.

>>  static uint64_t app_used_port_mask = 0;  static uint64_t
>> app_used_rx_port_mask = 0;  static uint64_t app_used_tx_port_mask = 0;
>> @@ -115,22 +115,23 @@ static inline int str_is(const char *str, const char
>*is)
>>  	return strcmp(str, is) == 0;
>>  }
>>
>> -/* returns core mask used by DPDK */
>> -static uint64_t
>> -app_eal_core_mask(void)
>> +/* compare used core with eal configuration,
>> +	returns:
>> +		1 if equal
>> +		0 if differ */
>> +static int
>> +app_eal_core_check(void)
>>  {
>> -	uint32_t i;
>> -	uint64_t cm = 0;
>> +	uint16_t i;
>> +	int ret = 1;
>>  	struct rte_config *cfg = rte_eal_get_configuration();
>>
>> -	for (i = 0; i < RTE_MAX_LCORE; i++) {
>> -		if (cfg->lcore_role[i] == ROLE_RTE)
>> -			cm |= (1ULL << i);
>> +	for (i = 0; i < RTE_MAX_LCORE && ret; i++) {
>> +		if ((cfg->lcore_role[i] == ROLE_RTE) !=
>> app_used_core_mask[i])
>> +			ret = 0;
>>  	}
>>
>> -	cm |= (1ULL << cfg->master_lcore);
>> -
>> -	return cm;
>> +	return ret;
>>  }
>>

Added tool function app_eal_core_check() to check compatibility used cores with information stored in configuration file. The function is used below.
Removed not used function app_eal_core_mask()

>>
>> @@ -292,14 +293,9 @@ app_parse_flow_conf(const char *conf_str)
>>  	app_used_tx_port_mask |= mask;
>>  	app_used_port_mask |= mask;
>>
>> -	mask = 1lu << pconf->rx_core;
>> -	app_used_core_mask |= mask;
>> -
>> -	mask = 1lu << pconf->wt_core;
>> -	app_used_core_mask |= mask;
>> -
>> -	mask = 1lu << pconf->tx_core;
>> -	app_used_core_mask |= mask;
>> +	app_used_core_mask[pconf->rx_core] = 1;
>> +	app_used_core_mask[pconf->wt_core] = 1;
>> +	app_used_core_mask[pconf->tx_core] = 1;
>>

Change method of set the mask on each used core according to change mask type.

>>  	nb_pfc++;
>>
>> @@ -335,7 +331,7 @@ app_parse_args(int argc, char **argv)
>>  	int option_index;
>>  	const char *optname;
>>  	char *prgname = argv[0];
>> -	uint32_t i, nb_lcores;
>> +	uint16_t i, j, k, nb_lcores;
>>
>>  	static struct option lgopts[] = {
>>  		{ "pfc", 1, 0, 0 },
>> @@ -349,6 +345,9 @@ app_parse_args(int argc, char **argv)
>>  		{ NULL,  0, 0, 0 }
>>  	};
>>
>> +	for (i = 0; i < RTE_MAX_LCORE; i++)
>> +		app_used_core_mask[i] = 0;
>> +

Set initial value of the mask.

>>  	/* initialize EAL first */
>>  	ret = rte_eal_init(argc, argv);
>>  	if (ret < 0)
>> @@ -436,19 +435,40 @@ app_parse_args(int argc, char **argv)
>>  	}
>>
>>  	/* check master core index validity */
>> -	for(i = 0; i <= app_master_core; i++) {
>> -		if (app_used_core_mask & (1u << app_master_core)) {
>> -			RTE_LOG(ERR, APP, "Master core index is not
>> configured properly\n");
>> -			app_usage(prgname);
>> -			return -1;
>> -		}
>> +	if (app_used_core_mask[app_master_core] == 1) {
>> +		RTE_LOG(ERR, APP,
>> +			"Master core index is not configured properly\n");
>> +		app_usage(prgname);
>> +		return -1;
>>  	}

Changed method of checking if mask is present on master core.

>> -	app_used_core_mask |= 1u << app_master_core;
>> +	app_used_core_mask[app_master_core] = 1;
>> +

Changed method of set master core in mask.

>> +	if ((app_eal_core_check() == 0) ||
>> +		(app_master_core != rte_get_master_lcore())) {
>> +
>> +		char used_hexstr[RTE_MAX_LCORE/4+1];
>> +		char conf_hexstr[RTE_MAX_LCORE/4+1];
>> +		int used_byte, conf_byte;
>> +		struct rte_config *cfg = rte_eal_get_configuration();
>> +
>> +		for (i = 0; i < RTE_MAX_LCORE/4; i++) {
>> +			used_byte = 0;
>> +			conf_byte = 0;
>> +			for (j = 0; j < 3; j++) {
>> +				k = 4 * (RTE_MAX_LCORE/4 - i - 1) + j;
>> +				used_byte += app_used_core_mask[k] << j;
>> +				conf_byte +=
>> +					((cfg->lcore_role[k] ==
>> +					ROLE_RTE)?1:0) << j;
>> +			}
>> +			sprintf(&used_hexstr[i], "%1x", used_byte);
>> +			sprintf(&conf_hexstr[i], "%1x", used_byte);
>> +		}
>> +
>> +		RTE_LOG(ERR, APP, "EAL core mask not configured
>> properly\n");
>> +		RTE_LOG(ERR, APP, "  must be   : %s\n", used_hexstr);
>> +		RTE_LOG(ERR, APP, "  instead of: %s\n", conf_hexstr);
>>
>> -	if ((app_used_core_mask != app_eal_core_mask()) ||
>> -			(app_master_core != rte_get_master_lcore())) {
>> -		RTE_LOG(ERR, APP, "EAL core mask not configured properly,
>> must be %" PRIx64
>> -				" instead of %" PRIx64 "\n" ,
>> app_used_core_mask, app_eal_core_mask());
>>  		return -1;
>>  	}
>>

Changed method of checking compatibility used cores with information stored in configuration file (the if statement).
Extended  information about wrong eal configuration to be more readable for the user (body of the true branch).

>> --
>> 1.9.1
>
>
>Can you please explain the root issue?
>
>This patch contains way too much code for fixing a shift overflow issue, it is
>basically a rework without explaining the issue or reason/benefit for the
>rework.
>
>This approach does not look right to me, I am sure there is a better and
>quicker way to fix the potential issue once we all understand it.
>

Hi Cristian

The original problem reported in the Coverity happened in reality if there are used more then 64 lcores. I think we should fix it.

Maximum possible value of lcores is 128 according to RTE_MAX_LCORE definition in configuration file.
The problem happened because mask of the used lcores is stored in 64 bits. 
Exactly the variable app_used_core_mask has uint64_t type.

To solve this problem I extended type of the variable app_used_core_mask to array size RTE_MAX_LCORE.
In this case I should change all places where the variable was used. It is reason why I changed so much code.
Detail description you can find inside the code above.

Best Regards,
Sławomir

Re: [dpdk-dev] [PATCH v3] examples/qos_sched: fix bad bit shift operation

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Mrozowicz, SlawomirX
> Sent: Tuesday, May 10, 2016 10:40 AM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Jastrzebski,
> MichalX K <michalx.k.jastrzebski@intel.com>; Zhang, Roy Fan
> <roy.fan.zhang@intel.com>; Singh, Jasvinder <jasvinder.singh@intel.com>
> Cc: dev@dpdk.org
> Subject: RE: [PATCH v3] examples/qos_sched: fix bad bit shift operation
> 
> 
> >-----Original Message-----
> >From: Dumitrescu, Cristian
> >Sent: Thursday, April 28, 2016 1:16 PM
> >To: Jastrzebski, MichalX K <michalx.k.jastrzebski@intel.com>; Zhang, Roy
> Fan
> ><roy.fan.zhang@intel.com>; Singh, Jasvinder <jasvinder.singh@intel.com>
> >Cc: dev@dpdk.org; Mrozowicz, SlawomirX
> <slawomirx.mrozowicz@intel.com>
> >Subject: RE: [PATCH v3] examples/qos_sched: fix bad bit shift operation
> >
> >
> >
> >> -----Original Message-----
> >> From: Jastrzebski, MichalX K
> >> Sent: Thursday, April 21, 2016 2:08 PM
> >> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Zhang, Roy
> >> Fan <roy.fan.zhang@intel.com>; Singh, Jasvinder
> >> <jasvinder.singh@intel.com>
> >> Cc: dev@dpdk.org; Mrozowicz, SlawomirX
> ><slawomirx.mrozowicz@intel.com>
> >> Subject: [PATCH v3] examples/qos_sched: fix bad bit shift operation
> >>
> >> From: Slawomir Mrozowicz <slawomirx.mrozowicz@intel.com>
> >>
> >> Fix issue reported by Coverity.
> >>
> >> Coverity ID 30690: Bad bit shift operation
> >> large_shift: In expression 1ULL << i, left shifting by more than 63
> >> bits has undefined behavior. The shift amount, i, is as much as 127.
> >>
> >> Fixes: de3cfa2c9823 ("sched: initial import")
> >>
> >> Signed-off-by: Slawomir Mrozowicz <slawomirx.mrozowicz@intel.com>
> >> ---
> >>  examples/qos_sched/args.c | 84 +++++++++++++++++++++++++++++--
> ---
> >-
> >> ------------
> >>  1 file changed, 52 insertions(+), 32 deletions(-)
> >>
> >> diff --git a/examples/qos_sched/args.c b/examples/qos_sched/args.c
> >> index 3e7fd08..cd077ba 100644
> >> --- a/examples/qos_sched/args.c
> >> +++ b/examples/qos_sched/args.c
> >> @@ -53,7 +53,7 @@
> >>
> >>  static uint32_t app_master_core = 1;
> >>  static uint32_t app_numa_mask;
> >> -static uint64_t app_used_core_mask = 0;
> >> +static int app_used_core_mask[RTE_MAX_LCORE];
> 
> Changed type of the app_used_core_mask variable to store up to
> RTE_MAX_LCORE cores information.
> 
> >>  static uint64_t app_used_port_mask = 0;  static uint64_t
> >> app_used_rx_port_mask = 0;  static uint64_t app_used_tx_port_mask =
> 0;
> >> @@ -115,22 +115,23 @@ static inline int str_is(const char *str, const char
> >*is)
> >>  	return strcmp(str, is) == 0;
> >>  }
> >>
> >> -/* returns core mask used by DPDK */
> >> -static uint64_t
> >> -app_eal_core_mask(void)
> >> +/* compare used core with eal configuration,
> >> +	returns:
> >> +		1 if equal
> >> +		0 if differ */
> >> +static int
> >> +app_eal_core_check(void)
> >>  {
> >> -	uint32_t i;
> >> -	uint64_t cm = 0;
> >> +	uint16_t i;
> >> +	int ret = 1;
> >>  	struct rte_config *cfg = rte_eal_get_configuration();
> >>
> >> -	for (i = 0; i < RTE_MAX_LCORE; i++) {
> >> -		if (cfg->lcore_role[i] == ROLE_RTE)
> >> -			cm |= (1ULL << i);
> >> +	for (i = 0; i < RTE_MAX_LCORE && ret; i++) {
> >> +		if ((cfg->lcore_role[i] == ROLE_RTE) !=
> >> app_used_core_mask[i])
> >> +			ret = 0;
> >>  	}
> >>
> >> -	cm |= (1ULL << cfg->master_lcore);
> >> -
> >> -	return cm;
> >> +	return ret;
> >>  }
> >>
> 
> Added tool function app_eal_core_check() to check compatibility used cores
> with information stored in configuration file. The function is used below.
> Removed not used function app_eal_core_mask()
> 
> >>
> >> @@ -292,14 +293,9 @@ app_parse_flow_conf(const char *conf_str)
> >>  	app_used_tx_port_mask |= mask;
> >>  	app_used_port_mask |= mask;
> >>
> >> -	mask = 1lu << pconf->rx_core;
> >> -	app_used_core_mask |= mask;
> >> -
> >> -	mask = 1lu << pconf->wt_core;
> >> -	app_used_core_mask |= mask;
> >> -
> >> -	mask = 1lu << pconf->tx_core;
> >> -	app_used_core_mask |= mask;
> >> +	app_used_core_mask[pconf->rx_core] = 1;
> >> +	app_used_core_mask[pconf->wt_core] = 1;
> >> +	app_used_core_mask[pconf->tx_core] = 1;
> >>
> 
> Change method of set the mask on each used core according to change mask
> type.
> 
> >>  	nb_pfc++;
> >>
> >> @@ -335,7 +331,7 @@ app_parse_args(int argc, char **argv)
> >>  	int option_index;
> >>  	const char *optname;
> >>  	char *prgname = argv[0];
> >> -	uint32_t i, nb_lcores;
> >> +	uint16_t i, j, k, nb_lcores;
> >>
> >>  	static struct option lgopts[] = {
> >>  		{ "pfc", 1, 0, 0 },
> >> @@ -349,6 +345,9 @@ app_parse_args(int argc, char **argv)
> >>  		{ NULL,  0, 0, 0 }
> >>  	};
> >>
> >> +	for (i = 0; i < RTE_MAX_LCORE; i++)
> >> +		app_used_core_mask[i] = 0;
> >> +
> 
> Set initial value of the mask.
> 
> >>  	/* initialize EAL first */
> >>  	ret = rte_eal_init(argc, argv);
> >>  	if (ret < 0)
> >> @@ -436,19 +435,40 @@ app_parse_args(int argc, char **argv)
> >>  	}
> >>
> >>  	/* check master core index validity */
> >> -	for(i = 0; i <= app_master_core; i++) {
> >> -		if (app_used_core_mask & (1u << app_master_core)) {
> >> -			RTE_LOG(ERR, APP, "Master core index is not
> >> configured properly\n");
> >> -			app_usage(prgname);
> >> -			return -1;
> >> -		}
> >> +	if (app_used_core_mask[app_master_core] == 1) {
> >> +		RTE_LOG(ERR, APP,
> >> +			"Master core index is not configured properly\n");
> >> +		app_usage(prgname);
> >> +		return -1;
> >>  	}
> 
> Changed method of checking if mask is present on master core.
> 
> >> -	app_used_core_mask |= 1u << app_master_core;
> >> +	app_used_core_mask[app_master_core] = 1;
> >> +
> 
> Changed method of set master core in mask.
> 
> >> +	if ((app_eal_core_check() == 0) ||
> >> +		(app_master_core != rte_get_master_lcore())) {
> >> +
> >> +		char used_hexstr[RTE_MAX_LCORE/4+1];
> >> +		char conf_hexstr[RTE_MAX_LCORE/4+1];
> >> +		int used_byte, conf_byte;
> >> +		struct rte_config *cfg = rte_eal_get_configuration();
> >> +
> >> +		for (i = 0; i < RTE_MAX_LCORE/4; i++) {
> >> +			used_byte = 0;
> >> +			conf_byte = 0;
> >> +			for (j = 0; j < 3; j++) {
> >> +				k = 4 * (RTE_MAX_LCORE/4 - i - 1) + j;
> >> +				used_byte += app_used_core_mask[k] << j;
> >> +				conf_byte +=
> >> +					((cfg->lcore_role[k] ==
> >> +					ROLE_RTE)?1:0) << j;
> >> +			}
> >> +			sprintf(&used_hexstr[i], "%1x", used_byte);
> >> +			sprintf(&conf_hexstr[i], "%1x", used_byte);
> >> +		}
> >> +
> >> +		RTE_LOG(ERR, APP, "EAL core mask not configured
> >> properly\n");
> >> +		RTE_LOG(ERR, APP, "  must be   : %s\n", used_hexstr);
> >> +		RTE_LOG(ERR, APP, "  instead of: %s\n", conf_hexstr);
> >>
> >> -	if ((app_used_core_mask != app_eal_core_mask()) ||
> >> -			(app_master_core != rte_get_master_lcore())) {
> >> -		RTE_LOG(ERR, APP, "EAL core mask not configured properly,
> >> must be %" PRIx64
> >> -				" instead of %" PRIx64 "\n" ,
> >> app_used_core_mask, app_eal_core_mask());
> >>  		return -1;
> >>  	}
> >>
> 
> Changed method of checking compatibility used cores with information
> stored in configuration file (the if statement).
> Extended  information about wrong eal configuration to be more readable
> for the user (body of the true branch).
> 
> >> --
> >> 1.9.1
> >
> >
> >Can you please explain the root issue?
> >
> >This patch contains way too much code for fixing a shift overflow issue, it is
> >basically a rework without explaining the issue or reason/benefit for the
> >rework.
> >
> >This approach does not look right to me, I am sure there is a better and
> >quicker way to fix the potential issue once we all understand it.
> >
> 
> Hi Cristian
> 
> The original problem reported in the Coverity happened in reality if there are
> used more then 64 lcores. I think we should fix it.
> 
> Maximum possible value of lcores is 128 according to RTE_MAX_LCORE
> definition in configuration file.
> The problem happened because mask of the used lcores is stored in 64 bits.
> Exactly the variable app_used_core_mask has uint64_t type.
> 
> To solve this problem I extended type of the variable app_used_core_mask
> to array size RTE_MAX_LCORE.
> In this case I should change all places where the variable was used. It is
> reason why I changed so much code.
> Detail description you can find inside the code above.
> 
> Best Regards,
> Sławomir
> 
> 
> 

This is a false problem, as we will never use more than 64 lcores with this application. The typical number of lcores used with this app is 3 or 6, with 12 as the absolute maximum when 4 x 3 lcores are used to handle 4 x 10GbE ports.

The fix you are looking for is a quick and straightforward way to limit the max number of lcores use d by this app to 64. Can you look for this type of solution, please?

Thanks,
Cristian