From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Van Haaren, Harry" Subject: Re: [PATCH v3 3/7] service cores: coremask parsing Date: Thu, 6 Jul 2017 14:47:20 +0000 Message-ID: References: <1498735421-100164-1-git-send-email-harry.van.haaren@intel.com> <1499031314-7172-1-git-send-email-harry.van.haaren@intel.com> <1499031314-7172-4-git-send-email-harry.van.haaren@intel.com> <20170704124548.GB14921@jerin> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "dev@dpdk.org" , "thomas@monjalon.net" , "Wiles, Keith" , "Richardson, Bruce" To: Jerin Jacob Return-path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 12BF22FDD for ; Thu, 6 Jul 2017 16:47:23 +0200 (CEST) In-Reply-To: <20170704124548.GB14921@jerin> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com] > Sent: Tuesday, July 4, 2017 1:46 PM > To: Van Haaren, Harry > Cc: dev@dpdk.org; thomas@monjalon.net; Wiles, Keith ; Richardson, > Bruce > Subject: Re: [PATCH v3 3/7] service cores: coremask parsing >=20 > -----Original Message----- > > Date: Sun, 2 Jul 2017 22:35:10 +0100 > > From: Harry van Haaren > > To: dev@dpdk.org > > CC: jerin.jacob@caviumnetworks.com, thomas@monjalon.net, > > keith.wiles@intel.com, bruce.richardson@intel.com, Harry van Haaren > > > > Subject: [PATCH v3 3/7] service cores: coremask parsing > > X-Mailer: git-send-email 2.7.4 > > > > Add logic for parsing a coremask from EAL, which allows > > the application to be unaware of the cores being taken from > > its coremask. > > > > Signed-off-by: Harry van Haaren > > > > diff --git a/lib/librte_eal/common/eal_common_options.c > b/lib/librte_eal/common/eal_common_options.c > > index f470195..cee200c 100644 > > --- a/lib/librte_eal/common/eal_common_options.c > > +++ b/lib/librte_eal/common/eal_common_options.c > > @@ -61,6 +61,7 @@ const char > > eal_short_options[] =3D > > "b:" /* pci-blacklist */ > > "c:" /* coremask */ > > + "s:" /* service coremask */ > > "d:" /* driver */ > > "h" /* help */ > > "l:" /* corelist */ > > @@ -267,6 +268,73 @@ static int xdigit2val(unsigned char c) > > } >=20 > Missing the --help update for service coremask details. >=20 > I think, EAL arguments are documented in another area of doc directory > as well. Update the documents. Will double check / fix this. Replying here now to advance discussion below= ;=20 > > static int > > +eal_parse_service_coremask(const char *coremask) > > +{ > > + struct rte_config *cfg =3D rte_eal_get_configuration(); > > + int i, j, idx =3D 0; > > + unsigned int count =3D 0; > > + char c; > > + int val; > > + > > + if (coremask =3D=3D NULL) > > + return -1; > > + /* Remove all blank characters ahead and after . > > + * Remove 0x/0X if exists. > > + */ > > + while (isblank(*coremask)) > > + coremask++; > > + if (coremask[0] =3D=3D '0' && ((coremask[1] =3D=3D 'x') > > + || (coremask[1] =3D=3D 'X'))) > > + coremask +=3D 2; > > + i =3D strlen(coremask); > > + while ((i > 0) && isblank(coremask[i - 1])) > > + i--; > > + > > + if (i =3D=3D 0) > > + return -1; > > + > > + for (i =3D i - 1; i >=3D 0 && idx < RTE_MAX_LCORE; i--) { > > + c =3D coremask[i]; > > + if (isxdigit(c) =3D=3D 0) { > > + /* invalid characters */ > > + return -1; > > + } > > + val =3D xdigit2val(c); > > + for (j =3D 0; j < BITS_PER_HEX && idx < RTE_MAX_LCORE; > > + j++, idx++) { > > + if ((1 << j) & val) { > > + /* handle master lcore already parsed */ > > + uint32_t lcore =3D idx; > > + if (master_lcore_parsed && > > + cfg->master_lcore =3D=3D lcore) > > + continue; > > + > > + if (!lcore_config[idx].detected) { > > + RTE_LOG(ERR, EAL, > > + "lcore %u unavailable\n", idx); > > + return -1; > > + } > > + lcore_config[idx].core_role =3D ROLE_SERVICE; >=20 > Why not to use rte_service_lcore_add(idx) here. So that in future some > changes we don't need to touch this file. The issue here is that the hugepages memory that service-cores requires is = not available at this point. Keep in mind, the EAL parse-opts runs before a= lmost anything else (makes sense, given we can specify e.g. --no-huge). Given that there is not rte_malloc() available at this point, we have a few= options: 1) Use existing allocated mem, e.g. the lcore_config[] array as above. 2) Delay the parsing of service-core mask until later. Breaks "parse -> val= idate-> config -> run" workflow. 3) Allocate temp memory to store the service-core indexes, and later free t= hat back (feels hacky to me?) Current scheme of (1) makes the most sense to me. > I added following code in unit testcase and I have 8 cores system. So I > was expecting cores prints from "0 3 4 5 6 7" as lcore 1 and 2 will be > stolen by service core. But it looks like RTE_LCORE_FOREACH not honoring > previous rte_service_lcore_add() functions. >=20 > testsuite_setup(void) > { > + int i; > + rte_service_lcore_add(1); > + rte_service_lcore_add(2); > + > + RTE_LCORE_FOREACH(i) > + printf("cores %d\n", i); Root cause found - and fixed. If you don't strongly object to lcore_config[= ] method above, then I can prioritize this and try get a patchset up ASAP.