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

["Mrozowicz, SlawomirX" <slawomirx.mrozowicz@intel.com>]

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