From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jerin Jacob Subject: Re: [PATCH v3 3/7] service cores: coremask parsing Date: Fri, 7 Jul 2017 16:15:30 +0530 Message-ID: <20170707104529.GA24514@jerin> 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 Cc: "dev@dpdk.org" , "thomas@monjalon.net" , "Wiles, Keith" , "Richardson, Bruce" To: "Van Haaren, Harry" Return-path: Received: from NAM03-BY2-obe.outbound.protection.outlook.com (mail-by2nam03on0047.outbound.protection.outlook.com [104.47.42.47]) by dpdk.org (Postfix) with ESMTP id C256B2C8 for ; Fri, 7 Jul 2017 12:46:12 +0200 (CEST) Content-Disposition: inline In-Reply-To: List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" -----Original Message----- > Date: Thu, 6 Jul 2017 14:47:20 +0000 > From: "Van Haaren, Harry" > To: Jerin Jacob > CC: "dev@dpdk.org" , "thomas@monjalon.net" > , "Wiles, Keith" , > "Richardson, Bruce" > Subject: RE: [PATCH v3 3/7] service cores: coremask parsing > > > 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 > > > > -----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[] = > > > "b:" /* pci-blacklist */ > > > "c:" /* coremask */ > > > + "s:" /* service coremask */ > > > "d:" /* driver */ > > > "h" /* help */ > > > "l:" /* corelist */ > > > @@ -267,6 +268,73 @@ static int xdigit2val(unsigned char c) > > > } > > > > Missing the --help update for service coremask details. > > > > 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; > > > > static int > > > +eal_parse_service_coremask(const char *coremask) > > > +{ > > > + struct rte_config *cfg = rte_eal_get_configuration(); > > > + int i, j, idx = 0; > > > + unsigned int count = 0; > > > + char c; > > > + int val; > > > + > > > + if (coremask == NULL) > > > + return -1; > > > + /* Remove all blank characters ahead and after . > > > + * Remove 0x/0X if exists. > > > + */ > > > + while (isblank(*coremask)) > > > + coremask++; > > > + if (coremask[0] == '0' && ((coremask[1] == 'x') > > > + || (coremask[1] == 'X'))) > > > + coremask += 2; > > > + i = strlen(coremask); > > > + while ((i > 0) && isblank(coremask[i - 1])) > > > + i--; > > > + > > > + if (i == 0) > > > + return -1; > > > + > > > + for (i = i - 1; i >= 0 && idx < RTE_MAX_LCORE; i--) { > > > + c = coremask[i]; > > > + if (isxdigit(c) == 0) { > > > + /* invalid characters */ > > > + return -1; > > > + } > > > + val = xdigit2val(c); > > > + for (j = 0; j < BITS_PER_HEX && idx < RTE_MAX_LCORE; > > > + j++, idx++) { > > > + if ((1 << j) & val) { > > > + /* handle master lcore already parsed */ > > > + uint32_t lcore = idx; > > > + if (master_lcore_parsed && > > > + cfg->master_lcore == lcore) > > > + continue; > > > + > > > + if (!lcore_config[idx].detected) { > > > + RTE_LOG(ERR, EAL, > > > + "lcore %u unavailable\n", idx); > > > + return -1; > > > + } > > > + lcore_config[idx].core_role = ROLE_SERVICE; > > > > 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 almost 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 -> validate-> config -> run" workflow. > 3) Allocate temp memory to store the service-core indexes, and later free that back (feels hacky to me?) > > Current scheme of (1) makes the most sense to me. Yes. Make sense to keep option 1. One suggestion: There is a lot duplicate code between new eal_parse_service_coremask() and eal_parse_coremask() on the same file. I think, we can add a common parsing logic and on match, the actions can be invoked through function pointer which is passed in the parsing function. > > > > 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. > > > > 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. Great!! >