[dpdk-dev,v3] 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 <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(+)

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;
  

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
  

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
  

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
  

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
  

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

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

>

>
  

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>
  

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