Re: [PATCH v3 3/7] service cores: coremask parsing

From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
Sent: Friday, July 7, 2017 11:46 AM
To: Van Haaren, Harry <redacted>
Cc: dev@dpdk.org; thomas@monjalon.net; Wiles, Keith <redacted>; Richardson,
Bruce [off-list ref]
Subject: Re: [PATCH v3 3/7] service cores: coremask parsing

-----Original Message-----

quoted

Date: Thu, 6 Jul 2017 14:47:20 +0000
From: "Van Haaren, Harry" <redacted>
To: Jerin Jacob <redacted>
CC: "dev@dpdk.org" <redacted>, "thomas@monjalon.net"
 [off-list ref], "Wiles, Keith" [off-list ref],
 "Richardson, Bruce" [off-list ref]
Subject: RE: [PATCH v3 3/7] service cores: coremask parsing

quoted

From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
Sent: Tuesday, July 4, 2017 1:46 PM
To: Van Haaren, Harry <redacted>
Cc: dev@dpdk.org; thomas@monjalon.net; Wiles, Keith <redacted>;

Richardson,

quoted

quoted

Bruce [off-list ref]
Subject: Re: [PATCH v3 3/7] service cores: coremask parsing

-----Original Message-----

quoted

Date: Sun, 2 Jul 2017 22:35:10 +0100
From: Harry van Haaren <redacted>
To: dev@dpdk.org
CC: jerin.jacob@caviumnetworks.com, thomas@monjalon.net,
 keith.wiles@intel.com, bruce.richardson@intel.com, Harry van Haaren
 [off-list ref]
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 <redacted>

diff --git a/lib/librte_eal/common/eal_common_options.c

b/lib/librte_eal/common/eal_common_options.c

quoted

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;

quoted

quoted

 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).

quoted

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.

quoted

3) Allocate temp memory to store the service-core indexes, and later free that back

(feels hacky to me?)

quoted

Current scheme of (1) makes the most sense to me.

Yes. Make sense to keep option 1.

Great,

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.

Agreed - lets leave this cleanup of EAL to another patchset. That patchset can consolidate the various coremask functions in EAL and other parts of DPDK (I recall a similar comment for eventdev_pipeline sample-app).

quoted

quoted

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!!

I'm working on improving one of the unit tests to launch app functions to verify ROLE_RTE, and will post then.

Thanks for the input, -Harry