[dpdk-dev] [PATCH v3] mem: command line option to delete hugepage backing files

[dpdk-dev] [PATCH v3] mem: command line option to delete hugepage backing files

[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 <shesha@cisco.com>
---
 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(+)

diff --git a/lib/librte_eal/common/eal_common_options.c
b/lib/librte_eal/common/eal_common_options.c
index 1f459ac..5fe6374 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -79,6 +79,7 @@ eal_long_options[] = {
 	{OPT_MASTER_LCORE,      1, NULL, OPT_MASTER_LCORE_NUM     },
 	{OPT_NO_HPET,           0, NULL, OPT_NO_HPET_NUM          },
 	{OPT_NO_HUGE,           0, NULL, OPT_NO_HUGE_NUM          },
+	{OPT_HUGE_UNLINK,       0, NULL, OPT_HUGE_UNLINK_NUM      },
 	{OPT_NO_PCI,            0, NULL, OPT_NO_PCI_NUM           },
 	{OPT_NO_SHCONF,         0, NULL, OPT_NO_SHCONF_NUM        },
 	{OPT_PCI_BLACKLIST,     1, NULL, OPT_PCI_BLACKLIST_NUM    },
@@ -722,6 +723,10 @@ eal_parse_common_option(int opt, const char *optarg,
 		conf->no_hugetlbfs = 1;
 		break;
 
+	case OPT_HUGE_UNLINK_NUM:
+		conf->hugepage_unlink = 1;
+		break;
+
 	case OPT_NO_PCI_NUM:
 		conf->no_pci = 1;
 		break;
@@ -856,6 +861,12 @@ eal_check_common_options(struct internal_config
*internal_cfg)
 		return -1;
 	}
 
+	if (internal_cfg->no_hugetlbfs && internal_cfg->hugepage_unlink) {
+		RTE_LOG(ERR, EAL, "Option --"OPT_HUGE_UNLINK" cannot "
+			"be specified together with --"OPT_NO_HUGE"\n");
+		return -1;
+	}
+
 	if (rte_eal_devargs_type_count(RTE_DEVTYPE_WHITELISTED_PCI) != 0 &&
 		rte_eal_devargs_type_count(RTE_DEVTYPE_BLACKLISTED_PCI) != 0) {
 		RTE_LOG(ERR, EAL, "Options blacklist (-b) and whitelist (-w) "
@@ -906,6 +917,7 @@ eal_common_usage(void)
 	       "  -h, --help          This help\n"
 	       "\nEAL options for DEBUG use only:\n"
 	       "  --"OPT_NO_HUGE"           Use malloc instead of hugetlbfs\n"
+	       "  --"OPT_HUGE_UNLINK"       Unlink hugepage backing file after
initalization\n"
 	       "  --"OPT_NO_PCI"            Disable PCI\n"
 	       "  --"OPT_NO_HPET"           Disable HPET\n"
 	       "  --"OPT_NO_SHCONF"         No shared config (mmap'd files)\n"
diff --git a/lib/librte_eal/common/eal_internal_cfg.h
b/lib/librte_eal/common/eal_internal_cfg.h
index e2ecb0d..84b075f 100644
--- a/lib/librte_eal/common/eal_internal_cfg.h
+++ b/lib/librte_eal/common/eal_internal_cfg.h
@@ -64,6 +64,7 @@ struct internal_config {
 	volatile unsigned force_nchannel; /**< force number of channels */
 	volatile unsigned force_nrank;    /**< force number of ranks */
 	volatile unsigned no_hugetlbfs;   /**< true to disable hugetlbfs */
+	volatile unsigned hugepage_unlink; /** < true to unlink backing files */
 	volatile unsigned xen_dom0_support; /**< support app running on Xen
Dom0*/
 	volatile unsigned no_pci;         /**< true to disable PCI */
 	volatile unsigned no_hpet;        /**< true to disable HPET */
diff --git a/lib/librte_eal/common/eal_options.h
b/lib/librte_eal/common/eal_options.h
index f6714d9..745f38c 100644
--- a/lib/librte_eal/common/eal_options.h
+++ b/lib/librte_eal/common/eal_options.h
@@ -63,6 +63,8 @@ enum {
 	OPT_PROC_TYPE_NUM,
 #define OPT_NO_HPET           "no-hpet"
 	OPT_NO_HPET_NUM,
+#define OPT_HUGE_UNLINK        "huge-unlink"
+	OPT_HUGE_UNLINK_NUM,
 #define OPT_NO_HUGE           "no-huge"
 	OPT_NO_HUGE_NUM,
 #define OPT_NO_PCI            "no-pci"
diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c
b/lib/librte_eal/linuxapp/eal/eal_memory.c
index ac2745e..c7e2485 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memory.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
@@ -786,6 +786,28 @@ copy_hugepages_to_shared_mem(struct hugepage_file *
dst, int dest_size,
 	return 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.
@@ -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;
+	}
+
 	/* free the temporary hugepage table */
 	free(tmp_hp);
 	tmp_hp = NULL;
-- 
1.9.1

Re: [dpdk-dev] [PATCH v3] mem: command line option to delete hugepage backing files

[Bruce Richardson]

On Wed, Oct 21, 2015 at 04:22:45PM +0000, shesha Sreenivasamurthy (shesha) wrote:
> 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 <shesha@cisco.com>
> ---
>  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>  
> +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.
> @@ -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)

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

Re: [dpdk-dev] [PATCH v3] mem: command line option to delete hugepage backing files

[Sergio Gonzalez Monroy]

On 21/10/2015 17:34, Bruce Richardson wrote:
> On Wed, Oct 21, 2015 at 04:22:45PM +0000, shesha Sreenivasamurthy (shesha) wrote:
>> 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 <shesha@cisco.com>
>> ---
>>   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>
>> +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.
>> @@ -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)
>
> 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

Re: [dpdk-dev] [PATCH v3] mem: command line option to delete hugepage backing files

[Bruce Richardson]

On Thu, Oct 22, 2015 at 09:51:28AM +0100, Sergio Gonzalez Monroy wrote:
> On 21/10/2015 17:34, Bruce Richardson wrote:
> >On Wed, Oct 21, 2015 at 04:22:45PM +0000, shesha Sreenivasamurthy (shesha) wrote:
> >>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 <shesha@cisco.com>
> >>---
> >>  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>
> >>+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.
> >>@@ -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)
> >
> >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
> 
I don't mind much where the functionality is done - I mostly care about reducing
the code diff. I'd prefer the unlink to be done either with the mmap of unmap, but
I won't lose any sleep if everyone else prefers a separate function for it.

/Bruce

Re: [dpdk-dev] [PATCH v3] mem: command line option to delete hugepage backing files

[shesha Sreenivasamurthy (shesha)]

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 <sergio.gonzalez.monroy@intel.com>
Date: Thursday, October 22, 2015 at 1:51 AM
To: Cisco Employee <shesha@cisco.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, Bruce Richardson
<bruce.richardson@intel.com>
Subject: Re: [dpdk-dev] [PATCH v3] mem: command line option to delete
hugepage backing files

On 21/10/2015 17:34, Bruce Richardson wrote:
> On Wed, Oct 21, 2015 at 04:22:45PM +0000, shesha Sreenivasamurthy
>(shesha) wrote:
>> 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 <shesha@cisco.com>
>> ---
>>   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>
>> +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.
>> @@ -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)
>
> 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

Re: [dpdk-dev] [PATCH v3] mem: command line option to delete hugepage backing files

[Sergio Gonzalez Monroy]

On 22/10/2015 17:03, shesha Sreenivasamurthy (shesha) wrote:
> 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) ?
The issue with unmap_unneeded_hugepages happens regardless of 
SINGLE_FILE_SEGMENT
being set or not.
The problem is that in that function, it assumes that no file has been 
unlinked.
In fact, with SINGLE_FILE_SEGMENT, it might re-open files again if it 
needs to truncate it,
so we need those files present in the file system.
> However, if you think having a separate function is better, I am all for
> it.
My initial thought was the same and to do the unlinking inside an 
existing function,
but as you may realized, the code is not the most straight forward, and 
the resulting
diff may be even bigger than with a single function.

I think the single function in v3 works properly because we only unlink 
the files left after
we have done all this mapping-unmapping.

Sergio
> --
> - Thanks
> char * (*shesha) (uint64_t cache, uint8_t F00D)
> { return 0x0000C0DE; }
>
>
> -----Original Message-----
> From: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>
> Date: Thursday, October 22, 2015 at 1:51 AM
> To: Cisco Employee <shesha@cisco.com>
> Cc: "dev@dpdk.org" <dev@dpdk.org>, Bruce Richardson
> <bruce.richardson@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v3] mem: command line option to delete
> hugepage backing files
>
> On 21/10/2015 17:34, Bruce Richardson wrote:
>> On Wed, Oct 21, 2015 at 04:22:45PM +0000, shesha Sreenivasamurthy
>> (shesha) wrote:
>>> 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 <shesha@cisco.com>
>>> ---
>>>    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>
>>> +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.
>>> @@ -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)
>>
>> 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
>
>

Re: [dpdk-dev] [PATCH v3] mem: command line option to delete hugepage backing files

[shesha Sreenivasamurthy (shesha)]

Understood and thanks for the clarification. Should I have to re-send
patch v3 or are we good here ?

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


-----Original Message-----
From: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>
Date: Friday, October 23, 2015 at 2:57 AM
To: Cisco Employee <shesha@cisco.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, Bruce Richardson
<bruce.richardson@intel.com>
Subject: Re: [dpdk-dev] [PATCH v3] mem: command line option to delete
hugepage backing files

On 22/10/2015 17:03, shesha Sreenivasamurthy (shesha) wrote:
> 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) ?
The issue with unmap_unneeded_hugepages happens regardless of
SINGLE_FILE_SEGMENT
being set or not.
The problem is that in that function, it assumes that no file has been
unlinked.
In fact, with SINGLE_FILE_SEGMENT, it might re-open files again if it
needs to truncate it,
so we need those files present in the file system.
> However, if you think having a separate function is better, I am all for
> it.
My initial thought was the same and to do the unlinking inside an
existing function,
but as you may realized, the code is not the most straight forward, and
the resulting
diff may be even bigger than with a single function.

I think the single function in v3 works properly because we only unlink
the files left after
we have done all this mapping-unmapping.

Sergio
> --
> - Thanks
> char * (*shesha) (uint64_t cache, uint8_t F00D)
> { return 0x0000C0DE; }
>
>
> -----Original Message-----
> From: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>
> Date: Thursday, October 22, 2015 at 1:51 AM
> To: Cisco Employee <shesha@cisco.com>
> Cc: "dev@dpdk.org" <dev@dpdk.org>, Bruce Richardson
> <bruce.richardson@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v3] mem: command line option to delete
> hugepage backing files
>
> On 21/10/2015 17:34, Bruce Richardson wrote:
>> On Wed, Oct 21, 2015 at 04:22:45PM +0000, shesha Sreenivasamurthy
>> (shesha) wrote:
>>> 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 <shesha@cisco.com>
>>> ---
>>>    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>
>>> +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.
>>> @@ -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)
>>
>> 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
>
>

Re: [dpdk-dev] [PATCH v3] mem: command line option to delete hugepage backing files

[Sergio Gonzalez Monroy]

On 21/10/2015 17:22, shesha Sreenivasamurthy (shesha) wrote:
> 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 <shesha@cisco.com>
> ---
>   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 <sergio.gonzalez.monroy@intel.com>

Re: [dpdk-dev] [PATCH v3] mem: command line option to delete hugepage backing files

[Sergio Gonzalez Monroy]

On 27/10/2015 11:42, Sergio Gonzalez Monroy wrote:
> On 21/10/2015 17:22, shesha Sreenivasamurthy (shesha) wrote:
>> 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 <shesha@cisco.com>
>> ---
>>   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 <sergio.gonzalez.monroy@intel.com>
Hi Shesha,

Even though the test-report says the patch is good, it is not. For some
reason it is wrapping some lines (76, 93, 106, 129) causing the errors.

You can add my Acked-by when you resend the patch.

Sergio