[dpdk-dev] [PATCH] linuxapp eal: set fd to -1 for MAP_ANONYMOUS cases

[dpdk-dev] [PATCH] linuxapp eal: set fd to -1 for MAP_ANONYMOUS cases

[Neil Horman]

https://dpdk.org/tracker/show_bug.cgi?id=18

Indicated that several mmap call sites in the linuxapp eal code used an
fd that was not -1 in their calls while using MAP_ANONYMOUS.  While
probably not a huge deal, the man page does say the fd should be -1 for
portability, as some implementations don't ignore fd as they should for
MAP_ANONYMOUS, and seting -1 here allows us to eliminate some code.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: Thomas Monjalon <thomas@monjalon.net>
CC: Ferruh Yigit <ferruh.yigit@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_memory.c | 32 +++++++-------------------------
 1 file changed, 7 insertions(+), 25 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c
index 38853b753..acb553243 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memory.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
@@ -222,7 +222,7 @@ aslr_enabled(void)
 }
 
 /*
- * Try to mmap *size bytes in /dev/zero. If it is successful, return the
+ * Try to mmap *size bytes. If it is successful, return the
  * pointer to the mmap'd area and keep *size unmodified. Else, retry
  * with a smaller zone: decrease *size by hugepage_sz until it reaches
  * 0. In this case, return NULL. Note: this function returns an address
@@ -233,7 +233,6 @@ get_virtual_area(size_t *size, size_t hugepage_sz)
 {
 	void *addr;
 	void *addr_hint;
-	int fd;
 	long aligned_addr;
 
 	if (internal_config.base_virtaddr != 0) {
@@ -248,11 +247,6 @@ get_virtual_area(size_t *size, size_t hugepage_sz)
 	RTE_LOG(DEBUG, EAL, "Ask a virtual area of 0x%zx bytes\n", *size);
 
 
-	fd = open("/dev/zero", O_RDONLY);
-	if (fd < 0){
-		RTE_LOG(ERR, EAL, "Cannot open /dev/zero\n");
-		return NULL;
-	}
 	do {
 		addr = mmap(addr_hint, (*size) + hugepage_sz, PROT_READ,
 #ifdef RTE_ARCH_PPC_64
@@ -260,7 +254,7 @@ get_virtual_area(size_t *size, size_t hugepage_sz)
 #else
 				MAP_PRIVATE,
 #endif
-				fd, 0);
+				-1, 0);
 		if (addr == MAP_FAILED) {
 			*size -= hugepage_sz;
 		} else if (addr_hint != NULL && addr != addr_hint) {
@@ -273,14 +267,12 @@ get_virtual_area(size_t *size, size_t hugepage_sz)
 	} while (addr == MAP_FAILED && *size > 0);
 
 	if (addr == MAP_FAILED) {
-		close(fd);
 		RTE_LOG(ERR, EAL, "Cannot get a virtual area: %s\n",
 			strerror(errno));
 		return NULL;
 	}
 
 	munmap(addr, (*size) + hugepage_sz);
-	close(fd);
 
 	/* align addr to a huge page size boundary */
 	aligned_addr = (long)addr;
@@ -1011,7 +1003,7 @@ rte_eal_hugepage_init(void)
 	/* hugetlbfs can be disabled */
 	if (internal_config.no_hugetlbfs) {
 		addr = mmap(NULL, internal_config.memory, PROT_READ | PROT_WRITE,
-				MAP_PRIVATE | MAP_ANONYMOUS, 0, 0);
+				MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
 		if (addr == MAP_FAILED) {
 			RTE_LOG(ERR, EAL, "%s: mmap() failed: %s\n", __func__,
 					strerror(errno));
@@ -1339,7 +1331,7 @@ rte_eal_hugepage_attach(void)
 	unsigned i, s = 0; /* s used to track the segment number */
 	unsigned max_seg = RTE_MAX_MEMSEG;
 	off_t size = 0;
-	int fd, fd_zero = -1, fd_hugepage = -1;
+	int fd, fd_hugepage = -1;
 
 	if (aslr_enabled() > 0) {
 		RTE_LOG(WARNING, EAL, "WARNING: Address Space Layout Randomization "
@@ -1350,11 +1342,6 @@ rte_eal_hugepage_attach(void)
 
 	test_phys_addrs_available();
 
-	fd_zero = open("/dev/zero", O_RDONLY);
-	if (fd_zero < 0) {
-		RTE_LOG(ERR, EAL, "Could not open /dev/zero\n");
-		goto error;
-	}
 	fd_hugepage = open(eal_hugepage_info_path(), O_RDONLY);
 	if (fd_hugepage < 0) {
 		RTE_LOG(ERR, EAL, "Could not open %s\n", eal_hugepage_info_path());
@@ -1373,8 +1360,6 @@ rte_eal_hugepage_attach(void)
 			break;
 
 		/*
-		 * fdzero is mmapped to get a contiguous block of virtual
-		 * addresses of the appropriate memseg size.
 		 * use mmap to get identical addresses as the primary process.
 		 */
 		base_addr = mmap(mcfg->memseg[s].addr, mcfg->memseg[s].len,
@@ -1384,21 +1369,21 @@ rte_eal_hugepage_attach(void)
 #else
 				 MAP_PRIVATE,
 #endif
-				 fd_zero, 0);
+				 -1, 0);
 		if (base_addr == MAP_FAILED ||
 		    base_addr != mcfg->memseg[s].addr) {
 			max_seg = s;
 			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] - "
+					"at [%p], got [%p] - "
 					"please use '--base-virtaddr' option\n",
 					(unsigned long long)mcfg->memseg[s].len,
 					mcfg->memseg[s].addr, base_addr);
 				munmap(base_addr, mcfg->memseg[s].len);
 			} else {
 				RTE_LOG(ERR, EAL, "Could not mmap %llu bytes "
-					"in /dev/zero at [%p]: '%s'\n",
+					"at [%p]: '%s'\n",
 					(unsigned long long)mcfg->memseg[s].len,
 					mcfg->memseg[s].addr, strerror(errno));
 			}
@@ -1465,7 +1450,6 @@ rte_eal_hugepage_attach(void)
 	}
 	/* unmap the hugepage config file, since we are done using it */
 	munmap(hp, size);
-	close(fd_zero);
 	close(fd_hugepage);
 	return 0;
 
@@ -1474,8 +1458,6 @@ rte_eal_hugepage_attach(void)
 		munmap(mcfg->memseg[i].addr, mcfg->memseg[i].len);
 	if (hp != NULL && hp != MAP_FAILED)
 		munmap(hp, size);
-	if (fd_zero >= 0)
-		close(fd_zero);
 	if (fd_hugepage >= 0)
 		close(fd_hugepage);
 	return -1;
-- 
2.14.3

Re: [dpdk-dev] [PATCH] linuxapp eal: set fd to -1 for MAP_ANONYMOUS cases

[Thomas Monjalon]

02/04/2018 14:06, Neil Horman:
> https://dpdk.org/tracker/show_bug.cgi?id=18

I really appreciate reading such bug request.
Maybe we should add a Suggested-by line
	Suggested-by: Solal Pirelli <solal.pirelli@gmail.com>
before the SoB line (chronological order).


> Indicated that several mmap call sites in the linuxapp eal code used an
> fd that was not -1 in their calls while using MAP_ANONYMOUS.  While
> probably not a huge deal, the man page does say the fd should be -1 for
> portability, as some implementations don't ignore fd as they should for
> MAP_ANONYMOUS, and seting -1 here allows us to eliminate some code.
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> CC: Thomas Monjalon <thomas@monjalon.net>
> CC: Ferruh Yigit <ferruh.yigit@intel.com>

Re: [dpdk-dev] [PATCH] linuxapp eal: set fd to -1 for MAP_ANONYMOUS cases

[Neil Horman]

On Mon, Apr 02, 2018 at 06:50:12PM +0200, Thomas Monjalon wrote:
> 02/04/2018 14:06, Neil Horman:
> > https://dpdk.org/tracker/show_bug.cgi?id=18
> 
> I really appreciate reading such bug request.
No worries. It was there, I had time :)

> Maybe we should add a Suggested-by line
> 	Suggested-by: Solal Pirelli <solal.pirelli@gmail.com>
> before the SoB line (chronological order).
> 
Philisophically, I'm not opposed, feel free to add it in during the commit.

That said, I'm not sure its overly needed, given that the bz is referenced, and
one can follow the documentation trail to the individual that opened the bug
report.

Neil

> 
> > Indicated that several mmap call sites in the linuxapp eal code used an
> > fd that was not -1 in their calls while using MAP_ANONYMOUS.  While
> > probably not a huge deal, the man page does say the fd should be -1 for
> > portability, as some implementations don't ignore fd as they should for
> > MAP_ANONYMOUS, and seting -1 here allows us to eliminate some code.
> > 
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > CC: Thomas Monjalon <thomas@monjalon.net>
> > CC: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> 
> 
> 

Re: [dpdk-dev] [PATCH] linuxapp eal: set fd to -1 for MAP_ANONYMOUS cases

[Thomas Monjalon]

02/04/2018 14:06, Neil Horman:
> https://dpdk.org/tracker/show_bug.cgi?id=18
> 
> Indicated that several mmap call sites in the linuxapp eal code used an
> fd that was not -1 in their calls while using MAP_ANONYMOUS.  While
> probably not a huge deal, the man page does say the fd should be -1 for
> portability, as some implementations don't ignore fd as they should for
> MAP_ANONYMOUS, and seting -1 here allows us to eliminate some code.
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> CC: Thomas Monjalon <thomas@monjalon.net>
> CC: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
>  lib/librte_eal/linuxapp/eal/eal_memory.c | 32 +++++++-------------------------
>  1 file changed, 7 insertions(+), 25 deletions(-)

This patch probably need a rebase on top of Anatoly's patches.

Anatoly, please, could you review?

[dpdk-dev] [PATCHv2] linuxapp eal: set fd to -1 for MAP_ANONYMOUS cases

[Neil Horman]

https://dpdk.org/tracker/show_bug.cgi?id=18

Indicated that several mmap call sites in the [linux|bsd]app eal code
set fd that was not -1 in their calls while using MAP_ANONYMOUS.  While
probably not a huge deal, the man page does say the fd should be -1 for
portability, as some implementations don't ignore fd as they should for
MAP_ANONYMOUS.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: Thomas Monjalon <thomas@monjalon.net>
CC: Ferruh Yigit <ferruh.yigit@intel.com>

---
Change notes

v2) Rebased to HEAD again to adjust for patches that landed ahead of
this
---
 lib/librte_eal/bsdapp/eal/eal_memory.c   | 2 +-
 lib/librte_eal/linuxapp/eal/eal_memory.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/bsdapp/eal/eal_memory.c b/lib/librte_eal/bsdapp/eal/eal_memory.c
index b27262c7e..a5e034789 100644
--- a/lib/librte_eal/bsdapp/eal/eal_memory.c
+++ b/lib/librte_eal/bsdapp/eal/eal_memory.c
@@ -70,7 +70,7 @@ rte_eal_hugepage_init(void)
 
 		addr = mmap(NULL, internal_config.memory,
 				PROT_READ | PROT_WRITE,
-				MAP_PRIVATE | MAP_ANONYMOUS, 0, 0);
+				MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
 		if (addr == MAP_FAILED) {
 			RTE_LOG(ERR, EAL, "%s: mmap() failed: %s\n", __func__,
 					strerror(errno));
diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c
index 7cdd3048e..b7a2e951d 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memory.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
@@ -1329,7 +1329,7 @@ eal_legacy_hugepage_init(void)
 		}
 
 		addr = mmap(NULL, internal_config.memory, PROT_READ | PROT_WRITE,
-				MAP_PRIVATE | MAP_ANONYMOUS, 0, 0);
+				MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
 		if (addr == MAP_FAILED) {
 			RTE_LOG(ERR, EAL, "%s: mmap() failed: %s\n", __func__,
 					strerror(errno));
-- 
2.14.3

Re: [dpdk-dev] [PATCHv2] linuxapp eal: set fd to -1 for MAP_ANONYMOUS cases

[Burakov, Anatoly]

On 12-Apr-18 12:16 PM, Neil Horman wrote:
> https://dpdk.org/tracker/show_bug.cgi?id=18
> 
> Indicated that several mmap call sites in the [linux|bsd]app eal code
> set fd that was not -1 in their calls while using MAP_ANONYMOUS.  While
> probably not a huge deal, the man page does say the fd should be -1 for
> portability, as some implementations don't ignore fd as they should for
> MAP_ANONYMOUS.
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> CC: Thomas Monjalon <thomas@monjalon.net>
> CC: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> ---
> Change notes
> 
> v2) Rebased to HEAD again to adjust for patches that landed ahead of
> this
> ---

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCHv2] linuxapp eal: set fd to -1 for MAP_ANONYMOUS cases

[Thomas Monjalon]

12/04/2018 14:05, Burakov, Anatoly:
> On 12-Apr-18 12:16 PM, Neil Horman wrote:
> > https://dpdk.org/tracker/show_bug.cgi?id=18
> > 
> > Indicated that several mmap call sites in the [linux|bsd]app eal code
> > set fd that was not -1 in their calls while using MAP_ANONYMOUS.  While
> > probably not a huge deal, the man page does say the fd should be -1 for
> > portability, as some implementations don't ignore fd as they should for
> > MAP_ANONYMOUS.
> > 
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > CC: Thomas Monjalon <thomas@monjalon.net>
> > CC: Ferruh Yigit <ferruh.yigit@intel.com>
> > 
> > ---
> > Change notes
> > 
> > v2) Rebased to HEAD again to adjust for patches that landed ahead of
> > this
> > ---
> 
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

Applied, thanks