mem: command line option to delete hugepage backing files

When an application using huge-pages crash or exists, the hugetlbfs backing files are not cleaned up. This is a patch to clean those files. There are multi-process DPDK applications that may be benefited by those backing files. Therefore, I have made that configurable so that the application that does not need those backing files can remove them, thus not changing the current default behavior. The application itself can clean it up, however the rationale behind DPDK cleaning it up is, DPDK created it and therefore, it is better it unlinks it.

Signed-off-by Shesha Sreenivasamurthy <[email protected]>

---
 lib/librte_eal/common/eal_common_options.c | 12 ++++++++++++
 lib/librte_eal/common/eal_internal_cfg.h   |  1 +
 lib/librte_eal/common/eal_options.h        |  2 ++
 lib/librte_eal/linuxapp/eal/eal_memory.c   | 30
++++++++++++++++++++++++++++++
 4 files changed, 45 insertions(+)

(quoted diff) +22 -0

+static int
+unlink_hugepage_files(struct hugepage_file *hugepg_tbl,
+		unsigned num_hp_info)
+{
+	unsigned socket, size;
+	int page, nrpages = 0;
+
+	/* get total number of hugepages */
+	for (size = 0; size < num_hp_info; size++)
+		for (socket = 0; socket < RTE_MAX_NUMA_NODES; socket++)
+			nrpages += internal_config.hugepage_info[size].num_pages[socket];
+
+	for (page = 0; page < nrpages; page++) {
+		struct hugepage_file *hp = &hugepg_tbl[page];
+		if (hp->final_va != NULL && unlink(hp->filepath)) {
+			RTE_LOG(WARNING, EAL, "%s(): Removing %s failed: %s\n",
+				__func__, hp->filepath, strerror(errno));
+		}
+	}
+	return 0;
+}
+
 /*
  * unmaps hugepages that are not going to be used. since we originally

allocate
* ALL hugepages (not just those we need), additional unmapping needs to
be done.

(quoted diff) +0 -0

@@ -1289,6 +1311,14 @@ rte_eal_hugepage_init(void)
 		goto fail;

Quoted 18 lines · shesha Sreenivasamurthy (shesha)

When an application using huge-pages crash or exists, the hugetlbfs backing files are not cleaned up. This is a patch to clean those files. There are multi-process DPDK applications that may be benefited by those backing files. Therefore, I have made that configurable so that the application that does not need those backing files can remove them, thus not changing the current default behavior. The application itself can clean it up, however the rationale behind DPDK cleaning it up is, DPDK created it and therefore, it is better it unlinks it.

Signed-off-by Shesha Sreenivasamurthy <[email protected]>

---
  lib/librte_eal/common/eal_common_options.c | 12 ++++++++++++
  lib/librte_eal/common/eal_internal_cfg.h   |  1 +
  lib/librte_eal/common/eal_options.h        |  2 ++
  lib/librte_eal/linuxapp/eal/eal_memory.c   | 30
++++++++++++++++++++++++++++++
  4 files changed, 45 insertions(+)

<snip>

Quoted 29 lines

(quoted diff) +22 -0

+static int
+unlink_hugepage_files(struct hugepage_file *hugepg_tbl,
+		unsigned num_hp_info)
+{
+	unsigned socket, size;
+	int page, nrpages = 0;
+
+	/* get total number of hugepages */
+	for (size = 0; size < num_hp_info; size++)
+		for (socket = 0; socket < RTE_MAX_NUMA_NODES; socket++)
+			nrpages += internal_config.hugepage_info[size].num_pages[socket];
+
+	for (page = 0; page < nrpages; page++) {
+		struct hugepage_file *hp = &hugepg_tbl[page];
+		if (hp->final_va != NULL && unlink(hp->filepath)) {
+			RTE_LOG(WARNING, EAL, "%s(): Removing %s failed: %s\n",
+				__func__, hp->filepath, strerror(errno));
+		}
+	}
+	return 0;
+}
+
  /*
   * unmaps hugepages that are not going to be used. since we originally

allocate
* ALL hugepages (not just those we need), additional unmapping needs to
be done.

(quoted diff) +0 -0

@@ -1289,6 +1311,14 @@ rte_eal_hugepage_init(void)
  		goto fail;

Review rte_eal_hugepage_init(void) · line 1321

Quoted 10 lines

(quoted diff) +8 -0

  	}
 
+	/* free the hugepage backing files */
+	if (internal_config.hugepage_unlink &&
+		unlink_hugepage_files(tmp_hp,
+			internal_config.num_hugepage_sizes) < 0) {
+			RTE_LOG(ERR, EAL, "Unlinking hugepage backing files failed!\n");
+		goto fail;
+	}
+

Sorry for the late comment, but...

Rather than adding a whole new function to be called here, can the same effect not be got by adding in 2/3 lines like:
if (internal_config.hugepage_unlink)
unlink(hugetlb[i].filepath)

at line 409 of eal_memory.c where were have done our final mmap of the file. [You also need the same couple of lines for the 32-bit special case at line 351]. It would be a shorter diff.

/Bruce

Quoted 65 lines · Bruce Richardson

Quoted 18 lines · shesha Sreenivasamurthy (shesha)

When an application using huge-pages crash or exists, the hugetlbfs backing files are not cleaned up. This is a patch to clean those files. There are multi-process DPDK applications that may be benefited by those backing files. Therefore, I have made that configurable so that the application that does not need those backing files can remove them, thus not changing the current default behavior. The application itself can clean it up, however the rationale behind DPDK cleaning it up is, DPDK created it and therefore, it is better it unlinks it.

Signed-off-by Shesha Sreenivasamurthy <[email protected]>

---
 lib/librte_eal/common/eal_common_options.c | 12 ++++++++++++
 lib/librte_eal/common/eal_internal_cfg.h   |  1 +
 lib/librte_eal/common/eal_options.h        |  2 ++
 lib/librte_eal/linuxapp/eal/eal_memory.c   | 30
++++++++++++++++++++++++++++++
 4 files changed, 45 insertions(+)

<snip>

Quoted 28 lines

(quoted diff) +22 -0

+static int
+unlink_hugepage_files(struct hugepage_file *hugepg_tbl,
+		unsigned num_hp_info)
+{
+	unsigned socket, size;
+	int page, nrpages = 0;
+
+	/* get total number of hugepages */
+	for (size = 0; size < num_hp_info; size++)
+		for (socket = 0; socket < RTE_MAX_NUMA_NODES; socket++)
+			nrpages += internal_config.hugepage_info[size].num_pages[socket];
+
+	for (page = 0; page < nrpages; page++) {
+		struct hugepage_file *hp = &hugepg_tbl[page];
+		if (hp->final_va != NULL && unlink(hp->filepath)) {
+			RTE_LOG(WARNING, EAL, "%s(): Removing %s failed: %s\n",
+				__func__, hp->filepath, strerror(errno));
+		}
+	}
+	return 0;
+}
+
 /*
  * unmaps hugepages that are not going to be used. since we originally

allocate
* ALL hugepages (not just those we need), additional unmapping needs to
be done.

Review rte_eal_hugepage_init(void) · line 1320

Quoted 10 lines

(quoted diff) +8 -0

@@ -1289,6 +1311,14 @@ rte_eal_hugepage_init(void)
 		goto fail;
 	}
+	/* free the hugepage backing files */
+	if (internal_config.hugepage_unlink &&
+		unlink_hugepage_files(tmp_hp,
+			internal_config.num_hugepage_sizes) < 0) {
+			RTE_LOG(ERR, EAL, "Unlinking hugepage backing files failed!\n");
+		goto fail;
+	}
+

Sorry for the late comment, but...

Rather than adding a whole new function to be called here, can the same effect not be got by adding in 2/3 lines like:
if (internal_config.hugepage_unlink)
unlink(hugetlb[i].filepath)

Quoted 5 lines

at line 409 of eal_memory.c where were have done our final mmap of the file. [You also need the same couple of lines for the 32-bit special case at line 351]. It would be a shorter diff.

/Bruce

If you wanted to avoid the extra function call, I might be cleaner to just unlink all files when
doing unmap_all_hugepages_orig.
My two cents: I think it would be easier to read/debug having a function that "unlinks files" instead
of unlinking files at different points in map_all_hugepages.

Unfortunately the proposed approach does not work for all cases:
- If we have single file segment, map_all_hugepages does not get call a second time, instead we call
remap_all_hugepages
- If we use options -m or --socket-mem, because unmap_unneeded_hugepages does not expect files
already unlinked, it will fail when trying to unlink unneeded hugepage
files.

The current patch would work as we only unlink after unmap_unneeded_hugepages.

Sergio

Quoted 18 lines · shesha Sreenivasamurthy (shesha)

When an application using huge-pages crash or exists, the hugetlbfs backing files are not cleaned up. This is a patch to clean those files. There are multi-process DPDK applications that may be benefited by those backing files. Therefore, I have made that configurable so that the application that does not need those backing files can remove them, thus not changing the current default behavior. The application itself can clean it up, however the rationale behind DPDK cleaning it up is, DPDK created it and therefore, it is better it unlinks it.

Signed-off-by Shesha Sreenivasamurthy <[email protected]>

---
  lib/librte_eal/common/eal_common_options.c | 12 ++++++++++++
  lib/librte_eal/common/eal_internal_cfg.h   |  1 +
  lib/librte_eal/common/eal_options.h        |  2 ++
  lib/librte_eal/linuxapp/eal/eal_memory.c   | 30
++++++++++++++++++++++++++++++
  4 files changed, 45 insertions(+)

<snip>

Quoted 30 lines

(quoted diff) +22 -0

+static int
+unlink_hugepage_files(struct hugepage_file *hugepg_tbl,
+		unsigned num_hp_info)
+{
+	unsigned socket, size;
+	int page, nrpages = 0;
+
+	/* get total number of hugepages */
+	for (size = 0; size < num_hp_info; size++)
+		for (socket = 0; socket < RTE_MAX_NUMA_NODES; socket++)
+			nrpages += internal_config.hugepage_info[size].num_pages[socket];
+
+	for (page = 0; page < nrpages; page++) {
+		struct hugepage_file *hp = &hugepg_tbl[page];
+		if (hp->final_va != NULL && unlink(hp->filepath)) {
+			RTE_LOG(WARNING, EAL, "%s(): Removing %s failed: %s\n",
+				__func__, hp->filepath, strerror(errno));
+		}
+	}
+	return 0;
+}
+
  /*
   * unmaps hugepages that are not going to be used. since we originally

allocate
* ALL hugepages (not just those we need), additional unmapping needs
to
be done.

(quoted diff) +0 -0

@@ -1289,6 +1311,14 @@ rte_eal_hugepage_init(void)
  		goto fail;

Review rte_eal_hugepage_init(void) · line 1321

Quoted 10 lines

(quoted diff) +8 -0

  	}
 
+	/* free the hugepage backing files */
+	if (internal_config.hugepage_unlink &&
+		unlink_hugepage_files(tmp_hp,
+			internal_config.num_hugepage_sizes) < 0) {
+			RTE_LOG(ERR, EAL, "Unlinking hugepage backing files failed!\n");
+		goto fail;
+	}
+

Sorry for the late comment, but...

Rather than adding a whole new function to be called here, can the same effect
not be got by adding in 2/3 lines like:
if (internal_config.hugepage_unlink)
unlink(hugetlb[i].filepath)

at line 409 of eal_memory.c where were have done our final mmap of the file.
[You also need the same couple of lines for the 32-bit special case at line 351].
It would be a shorter diff.

/Bruce

Sergio,
Your comment regarding remap_all_functions is correct and can be fixed
by unlinking in remap_all_hugepages() too. However, regarding you comment that ³unmap_unneeded_hugepages² will fail ‹ in the unmap_unneeded_hugepages() we do not unlink if final_va is equal to NULL guarded by RTE_EAL_SINGLE_FILE_SEGMENTS. My testing did not catch as RTE_EAL_SINGLE_FILE_SEGMENTS was set. Is there any reason why we should not skip unlinking if final_va is null always (removing ifdef RTE_EAL_SINGLE_FILE_SEGMENTS) ?

However, if you think having a separate function is better, I am all for it.

--
- Thanks
char * (*shesha) (uint64_t cache, uint8_t F00D)
{ return 0x0000C0DE; }

-----Original Message-----
From: Sergio Gonzalez Monroy <[email protected]>
Date: Thursday, October 22, 2015 at 1:51 AM
To: Cisco Employee <[email protected]>
Cc: "[email protected]" <[email protected]>, Bruce Richardson <[email protected]>
Subject: Re: [dpdk-dev] [PATCH v3] mem: command line option to delete hugepage backing files

Quoted 69 lines · Bruce Richardson

Quoted 18 lines · shesha Sreenivasamurthy (shesha)

When an application using huge-pages crash or exists, the hugetlbfs backing files are not cleaned up. This is a patch to clean those files. There are multi-process DPDK applications that may be benefited by those backing files. Therefore, I have made that configurable so that the application that does not need those backing files can remove them, thus not changing the current default behavior. The application itself can clean it up, however the rationale behind DPDK cleaning it up is, DPDK created it and therefore, it is better it unlinks it.

Signed-off-by Shesha Sreenivasamurthy <[email protected]>

---
   lib/librte_eal/common/eal_common_options.c | 12 ++++++++++++
   lib/librte_eal/common/eal_internal_cfg.h   |  1 +
   lib/librte_eal/common/eal_options.h        |  2 ++
   lib/librte_eal/linuxapp/eal/eal_memory.c   | 30
++++++++++++++++++++++++++++++
   4 files changed, 45 insertions(+)

<snip>

Quoted 30 lines

(quoted diff) +22 -0

+static int
+unlink_hugepage_files(struct hugepage_file *hugepg_tbl,
+		unsigned num_hp_info)
+{
+	unsigned socket, size;
+	int page, nrpages = 0;
+
+	/* get total number of hugepages */
+	for (size = 0; size < num_hp_info; size++)
+		for (socket = 0; socket < RTE_MAX_NUMA_NODES; socket++)
+			nrpages += internal_config.hugepage_info[size].num_pages[socket];
+
+	for (page = 0; page < nrpages; page++) {
+		struct hugepage_file *hp = &hugepg_tbl[page];
+		if (hp->final_va != NULL && unlink(hp->filepath)) {
+			RTE_LOG(WARNING, EAL, "%s(): Removing %s failed: %s\n",
+				__func__, hp->filepath, strerror(errno));
+		}
+	}
+	return 0;
+}
+
   /*
    * unmaps hugepages that are not going to be used. since we originally

allocate
* ALL hugepages (not just those we need), additional unmapping needs
to
be done.

(quoted diff) +0 -0

@@ -1289,6 +1311,14 @@ rte_eal_hugepage_init(void)
   		goto fail;

Review rte_eal_hugepage_init(void) · line 1321

Quoted 10 lines

(quoted diff) +8 -0

   	}
 
+	/* free the hugepage backing files */
+	if (internal_config.hugepage_unlink &&
+		unlink_hugepage_files(tmp_hp,
+			internal_config.num_hugepage_sizes) < 0) {
+			RTE_LOG(ERR, EAL, "Unlinking hugepage backing files failed!\n");
+		goto fail;
+	}
+

Sorry for the late comment, but...

Rather than adding a whole new function to be called here, can the same effect
not be got by adding in 2/3 lines like:
if (internal_config.hugepage_unlink)
unlink(hugetlb[i].filepath)

Quoted 7 lines

at line 409 of eal_memory.c where were have done our final mmap of the file.
[You also need the same couple of lines for the 32-bit special case at line 351].
It would be a shorter diff.

/Bruce

If you wanted to avoid the extra function call, I might be cleaner to just unlink all files when
doing unmap_all_hugepages_orig.
My two cents: I think it would be easier to read/debug having a function that "unlinks files" instead
of unlinking files at different points in map_all_hugepages.

Unfortunately the proposed approach does not work for all cases:
- If we have single file segment, map_all_hugepages does not get call a second time, instead we call
remap_all_hugepages
- If we use options -m or --socket-mem, because unmap_unneeded_hugepages does not expect files
already unlinked, it will fail when trying to unlink unneeded
hugepage files.

The current patch would work as we only unlink after unmap_unneeded_hugepages.

Sergio

Sergio,
Your comment regarding remap_all_functions is correct and can be fixed
by unlinking in remap_all_hugepages() too. However, regarding you comment that ³unmap_unneeded_hugepages² will fail ‹ in the unmap_unneeded_hugepages() we do not unlink if final_va is equal to NULL guarded by RTE_EAL_SINGLE_FILE_SEGMENTS. My testing did not catch as RTE_EAL_SINGLE_FILE_SEGMENTS was set. Is there any reason why we should not skip unlinking if final_va is null always (removing ifdef RTE_EAL_SINGLE_FILE_SEGMENTS) ?

However, if you think having a separate function is better, I am all for it.

--
- Thanks
char * (*shesha) (uint64_t cache, uint8_t F00D)
{ return 0x0000C0DE; }

-----Original Message-----
From: Sergio Gonzalez Monroy <[email protected]>
Date: Thursday, October 22, 2015 at 1:51 AM
To: Cisco Employee <[email protected]>
Cc: "[email protected]" <[email protected]>, Bruce Richardson <[email protected]>
Subject: Re: [dpdk-dev] [PATCH v3] mem: command line option to delete hugepage backing files

Quoted 73 lines · Bruce Richardson

Quoted 20 lines · shesha Sreenivasamurthy (shesha)

When an application using huge-pages crash or exists, the hugetlbfs backing files are not cleaned up. This is a patch to clean those files. There are multi-process DPDK applications that may be benefited by those
backing files. Therefore, I have made that configurable so that the application that does not need those backing files can remove them, thus
not changing the current default behavior. The application itself can clean it up, however the rationale behind DPDK cleaning it up is, DPDK created it and therefore, it is better it unlinks it.

Signed-off-by Shesha Sreenivasamurthy <[email protected]>

---
   lib/librte_eal/common/eal_common_options.c | 12 ++++++++++++
   lib/librte_eal/common/eal_internal_cfg.h   |  1 +
   lib/librte_eal/common/eal_options.h        |  2 ++
   lib/librte_eal/linuxapp/eal/eal_memory.c   | 30
++++++++++++++++++++++++++++++
   4 files changed, 45 insertions(+)

<snip>

Quoted 32 lines

(quoted diff) +22 -0

+static int
+unlink_hugepage_files(struct hugepage_file *hugepg_tbl,
+		unsigned num_hp_info)
+{
+	unsigned socket, size;
+	int page, nrpages = 0;
+
+	/* get total number of hugepages */
+	for (size = 0; size < num_hp_info; size++)
+		for (socket = 0; socket < RTE_MAX_NUMA_NODES; socket++)
+			nrpages += internal_config.hugepage_info[size].num_pages[socket];
+
+	for (page = 0; page < nrpages; page++) {
+		struct hugepage_file *hp = &hugepg_tbl[page];
+		if (hp->final_va != NULL && unlink(hp->filepath)) {
+			RTE_LOG(WARNING, EAL, "%s(): Removing %s failed: %s\n",
+				__func__, hp->filepath, strerror(errno));
+		}
+	}
+	return 0;
+}
+
   /*
    * unmaps hugepages that are not going to be used. since we

originally
allocate
* ALL hugepages (not just those we need), additional unmapping
needs
to
be done.

(quoted diff) +0 -0

@@ -1289,6 +1311,14 @@ rte_eal_hugepage_init(void)
   		goto fail;

Review rte_eal_hugepage_init(void) · line 1321

Quoted 10 lines

(quoted diff) +8 -0

   	}
 
+	/* free the hugepage backing files */
+	if (internal_config.hugepage_unlink &&
+		unlink_hugepage_files(tmp_hp,
+			internal_config.num_hugepage_sizes) < 0) {
+			RTE_LOG(ERR, EAL, "Unlinking hugepage backing files failed!\n");
+		goto fail;
+	}
+

Sorry for the late comment, but...

Rather than adding a whole new function to be called here, can the same effect
not be got by adding in 2/3 lines like:
if (internal_config.hugepage_unlink)
unlink(hugetlb[i].filepath)

Quoted 7 lines

at line 409 of eal_memory.c where were have done our final mmap of the file.
[You also need the same couple of lines for the 32-bit special case at line 351].
It would be a shorter diff.

/Bruce

If you wanted to avoid the extra function call, I might be cleaner to just unlink all files when
doing unmap_all_hugepages_orig.
My two cents: I think it would be easier to read/debug having a function that "unlinks files" instead
of unlinking files at different points in map_all_hugepages.

Unfortunately the proposed approach does not work for all cases:
- If we have single file segment, map_all_hugepages does not get call a second time, instead we call
remap_all_hugepages
- If we use options -m or --socket-mem, because unmap_unneeded_hugepages does not expect files
already unlinked, it will fail when trying to unlink unneeded
hugepage files.

The current patch would work as we only unlink after unmap_unneeded_hugepages.

Sergio

When an application using huge-pages crash or exists, the hugetlbfs backing files are not cleaned up. This is a patch to clean those files. There are multi-process DPDK applications that may be benefited by those backing files. Therefore, I have made that configurable so that the application that does not need those backing files can remove them, thus not changing the current default behavior. The application itself can clean it up, however the rationale behind DPDK cleaning it up is, DPDK created it and therefore, it is better it unlinks it.

Signed-off-by Shesha Sreenivasamurthy <[email protected]>

---
  lib/librte_eal/common/eal_common_options.c | 12 ++++++++++++
  lib/librte_eal/common/eal_internal_cfg.h   |  1 +
  lib/librte_eal/common/eal_options.h        |  2 ++
  lib/librte_eal/linuxapp/eal/eal_memory.c   | 30
++++++++++++++++++++++++++++++
  4 files changed, 45 insertions(+)

Quoted 9 lines · shesha Sreenivasamurthy (shesha)

When an application using huge-pages crash or exists, the hugetlbfs backing files are not cleaned up. This is a patch to clean those files. There are multi-process DPDK applications that may be benefited by those backing files. Therefore, I have made that configurable so that the application that does not need those backing files can remove them, thus not changing the current default behavior. The application itself can clean it up, however the rationale behind DPDK cleaning it up is, DPDK created it and therefore, it is better it unlinks it.

Quoted 9 lines

Signed-off-by Shesha Sreenivasamurthy <[email protected]>

---
  lib/librte_eal/common/eal_common_options.c | 12 ++++++++++++
  lib/librte_eal/common/eal_internal_cfg.h   |  1 +
  lib/librte_eal/common/eal_options.h        |  2 ++
  lib/librte_eal/linuxapp/eal/eal_memory.c   | 30
++++++++++++++++++++++++++++++
  4 files changed, 45 insertions(+)

You need to update patchwork to reflect that v4 is rejected and set v3 with 'New' state.

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