eal: Fix misleading error messages, errno can't be trusted.

  lib/librte_eal/linuxapp/eal/eal.c        | 14 +++++++++++---
  lib/librte_eal/linuxapp/eal/eal_memory.c | 16 ++++++++++++----
  2 files changed, 23 insertions(+), 7 deletions(-)

lib/librte_eal/linuxapp/eal/eal.c +5 -1

index 3fb2188..5df9f6a 100644
@@ -238,7 +238,8 @@ rte_eal_config_attach(void)
  	mem_config = (struct rte_mem_config *) mmap(NULL, sizeof(*mem_config),
  			PROT_READ, MAP_SHARED, mem_cfg_fd, 0);
  	if (mem_config == MAP_FAILED)
-		rte_panic("Cannot mmap memory for rte_config\n");
+		rte_panic("Cannot mmap memory for rte_config! error %i (%s)\n",
+			  errno, strerror(errno));
 
  	rte_config.mem_config = mem_config;
  }

@@ -263,9 +264,16 @@ rte_eal_config_reattach(void)
  	mem_config = (struct rte_mem_config *) mmap(rte_mem_cfg_addr,
  			sizeof(*mem_config), PROT_READ | PROT_WRITE, MAP_SHARED,
  			mem_cfg_fd, 0);
+	if (mem_config == MAP_FAILED || mem_config != rte_mem_cfg_addr) {
+		if (mem_config != MAP_FAILED)
+			/* errno is stale, don't use */

lib/librte_eal/linuxapp/eal/eal.c +0 -0

  	rte_config.mem_config = mem_config;
  }

lib/librte_eal/linuxapp/eal/eal_memory.c +12 -4

index 41e0a92..b036ffc 100644
@@ -1615,10 +1615,18 @@ rte_eal_hugepage_attach(void)
  				 PROT_READ, MAP_PRIVATE, fd_zero, 0);
  		if (base_addr == MAP_FAILED ||
  		    base_addr != mcfg->memseg[s].addr) {
-			RTE_LOG(ERR, EAL, "Could not mmap %llu bytes "
-				"in /dev/zero to requested address [%p]: '%s'\n",
-				(unsigned long long)mcfg->memseg[s].len,
-				mcfg->memseg[s].addr, strerror(errno));
+			if (base_addr != MAP_FAILED)
+				/* errno is stale, don't use */
+				RTE_LOG(ERR, EAL, "Could not mmap %llu bytes "
+					"in /dev/zero at [%p], got [%p] - "
+					"please use '--base-virtaddr' option\n",
+					(unsigned long long)mcfg->memseg[s].len,
+					mcfg->memseg[s].addr, base_addr);
+			else
+				RTE_LOG(ERR, EAL, "Could not mmap %llu bytes "
+					"in /dev/zero at [%p]: '%s'\n",
+					(unsigned long long)mcfg->memseg[s].len,
+					mcfg->memseg[s].addr, strerror(errno));
  			if (aslr_enabled() > 0) {
  				RTE_LOG(ERR, EAL, "It is recommended to "
  					"disable ASLR in the kernel "

Hi Jean,

There are some format issues with the patch:

You can run scripts/check-git-log.sh to check them:
Wrong headline format:
eal: Fix misleading error messages, errno can't be trusted.
Wrong headline uppercase:
eal: Fix misleading error messages, errno can't be trusted.
Missing 'Fixes' tag:
eal: Fix misleading error messages, errno can't be trusted.

The script's output highlights the different issues.

Quoted 11 lines

lib/librte_eal/linuxapp/eal/eal.c +7 -0

@@ -263,9 +264,16 @@ rte_eal_config_reattach(void)
 	mem_config = (struct rte_mem_config *) mmap(rte_mem_cfg_addr,
 			sizeof(*mem_config), PROT_READ | PROT_WRITE, MAP_SHARED,
 			mem_cfg_fd, 0);
+	if (mem_config == MAP_FAILED || mem_config != rte_mem_cfg_addr) {
+		if (mem_config != MAP_FAILED)
+			/* errno is stale, don't use */
+			rte_panic("Cannot mmap memory for rte_config at [%p], got [%p] - please use '--base-virtaddr' option\n",
+				  rte_mem_cfg_addr, mem_config);
+		else
+			rte_panic("Cannot mmap memory for rte_config! error %i (%s)\n",

lib/librte_eal/linuxapp/eal/eal.c rte_eal_config_reattach(void) · line 268

Quoted 5 lines

lib/librte_eal/linuxapp/eal/eal.c +2 -2

+				  errno, strerror(errno));
+	}
 	close(mem_cfg_fd);
-	if (mem_config == MAP_FAILED || mem_config != rte_mem_cfg_addr)
-		rte_panic("Cannot mmap memory for rte_config\n");

NIT but any reason you moved the check before closing the file descriptor? (not that it matters with current code as we panic anyway)

Thanks,
Sergio

-----Original Message-----
From: dev [mailto:[email protected]] On Behalf Of Jean Tourrilhes
Sent: Monday, October 3, 2016 4:56 PM
To: Gonzalez Monroy, Sergio <[email protected]>
Cc: [email protected]
Subject: Re: [dpdk-dev] [PATCH 1/1 v2] eal: Fix misleading error messages, errno can't be trusted.

Quoted 12 lines · Sergio Gonzalez Monroy

Hi Jean,

There are some format issues with the patch:

You can run scripts/check-git-log.sh to check them:
Wrong headline format:
eal: Fix misleading error messages, errno can't be trusted.
Wrong headline uppercase:
eal: Fix misleading error messages, errno can't be trusted.
Missing 'Fixes' tag:
eal: Fix misleading error messages, errno can't be trusted.

Quoted 1 line

The script's output highlights the different issues.

SOrry about that, I casually read the page on http://dpdk.org/dev,
but obviously I need to look at it again.

The longer more detailed version is here: "Contributing Code to DPDK":

http://dpdk.org/doc/guides/contributing/patches.html

John

Quoted 12 lines

Hi Jean,

There are some format issues with the patch:

You can run scripts/check-git-log.sh to check them:
Wrong headline format:
eal: Fix misleading error messages, errno can't be trusted.
Wrong headline uppercase:
eal: Fix misleading error messages, errno can't be trusted.
Missing 'Fixes' tag:
eal: Fix misleading error messages, errno can't be trusted.

Quoted 1 line

The script's output highlights the different issues.

SOrry about that, I casually read the page on
http://dpdk.org/dev, but obviously I need to look at it again.

Quoted 12 lines · Jean Tourrilhes

Quoted 11 lines

lib/librte_eal/linuxapp/eal/eal.c +7 -0

@@ -263,9 +264,16 @@ rte_eal_config_reattach(void)
 	mem_config = (struct rte_mem_config *) mmap(rte_mem_cfg_addr,
 			sizeof(*mem_config), PROT_READ | PROT_WRITE, MAP_SHARED,
 			mem_cfg_fd, 0);
+	if (mem_config == MAP_FAILED || mem_config != rte_mem_cfg_addr) {
+		if (mem_config != MAP_FAILED)
+			/* errno is stale, don't use */
+			rte_panic("Cannot mmap memory for rte_config at [%p], got [%p] - please use '--base-virtaddr' option\n",
+				  rte_mem_cfg_addr, mem_config);
+		else
+			rte_panic("Cannot mmap memory for rte_config! error %i (%s)\n",

Quoted 8 lines

lib/librte_eal/linuxapp/eal/eal.c rte_eal_config_reattach(void) · line 268

Quoted 5 lines

lib/librte_eal/linuxapp/eal/eal.c +2 -2

+				  errno, strerror(errno));
+	}
 	close(mem_cfg_fd);
-	if (mem_config == MAP_FAILED || mem_config != rte_mem_cfg_addr)
-		rte_panic("Cannot mmap memory for rte_config\n");

NIT but any reason you moved the check before closing the file descriptor? (not that it matters with current code as we panic anyway)

"close()" may change "errno" according to its man page.

2016-10-03 08:55, Jean Tourrilhes:

Quoted 13 lines · Sergio Gonzalez Monroy

Quoted 12 lines

Hi Jean,

There are some format issues with the patch:

You can run scripts/check-git-log.sh to check them:
Wrong headline format:
eal: Fix misleading error messages, errno can't be trusted.
Wrong headline uppercase:
eal: Fix misleading error messages, errno can't be trusted.
Missing 'Fixes' tag:
eal: Fix misleading error messages, errno can't be trusted.

Quoted 3 lines

Quoted 1 line

The script's output highlights the different issues.

SOrry about that, I casually read the page on
http://dpdk.org/dev, but obviously I need to look at it again.

No problem. This guide is more oriented towards regular contributors. You come with a bug and its fix, we can make some effort to format the patch :)

The title could be "mem: fix hugepage mapping error messages"

Quoted 19 lines

Quoted 19 lines · Jean Tourrilhes

Quoted 6 lines

lib/librte_eal/linuxapp/eal/eal.c +2 -0

@@ -263,9 +264,16 @@ rte_eal_config_reattach(void)
  	mem_config = (struct rte_mem_config *) mmap(rte_mem_cfg_addr,
  			sizeof(*mem_config), PROT_READ | PROT_WRITE, MAP_SHARED,
  			mem_cfg_fd, 0);
+	if (mem_config == MAP_FAILED || mem_config != rte_mem_cfg_addr) {
+		if (mem_config != MAP_FAILED)

lib/librte_eal/linuxapp/eal/eal.c rte_eal_config_reattach(void) · line 268

Quoted 10 lines

lib/librte_eal/linuxapp/eal/eal.c +7 -2

+			/* errno is stale, don't use */
+			rte_panic("Cannot mmap memory for rte_config at [%p], got [%p] - please use '--base-virtaddr' option\n",
+				  rte_mem_cfg_addr, mem_config);
+		else
+			rte_panic("Cannot mmap memory for rte_config! error %i (%s)\n",
+				  errno, strerror(errno));
+	}
  	close(mem_cfg_fd);
-	if (mem_config == MAP_FAILED || mem_config != rte_mem_cfg_addr)
-		rte_panic("Cannot mmap memory for rte_config\n");

NIT but any reason you moved the check before closing the file descriptor? (not that it matters with current code as we panic anyway)

Quoted 1 line

"close()" may change "errno" according to its man page.

Sergio, do you have more comments?

Should we wait another version or is it OK?
Maybe you'd prefer to rework it yourself?

Quoted 22 lines

2016-10-03 08:55, Jean Tourrilhes:

Quoted 13 lines · Sergio Gonzalez Monroy

Quoted 12 lines

Hi Jean,

There are some format issues with the patch:

You can run scripts/check-git-log.sh to check them:
Wrong headline format:
eal: Fix misleading error messages, errno can't be trusted.
Wrong headline uppercase:
eal: Fix misleading error messages, errno can't be trusted.
Missing 'Fixes' tag:
eal: Fix misleading error messages, errno can't be trusted.

Quoted 3 lines

Quoted 1 line

The script's output highlights the different issues.

SOrry about that, I casually read the page on
http://dpdk.org/dev, but obviously I need to look at it again.

No problem. This guide is more oriented towards regular contributors. You come with a bug and its fix, we can make some effort to format the patch :)

The title could be "mem: fix hugepage mapping error messages"

Quoted 1 line

Sergio, do you have more comments?

Nop.

Quoted 2 lines

Should we wait another version or is it OK?
Maybe you'd prefer to rework it yourself?

AFAICS it really is just the commit message formatting, so maybe you can fix that when applying it?

Running secondary is tricky due to the need to map the memory region at the right place in VM, which is whatever primary has chosen. If the base address for primary happens to by already mapped in the secondary, we will hit precisely these error messages (depending if we fail on the config region or the hugepages). This is why there is already a comment about ASLR.

The issue is that in most cases, remapping does not happen and "errno" is not changed and therefore stale. In our case, we got a "permission denied", which sent us down the wrong track. It's such a common error for secondary that I feel this error message should be unambiguous and helpful.
The call to close was also moved because close() may override errno.

Signed-off-by Jean Tourrilhes <[email protected]>

---
  lib/librte_eal/linuxapp/eal/eal.c        | 14 +++++++++++---
  lib/librte_eal/linuxapp/eal/eal_memory.c | 16 ++++++++++++----
  2 files changed, 23 insertions(+), 7 deletions(-)

Quoted 14 lines

Running secondary is tricky due to the need to map the memory region at the right place in VM, which is whatever primary has chosen. If the base address for primary happens to by already mapped in the secondary, we will hit precisely these error messages (depending if we fail on the config region or the hugepages). This is why there is already a comment about ASLR.

The issue is that in most cases, remapping does not happen and "errno" is not changed and therefore stale. In our case, we got a "permission denied", which sent us down the wrong track. It's such a common error for secondary that I feel this error message should be unambiguous and helpful.
The call to close was also moved because close() may override errno.

Quoted 5 lines

Signed-off-by Jean Tourrilhes <[email protected]>

---
  lib/librte_eal/linuxapp/eal/eal.c        | 14 +++++++++++---
  lib/librte_eal/linuxapp/eal/eal_memory.c | 16 ++++++++++++----
  2 files changed, 23 insertions(+), 7 deletions(-)

Acked-by Sergio Gonzalez Monroy <[email protected]>

Applied, thanks
A rebase was necessary because of this patch: http://dpdk.org/commit/c00ae961
Please check everything is OK.

Hi Jean,

NIT but any reason you moved the check before closing the file descriptor? (not that it matters with current code as we panic anyway)

Thanks,
Sergio