From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 7C8E095CC for ; Tue, 10 May 2016 11:45:39 +0200 (CEST) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga104.fm.intel.com with ESMTP; 10 May 2016 02:45:38 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,604,1455004800"; d="scan'208";a="100404566" Received: from irsmsx106.ger.corp.intel.com ([163.33.3.31]) by fmsmga004.fm.intel.com with ESMTP; 10 May 2016 02:45:37 -0700 Received: from irsmsx108.ger.corp.intel.com ([169.254.11.241]) by IRSMSX106.ger.corp.intel.com ([169.254.8.131]) with mapi id 14.03.0248.002; Tue, 10 May 2016 10:45:37 +0100 From: "Dumitrescu, Cristian" To: "Mrozowicz, SlawomirX" , "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/8pCAABJCoA== Date: Tue, 10 May 2016 09:45:36 +0000 Message-ID: <3EB4FA525960D640B5BDFFD6A3D89126479BABDA@IRSMSX108.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> <158888A50F43E34AAE179517F56C97455A4600@IRSMSX103.ger.corp.intel.com> In-Reply-To: <158888A50F43E34AAE179517F56C97455A4600@IRSMSX103.ger.corp.intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNTg1MmQ1ZmItMjgyYS00MzYzLWIwZTAtN2NjMGQwYjAzNGMwIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6ImlJMUUzMmpzZjgxcTMraUtwT3dFUWd5YlBEbmt1SnBOYitwZ0d5a2tDN1E9In0= x-ctpclassification: CTP_IC 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:45:40 -0000 > -----Original Message----- > From: Mrozowicz, SlawomirX > Sent: Tuesday, May 10, 2016 10:40 AM > To: Dumitrescu, Cristian ; Jastrzebski, > MichalX K ; Zhang, Roy Fan > ; Singh, Jasvinder > Cc: dev@dpdk.org > Subject: RE: [PATCH v3] examples/qos_sched: fix bad bit shift operation >=20 >=20 > >-----Original Message----- > >From: Dumitrescu, Cristian > >Sent: Thursday, April 28, 2016 1:16 PM > >To: Jastrzebski, MichalX K ; Zhang, Roy > Fan > >; 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]; >=20 > Changed type of the app_used_core_mask variable to store up to > RTE_MAX_LCORE cores information. >=20 > >> 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 = char > >*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; > >> } > >> >=20 > Added tool function app_eal_core_check() to check compatibility used core= s > with information stored in configuration file. The function is used below= . > Removed not used function app_eal_core_mask() >=20 > >> > >> @@ -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; > >> >=20 > Change method of set the mask on each used core according to change mask > type. >=20 > >> 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; > >> + >=20 > Set initial value of the mask. >=20 > >> /* 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; > >> } >=20 > Changed method of checking if mask is present on master core. >=20 > >> - app_used_core_mask |=3D 1u << app_master_core; > >> + app_used_core_mask[app_master_core] =3D 1; > >> + >=20 > Changed method of set master core in mask. >=20 > >> + 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; > >> } > >> >=20 > 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). >=20 > >> -- > >> 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 th= e > >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. > > >=20 > Hi Cristian >=20 > The original problem reported in the Coverity happened in reality if ther= e are > used more then 64 lcores. I think we should fix it. >=20 > 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. >=20 > 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 i= s > reason why I changed so much code. > Detail description you can find inside the code above. >=20 > Best Regards, > S=B3awomir >=20 >=20 >=20 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, wi= th 12 as the absolute maximum when 4 x 3 lcores are used to handle 4 x 10Gb= E 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 o= f solution, please? Thanks, Cristian