From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id A4D0D6949 for ; Tue, 10 May 2016 11:39:52 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga101.fm.intel.com with ESMTP; 10 May 2016 02:39:52 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,604,1455004800"; d="scan'208";a="962544758" Received: from irsmsx151.ger.corp.intel.com ([163.33.192.59]) by fmsmga001.fm.intel.com with ESMTP; 10 May 2016 02:39:50 -0700 Received: from irsmsx103.ger.corp.intel.com ([169.254.3.54]) by IRSMSX151.ger.corp.intel.com ([169.254.4.7]) with mapi id 14.03.0248.002; Tue, 10 May 2016 10:39:50 +0100 From: "Mrozowicz, SlawomirX" To: "Dumitrescu, Cristian" , "Jastrzebski, MichalX K" , "Zhang, Roy Fan" , "Singh, Jasvinder" CC: "dev@dpdk.org" Thread-Topic: [PATCH v3] examples/qos_sched: fix bad bit shift operation Thread-Index: AQHRm87xCoezDqE7r0OjvZD9IRZn35+fNmmAgBK/8pA= Date: Tue, 10 May 2016 09:39:49 +0000 Message-ID: <158888A50F43E34AAE179517F56C97455A4600@IRSMSX103.ger.corp.intel.com> References: <1461244093-2008-1-git-send-email-michalx.k.jastrzebski@intel.com> <1461244093-2008-3-git-send-email-michalx.k.jastrzebski@intel.com> <3EB4FA525960D640B5BDFFD6A3D89126479A6F2B@IRSMSX108.ger.corp.intel.com> In-Reply-To: <3EB4FA525960D640B5BDFFD6A3D89126479A6F2B@IRSMSX108.ger.corp.intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.182] Content-Type: text/plain; charset="iso-8859-2" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v3] examples/qos_sched: fix bad bit shift operation X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 10 May 2016 09:39:53 -0000 >-----Original Message----- >From: Dumitrescu, Cristian >Sent: Thursday, April 28, 2016 1:16 PM >To: Jastrzebski, MichalX K ; Zhang, Roy F= an >; Singh, Jasvinder >Cc: dev@dpdk.org; Mrozowicz, SlawomirX >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 ; Zhang, Roy >> Fan ; Singh, Jasvinder >> >> Cc: dev@dpdk.org; Mrozowicz, SlawomirX > >> Subject: [PATCH v3] examples/qos_sched: fix bad bit shift operation >> >> From: 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 >> --- >> 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 =3D 1; >> static uint32_t app_numa_mask; >> -static uint64_t app_used_core_mask =3D 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_LCOR= E cores information. >> static uint64_t app_used_port_mask =3D 0; static uint64_t >> app_used_rx_port_mask =3D 0; static uint64_t app_used_tx_port_mask =3D = 0; >> @@ -115,22 +115,23 @@ static inline int str_is(const char *str, const ch= ar >*is) >> return strcmp(str, is) =3D=3D 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 =3D 0; >> + uint16_t i; >> + int ret =3D 1; >> struct rte_config *cfg =3D rte_eal_get_configuration(); >> >> - for (i =3D 0; i < RTE_MAX_LCORE; i++) { >> - if (cfg->lcore_role[i] =3D=3D ROLE_RTE) >> - cm |=3D (1ULL << i); >> + for (i =3D 0; i < RTE_MAX_LCORE && ret; i++) { >> + if ((cfg->lcore_role[i] =3D=3D ROLE_RTE) !=3D >> app_used_core_mask[i]) >> + ret =3D 0; >> } >> >> - cm |=3D (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 |=3D mask; >> app_used_port_mask |=3D mask; >> >> - mask =3D 1lu << pconf->rx_core; >> - app_used_core_mask |=3D mask; >> - >> - mask =3D 1lu << pconf->wt_core; >> - app_used_core_mask |=3D mask; >> - >> - mask =3D 1lu << pconf->tx_core; >> - app_used_core_mask |=3D mask; >> + app_used_core_mask[pconf->rx_core] =3D 1; >> + app_used_core_mask[pconf->wt_core] =3D 1; >> + app_used_core_mask[pconf->tx_core] =3D 1; >> Change method of set the mask on each used core according to change mask ty= pe. >> nb_pfc++; >> >> @@ -335,7 +331,7 @@ app_parse_args(int argc, char **argv) >> int option_index; >> const char *optname; >> char *prgname =3D argv[0]; >> - uint32_t i, nb_lcores; >> + uint16_t i, j, k, nb_lcores; >> >> static struct option lgopts[] =3D { >> { "pfc", 1, 0, 0 }, >> @@ -349,6 +345,9 @@ app_parse_args(int argc, char **argv) >> { NULL, 0, 0, 0 } >> }; >> >> + for (i =3D 0; i < RTE_MAX_LCORE; i++) >> + app_used_core_mask[i] =3D 0; >> + Set initial value of the mask. >> /* initialize EAL first */ >> ret =3D 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 =3D 0; i <=3D 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] =3D=3D 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 |=3D 1u << app_master_core; >> + app_used_core_mask[app_master_core] =3D 1; >> + Changed method of set master core in mask. >> + if ((app_eal_core_check() =3D=3D 0) || >> + (app_master_core !=3D 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 =3D rte_eal_get_configuration(); >> + >> + for (i =3D 0; i < RTE_MAX_LCORE/4; i++) { >> + used_byte =3D 0; >> + conf_byte =3D 0; >> + for (j =3D 0; j < 3; j++) { >> + k =3D 4 * (RTE_MAX_LCORE/4 - i - 1) + j; >> + used_byte +=3D app_used_core_mask[k] << j; >> + conf_byte +=3D >> + ((cfg->lcore_role[k] =3D=3D >> + 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 !=3D app_eal_core_mask()) || >> - (app_master_core !=3D 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, i= t 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 definiti= on in configuration file. The problem happened because mask of the used lcores is stored in 64 bits.= =20 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=B3awomir