> -----Original Message----- > From: Mrozowicz, SlawomirX > Sent: Tuesday, May 10, 2016 10:40 AM > To: Dumitrescu, Cristian <cristian.dumitrescu at intel.com>; Jastrzebski, > MichalX K <michalx.k.jastrzebski at intel.com>; Zhang, Roy Fan > <roy.fan.zhang at intel.com>; Singh, Jasvinder <jasvinder.singh at intel.com> > Cc: dev at 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 at intel.com>; Zhang, Roy > Fan > ><roy.fan.zhang at intel.com>; Singh, Jasvinder <jasvinder.singh at intel.com> > >Cc: dev at dpdk.org; Mrozowicz, SlawomirX > <slawomirx.mrozowicz at 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 at intel.com>; Zhang, Roy > >> Fan <roy.fan.zhang at intel.com>; Singh, Jasvinder > >> <jasvinder.singh at intel.com> > >> Cc: dev at dpdk.org; Mrozowicz, SlawomirX > ><slawomirx.mrozowicz at intel.com> > >> Subject: [PATCH v3] examples/qos_sched: fix bad bit shift operation > >> > >> From: Slawomir Mrozowicz <slawomirx.mrozowicz at 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 at 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