* [BUG?] shmem: memory leak on NO-MMU arch
@ 2011-03-08 9:17 Bob Liu
2011-03-20 20:35 ` Hugh Dickins
0 siblings, 1 reply; 5+ messages in thread
From: Bob Liu @ 2011-03-08 9:17 UTC (permalink / raw)
To: linux-mm; +Cc: viro, hch, hughd, npiggin, tj, Bob Liu
Hi, folks
I got a problem about shmem on NO-MMU arch, it seems memory leak
happened.
A simple test file is like this:
=========
#include <stdio.h>
#include <stdlib.h>
#include <sys/types.h>
#include <sys/ipc.h>
#include <sys/shm.h>
#include <errno.h>
#include <string.h>
int main(void)
{
int i;
key_t k = ftok("/etc", 42);
for ( i=0; i<2; ++i) {
int id = shmget(k, 10000, 0644|IPC_CREAT);
if (id == -1) {
printf("shmget error\n");
}
if(shmctl(id, IPC_RMID, NULL ) == -1) {
printf("shm rm error\n");
return -1;
}
}
printf("run ok...\n");
return 0;
}
The test results:
root:/> free
total used free shared buffers
Mem: 60528 13876 46652 0 0
root:/> ./shmem
run ok...
root:/> free
total used free shared buffers
Mem: 60528 15104 45424 0 0
root:/> ./shmem
run ok...
root:/> free
total used free shared buffers
Mem: 60528 16292 44236 0 0
root:/> ./shmem
run ok...
root:/> free
total used free shared buffers
Mem: 60528 17496 43032 0 0
root:/> ./shmem
run ok...
root:/> free
total used free shared buffers
Mem: 60528 18700 41828 0 0
root:/> ./shmem
run ok...
root:/> free
total used free shared buffers
Mem: 60528 19904 40624 0 0
root:/> ./shmem
run ok...
root:/> free
total used free shared buffers
Mem: 60528 21104 39424 0 0
root:/>
It seems the shmem didn't free it's memory after using shmctl(IPC_RMID) to rm
it.
=========
Patch below can work, but I know it's too simple and may cause other problems.
Any ideas is welcome.
Thanks!
Signed-off-by: Bob Liu <lliubbo@gmail.com>
---
diff --git a/fs/ramfs/file-nommu.c b/fs/ramfs/file-nommu.c
index 9eead2c..831e6d5 100644
--- a/fs/ramfs/file-nommu.c
+++ b/fs/ramfs/file-nommu.c
@@ -59,6 +59,8 @@ const struct inode_operations ramfs_file_inode_operations = {
* size 0 on the assumption that it's going to be used for an mmap of shared
* memory
*/
+struct page *ramfs_pages;
+unsigned long ramfs_nr_pages;
int ramfs_nommu_expand_for_mapping(struct inode *inode, size_t newsize)
{
unsigned long npages, xpages, loop;
@@ -114,6 +116,8 @@ int ramfs_nommu_expand_for_mapping(struct inode *inode, size_t newsize)
unlock_page(page);
}
+ ramfs_pages = pages;
+ ramfs_nr_pages = loop;
return 0;
add_error:
diff --git a/fs/ramfs/inode.c b/fs/ramfs/inode.c
index eacb166..2eb33e5 100644
--- a/fs/ramfs/inode.c
+++ b/fs/ramfs/inode.c
@@ -139,6 +139,23 @@ static int ramfs_symlink(struct inode * dir, struct dentry *dentry, const char *
return error;
}
+static void ramfs_delete_inode(struct inode *inode)
+{
+ int loop;
+ struct page *page;
+
+ truncate_inode_pages(&inode->i_data, 0);
+ clear_inode(inode);
+
+ for (loop = 0; loop < ramfs_nr_pages; loop++ ){
+ page = ramfs_pages[loop];
+ page->flags &= ~PAGE_FLAGS_CHECK_AT_FREE;
+ if(page)
+ __free_pages(page, 0);
+ }
+ kfree(ramfs_pages);
+}
+
static const struct inode_operations ramfs_dir_inode_operations = {
.create = ramfs_create,
.lookup = simple_lookup,
@@ -153,6 +170,7 @@ static const struct inode_operations ramfs_dir_inode_operations = {
static const struct super_operations ramfs_ops = {
.statfs = simple_statfs,
+ .delete_inode = ramfs_delete_inode,
.drop_inode = generic_delete_inode,
.show_options = generic_show_options,
};
diff --git a/fs/ramfs/internal.h b/fs/ramfs/internal.h
index 6b33063..0b7b222 100644
--- a/fs/ramfs/internal.h
+++ b/fs/ramfs/internal.h
@@ -12,3 +12,5 @@
extern const struct address_space_operations ramfs_aops;
extern const struct inode_operations ramfs_file_inode_operations;
+extern struct page *ramfs_pages;
+extern unsigned long ramfs_nr_pages;
--
1.6.3.3
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [BUG?] shmem: memory leak on NO-MMU arch
2011-03-08 9:17 [BUG?] shmem: memory leak on NO-MMU arch Bob Liu
@ 2011-03-20 20:35 ` Hugh Dickins
2011-03-21 6:26 ` Bob Liu
2011-03-22 11:47 ` Paul Mundt
0 siblings, 2 replies; 5+ messages in thread
From: Hugh Dickins @ 2011-03-20 20:35 UTC (permalink / raw)
To: Bob Liu
Cc: linux-mm, viro, hch, npiggin, tj, David Howells, Paul Mundt,
Magnus Damm
On Tue, 8 Mar 2011, Bob Liu wrote:
> Hi, folks
Of course I agree with Al and Andrew about your other patch,
I don't know of any shmem inode leak in the MMU case.
I'm afraid we MM folks tend to be very ignorant of the NOMMU case.
I've sometimes wished we had a NOMMU variant of the x86 architecture,
that we could at least build and test changes on.
Let's Cc David, Paul and Magnus: they do understand NOMMU.
>
> I got a problem about shmem on NO-MMU arch, it seems memory leak
> happened.
>
> A simple test file is like this:
> =========
> #include <stdio.h>
> #include <stdlib.h>
> #include <sys/types.h>
> #include <sys/ipc.h>
> #include <sys/shm.h>
> #include <errno.h>
> #include <string.h>
>
> int main(void)
> {
> int i;
> key_t k = ftok("/etc", 42);
>
> for ( i=0; i<2; ++i) {
> int id = shmget(k, 10000, 0644|IPC_CREAT);
> if (id == -1) {
> printf("shmget error\n");
> }
> if(shmctl(id, IPC_RMID, NULL ) == -1) {
> printf("shm rm error\n");
> return -1;
> }
> }
> printf("run ok...\n");
> return 0;
> }
>
> The test results:
> root:/> free
> total used free shared buffers
> Mem: 60528 13876 46652 0 0
> root:/> ./shmem
> run ok...
> root:/> free
> total used free shared buffers
> Mem: 60528 15104 45424 0 0
> root:/> ./shmem
> run ok...
> root:/> free
> total used free shared buffers
> Mem: 60528 16292 44236 0 0
> root:/> ./shmem
> run ok...
> root:/> free
> total used free shared buffers
> Mem: 60528 17496 43032 0 0
> root:/> ./shmem
> run ok...
> root:/> free
> total used free shared buffers
> Mem: 60528 18700 41828 0 0
> root:/> ./shmem
> run ok...
> root:/> free
> total used free shared buffers
> Mem: 60528 19904 40624 0 0
> root:/> ./shmem
> run ok...
> root:/> free
> total used free shared buffers
> Mem: 60528 21104 39424 0 0
> root:/>
>
> It seems the shmem didn't free it's memory after using shmctl(IPC_RMID) to rm
> it.
There does indeed appear to be a leak there. But I'm feeling very
stupid, the leak of ~1200kB per run looks a lot more than the ~20kB
that each run of your test program would lose if the bug is as you say.
Maybe I can't count today.
> =========
>
> Patch below can work, but I know it's too simple and may cause other problems.
> Any ideas is welcome.
>
> Thanks!
>
> Signed-off-by: Bob Liu <lliubbo@gmail.com>
I don't think any patch with a global ramfs_pages, ignoring the
inode in question, can possibly work beyond the simplest of cases.
Yet it does look to me that you're right that ramfs_nommu_expand_for_mapping
forgets to release a reference to its pages; though it's hard to believe
that could go unnoticed for so long - more likely we're both overlooking
something.
> ---
> diff --git a/fs/ramfs/file-nommu.c b/fs/ramfs/file-nommu.c
> index 9eead2c..831e6d5 100644
> --- a/fs/ramfs/file-nommu.c
> +++ b/fs/ramfs/file-nommu.c
> @@ -59,6 +59,8 @@ const struct inode_operations ramfs_file_inode_operations = {
> * size 0 on the assumption that it's going to be used for an mmap of shared
> * memory
> */
> +struct page *ramfs_pages;
> +unsigned long ramfs_nr_pages;
> int ramfs_nommu_expand_for_mapping(struct inode *inode, size_t newsize)
> {
> unsigned long npages, xpages, loop;
> @@ -114,6 +116,8 @@ int ramfs_nommu_expand_for_mapping(struct inode *inode, size_t newsize)
> unlock_page(page);
> }
>
> + ramfs_pages = pages;
> + ramfs_nr_pages = loop;
> return 0;
>
> add_error:
> diff --git a/fs/ramfs/inode.c b/fs/ramfs/inode.c
> index eacb166..2eb33e5 100644
> --- a/fs/ramfs/inode.c
> +++ b/fs/ramfs/inode.c
> @@ -139,6 +139,23 @@ static int ramfs_symlink(struct inode * dir, struct dentry *dentry, const char *
> return error;
> }
>
> +static void ramfs_delete_inode(struct inode *inode)
> +{
> + int loop;
> + struct page *page;
> +
> + truncate_inode_pages(&inode->i_data, 0);
> + clear_inode(inode);
> +
> + for (loop = 0; loop < ramfs_nr_pages; loop++ ){
> + page = ramfs_pages[loop];
> + page->flags &= ~PAGE_FLAGS_CHECK_AT_FREE;
> + if(page)
> + __free_pages(page, 0);
> + }
> + kfree(ramfs_pages);
> +}
> +
> static const struct inode_operations ramfs_dir_inode_operations = {
> .create = ramfs_create,
> .lookup = simple_lookup,
> @@ -153,6 +170,7 @@ static const struct inode_operations ramfs_dir_inode_operations = {
>
> static const struct super_operations ramfs_ops = {
> .statfs = simple_statfs,
> + .delete_inode = ramfs_delete_inode,
> .drop_inode = generic_delete_inode,
> .show_options = generic_show_options,
> };
> diff --git a/fs/ramfs/internal.h b/fs/ramfs/internal.h
> index 6b33063..0b7b222 100644
> --- a/fs/ramfs/internal.h
> +++ b/fs/ramfs/internal.h
> @@ -12,3 +12,5 @@
>
> extern const struct address_space_operations ramfs_aops;
> extern const struct inode_operations ramfs_file_inode_operations;
> +extern struct page *ramfs_pages;
> +extern unsigned long ramfs_nr_pages;
> --
> 1.6.3.3
Here's my own suggestion for a patch; but I've not even tried to
compile it, let alone test it, so I'm certainly not signing it.
Hugh
---
fs/ramfs/file-nommu.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
--- 2.6.38/fs/ramfs/file-nommu.c 2010-10-20 13:30:22.000000000 -0700
+++ linux/fs/ramfs/file-nommu.c 2011-03-20 12:55:35.000000000 -0700
@@ -90,23 +90,19 @@ int ramfs_nommu_expand_for_mapping(struc
split_page(pages, order);
- /* trim off any pages we don't actually require */
- for (loop = npages; loop < xpages; loop++)
- __free_page(pages + loop);
-
/* clear the memory we allocated */
newsize = PAGE_SIZE * npages;
data = page_address(pages);
memset(data, 0, newsize);
- /* attach all the pages to the inode's address space */
+ /* attach the pages we require to the inode's address space */
for (loop = 0; loop < npages; loop++) {
struct page *page = pages + loop;
ret = add_to_page_cache_lru(page, inode->i_mapping, loop,
GFP_KERNEL);
if (ret < 0)
- goto add_error;
+ break;
/* prevent the page from being discarded on memory pressure */
SetPageDirty(page);
@@ -114,11 +110,14 @@ int ramfs_nommu_expand_for_mapping(struc
unlock_page(page);
}
- return 0;
+ /*
+ * release our reference to the pages now added to cache,
+ * and trim off any pages we don't actually require.
+ * truncate inode back to 0 if not all pages could be added??
+ */
+ for (loop = 0; loop < xpages; loop++)
+ put_page(pages + loop);
-add_error:
- while (loop < npages)
- __free_page(pages + loop++);
return ret;
}
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [BUG?] shmem: memory leak on NO-MMU arch
2011-03-20 20:35 ` Hugh Dickins
@ 2011-03-21 6:26 ` Bob Liu
2011-03-22 11:47 ` Paul Mundt
1 sibling, 0 replies; 5+ messages in thread
From: Bob Liu @ 2011-03-21 6:26 UTC (permalink / raw)
To: Hugh Dickins
Cc: linux-mm, viro, hch, npiggin, tj, David Howells, Paul Mundt,
Magnus Damm
Hi, Hugh
On Mon, Mar 21, 2011 at 4:35 AM, Hugh Dickins <hughd@google.com> wrote:
> On Tue, 8 Mar 2011, Bob Liu wrote:
>> Hi, folks
>
> Of course I agree with Al and Andrew about your other patch,
> I don't know of any shmem inode leak in the MMU case.
>
> I'm afraid we MM folks tend to be very ignorant of the NOMMU case.
> I've sometimes wished we had a NOMMU variant of the x86 architecture,
> that we could at least build and test changes on.
>
> Let's Cc David, Paul and Magnus: they do understand NOMMU.
>
>>
>> I got a problem about shmem on NO-MMU arch, it seems memory leak
>> happened.
>>
>> A simple test file is like this:
>> =========
>> #include <stdio.h>
>> #include <stdlib.h>
>> #include <sys/types.h>
>> #include <sys/ipc.h>
>> #include <sys/shm.h>
>> #include <errno.h>
>> #include <string.h>
>>
>> int main(void)
>> {
>> int i;
>> key_t k = ftok("/etc", 42);
>>
>> for ( i=0; i<2; ++i) {
>> int id = shmget(k, 10000, 0644|IPC_CREAT);
>> if (id == -1) {
>> printf("shmget error\n");
>> }
>> if(shmctl(id, IPC_RMID, NULL ) == -1) {
>> printf("shm rm error\n");
>> return -1;
>> }
>> }
>> printf("run ok...\n");
>> return 0;
>> }
>>
>> The test results:
>> root:/> free
>> total used free shared buffers
>> Mem: 60528 13876 46652 0 0
>> root:/> ./shmem
>> run ok...
>> root:/> free
>> total used free shared buffers
>> Mem: 60528 15104 45424 0 0
>> root:/> ./shmem
>> run ok...
>> root:/> free
>> total used free shared buffers
>> Mem: 60528 16292 44236 0 0
>> root:/> ./shmem
>> run ok...
>> root:/> free
>> total used free shared buffers
>> Mem: 60528 17496 43032 0 0
>> root:/> ./shmem
>> run ok...
>> root:/> free
>> total used free shared buffers
>> Mem: 60528 18700 41828 0 0
>> root:/> ./shmem
>> run ok...
>> root:/> free
>> total used free shared buffers
>> Mem: 60528 19904 40624 0 0
>> root:/> ./shmem
>> run ok...
>> root:/> free
>> total used free shared buffers
>> Mem: 60528 21104 39424 0 0
>> root:/>
>>
>> It seems the shmem didn't free it's memory after using shmctl(IPC_RMID) to rm
>> it.
>
> There does indeed appear to be a leak there. But I'm feeling very
> stupid, the leak of ~1200kB per run looks a lot more than the ~20kB
> that each run of your test program would lose if the bug is as you say.
> Maybe I can't count today.
>
>> =========
>>
>> Patch below can work, but I know it's too simple and may cause other problems.
>> Any ideas is welcome.
>>
>> Thanks!
>>
>> Signed-off-by: Bob Liu <lliubbo@gmail.com>
>
> I don't think any patch with a global ramfs_pages, ignoring the
> inode in question, can possibly work beyond the simplest of cases.
>
>
> Yet it does look to me that you're right that ramfs_nommu_expand_for_mapping
> forgets to release a reference to its pages; though it's hard to believe
> that could go unnoticed for so long - more likely we're both overlooking
> something.
>
>> ---
>> diff --git a/fs/ramfs/file-nommu.c b/fs/ramfs/file-nommu.c
>> index 9eead2c..831e6d5 100644
>> --- a/fs/ramfs/file-nommu.c
>> +++ b/fs/ramfs/file-nommu.c
>> @@ -59,6 +59,8 @@ const struct inode_operations ramfs_file_inode_operations = {
>> * size 0 on the assumption that it's going to be used for an mmap of shared
>> * memory
>> */
>> +struct page *ramfs_pages;
>> +unsigned long ramfs_nr_pages;
>> int ramfs_nommu_expand_for_mapping(struct inode *inode, size_t newsize)
>> {
>> unsigned long npages, xpages, loop;
>> @@ -114,6 +116,8 @@ int ramfs_nommu_expand_for_mapping(struct inode *inode, size_t newsize)
>> unlock_page(page);
>> }
>>
>> + ramfs_pages = pages;
>> + ramfs_nr_pages = loop;
>> return 0;
>>
>> add_error:
>> diff --git a/fs/ramfs/inode.c b/fs/ramfs/inode.c
>> index eacb166..2eb33e5 100644
>> --- a/fs/ramfs/inode.c
>> +++ b/fs/ramfs/inode.c
>> @@ -139,6 +139,23 @@ static int ramfs_symlink(struct inode * dir, struct dentry *dentry, const char *
>> return error;
>> }
>>
>> +static void ramfs_delete_inode(struct inode *inode)
>> +{
>> + int loop;
>> + struct page *page;
>> +
>> + truncate_inode_pages(&inode->i_data, 0);
>> + clear_inode(inode);
>> +
>> + for (loop = 0; loop < ramfs_nr_pages; loop++ ){
>> + page = ramfs_pages[loop];
>> + page->flags &= ~PAGE_FLAGS_CHECK_AT_FREE;
>> + if(page)
>> + __free_pages(page, 0);
>> + }
>> + kfree(ramfs_pages);
>> +}
>> +
>> static const struct inode_operations ramfs_dir_inode_operations = {
>> .create = ramfs_create,
>> .lookup = simple_lookup,
>> @@ -153,6 +170,7 @@ static const struct inode_operations ramfs_dir_inode_operations = {
>>
>> static const struct super_operations ramfs_ops = {
>> .statfs = simple_statfs,
>> + .delete_inode = ramfs_delete_inode,
>> .drop_inode = generic_delete_inode,
>> .show_options = generic_show_options,
>> };
>> diff --git a/fs/ramfs/internal.h b/fs/ramfs/internal.h
>> index 6b33063..0b7b222 100644
>> --- a/fs/ramfs/internal.h
>> +++ b/fs/ramfs/internal.h
>> @@ -12,3 +12,5 @@
>>
>> extern const struct address_space_operations ramfs_aops;
>> extern const struct inode_operations ramfs_file_inode_operations;
>> +extern struct page *ramfs_pages;
>> +extern unsigned long ramfs_nr_pages;
>> --
>> 1.6.3.3
>
> Here's my own suggestion for a patch; but I've not even tried to
> compile it, let alone test it, so I'm certainly not signing it.
>
Great.
I have compiled and tested this patch and it works fine.
Would you please sign and commit it ?
Thanks.
root:/> free
total used free shared buffers
Mem: 60512 13852 46660 0 0
root:/> ./shmem
run ok...
root:/> free
total used free shared buffers
Mem: 60512 13892 46620 0 0
root:/> ./shmem
run ok...
root:/> free
total used free shared buffers
Mem: 60512 13868 46644 0 0
root:/> ./shmem
run ok...
root:/> free
total used free shared buffers
Mem: 60512 13860 46652 0 0
root:/> ./shmem
run ok...
root:/> free
total used free shared buffers
Mem: 60512 13860 46652 0 0
root:/> ./shmem
run ok...
root:/> free
total used free shared buffers
Mem: 60512 13864 46648 0 0
root:/> ./shmem
run ok...
root:/> free
total used free shared buffers
Mem: 60512 13868 46644 0 0
root:/> ./shmem
run ok...
root:/> free
total used free shared buffers
Mem: 60512 13868 46644 0 0
root:/> ./shmem
run ok...
root:/> free
total used free shared buffers
Mem: 60512 13868 46644 0 0
root:/>
> ---
>
> fs/ramfs/file-nommu.c | 19 +++++++++----------
> 1 file changed, 9 insertions(+), 10 deletions(-)
>
> --- 2.6.38/fs/ramfs/file-nommu.c 2010-10-20 13:30:22.000000000 -0700
> +++ linux/fs/ramfs/file-nommu.c 2011-03-20 12:55:35.000000000 -0700
> @@ -90,23 +90,19 @@ int ramfs_nommu_expand_for_mapping(struc
>
> split_page(pages, order);
>
> - /* trim off any pages we don't actually require */
> - for (loop = npages; loop < xpages; loop++)
> - __free_page(pages + loop);
> -
> /* clear the memory we allocated */
> newsize = PAGE_SIZE * npages;
> data = page_address(pages);
> memset(data, 0, newsize);
>
> - /* attach all the pages to the inode's address space */
> + /* attach the pages we require to the inode's address space */
> for (loop = 0; loop < npages; loop++) {
> struct page *page = pages + loop;
>
> ret = add_to_page_cache_lru(page, inode->i_mapping, loop,
> GFP_KERNEL);
> if (ret < 0)
> - goto add_error;
> + break;
>
> /* prevent the page from being discarded on memory pressure */
> SetPageDirty(page);
> @@ -114,11 +110,14 @@ int ramfs_nommu_expand_for_mapping(struc
> unlock_page(page);
> }
>
> - return 0;
> + /*
> + * release our reference to the pages now added to cache,
> + * and trim off any pages we don't actually require.
> + * truncate inode back to 0 if not all pages could be added??
> + */
> + for (loop = 0; loop < xpages; loop++)
> + put_page(pages + loop);
>
> -add_error:
> - while (loop < npages)
> - __free_page(pages + loop++);
> return ret;
> }
>
>
--
Regards,
--Bob
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [BUG?] shmem: memory leak on NO-MMU arch
2011-03-20 20:35 ` Hugh Dickins
2011-03-21 6:26 ` Bob Liu
@ 2011-03-22 11:47 ` Paul Mundt
2011-03-23 3:23 ` Bob Liu
1 sibling, 1 reply; 5+ messages in thread
From: Paul Mundt @ 2011-03-22 11:47 UTC (permalink / raw)
To: Hugh Dickins
Cc: Bob Liu, linux-mm, viro, hch, npiggin, tj, David Howells,
Magnus Damm
On Sun, Mar 20, 2011 at 01:35:50PM -0700, Hugh Dickins wrote:
> On Tue, 8 Mar 2011, Bob Liu wrote:
> > Hi, folks
>
> Of course I agree with Al and Andrew about your other patch,
> I don't know of any shmem inode leak in the MMU case.
>
> I'm afraid we MM folks tend to be very ignorant of the NOMMU case.
> I've sometimes wished we had a NOMMU variant of the x86 architecture,
> that we could at least build and test changes on.
>
NOMMU folks tend to be very ignorant of the MM cases, so it all balances
out :-)
> Let's Cc David, Paul and Magnus: they do understand NOMMU.
>
> > root:/> ./shmem
> > run ok...
> > root:/> free
> > total used free shared buffers
> > Mem: 60528 19904 40624 0 0
> > root:/> ./shmem
> > run ok...
> > root:/> free
> > total used free shared buffers
> > Mem: 60528 21104 39424 0 0
> > root:/>
> >
> > It seems the shmem didn't free it's memory after using shmctl(IPC_RMID) to rm
> > it.
>
> There does indeed appear to be a leak there. But I'm feeling very
> stupid, the leak of ~1200kB per run looks a lot more than the ~20kB
> that each run of your test program would lose if the bug is as you say.
> Maybe I can't count today.
>
Your 1200 figure looks accurate, I came up with the same figure. In any
event, it would be interesting to know what page size is being used. It's
not uncommon to see a 64kB PAGE_SIZE on a system with 64M of memory, but
that still wouldn't account for that level of discrepancy.
My initial thought was that perhaps we were missing a
truncate_pagecache() for a caller of ramfs_nommu_expand_for_mapping() on
an existing inode with an established size (which assumes that one is
always expanding from 0 up, and so doesn't bother with truncating), but
the shmem user in this case is fetching a new inode on each iteration so
this seems improbable, and the same 1200kB discrepancy is visible even
after the initial shmget. I'm likely overlooking something obvious.
> Yet it does look to me that you're right that ramfs_nommu_expand_for_mapping
> forgets to release a reference to its pages; though it's hard to believe
> that could go unnoticed for so long - more likely we're both overlooking
> something.
>
page refcounting on nommu has a rather tenuous relationship with reality
at the best of times; surprise was indeed not the first thought that came
to mind.
My guess is that this used to be caught by virtue of the __put_page()
hack we used to have in __free_pages_ok() for the nommu case, prior to
the conversion to compound pages.
> Here's my own suggestion for a patch; but I've not even tried to
> compile it, let alone test it, so I'm certainly not signing it.
>
This definitely looks like an improvement, but I wonder if it's not
easier to simply use alloc_pages_exact() and throw out the bulk of the
function entirely (a __GFP_ZERO would further simplify things, too)?
> @@ -114,11 +110,14 @@ int ramfs_nommu_expand_for_mapping(struc
> unlock_page(page);
> }
>
> - return 0;
> + /*
> + * release our reference to the pages now added to cache,
> + * and trim off any pages we don't actually require.
> + * truncate inode back to 0 if not all pages could be added??
> + */
> + for (loop = 0; loop < xpages; loop++)
> + put_page(pages + loop);
>
Unless you have some callchain in mind that I'm not aware of, an error is
handed back when add_to_page_cache_lru() fails and the inode is destroyed
by the caller in each case. As such, we should make it down to
truncate_inode_pages(..., 0) via natural iput() eviction.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [BUG?] shmem: memory leak on NO-MMU arch
2011-03-22 11:47 ` Paul Mundt
@ 2011-03-23 3:23 ` Bob Liu
0 siblings, 0 replies; 5+ messages in thread
From: Bob Liu @ 2011-03-23 3:23 UTC (permalink / raw)
To: Paul Mundt
Cc: Hugh Dickins, linux-mm, viro, hch, npiggin, tj, David Howells,
Magnus Damm
On Tue, Mar 22, 2011 at 7:47 PM, Paul Mundt <lethal@linux-sh.org> wrote:
> On Sun, Mar 20, 2011 at 01:35:50PM -0700, Hugh Dickins wrote:
>> On Tue, 8 Mar 2011, Bob Liu wrote:
>> > Hi, folks
>>
>> Of course I agree with Al and Andrew about your other patch,
>> I don't know of any shmem inode leak in the MMU case.
>>
>> I'm afraid we MM folks tend to be very ignorant of the NOMMU case.
>> I've sometimes wished we had a NOMMU variant of the x86 architecture,
>> that we could at least build and test changes on.
>>
> NOMMU folks tend to be very ignorant of the MM cases, so it all balances
> out :-)
>
>> Let's Cc David, Paul and Magnus: they do understand NOMMU.
>>
>> > root:/> ./shmem
>> > run ok...
>> > root:/> free
>> > total used free shared buffers
>> > Mem: 60528 19904 40624 0 0
>> > root:/> ./shmem
>> > run ok...
>> > root:/> free
>> > total used free shared buffers
>> > Mem: 60528 21104 39424 0 0
>> > root:/>
>> >
>> > It seems the shmem didn't free it's memory after using shmctl(IPC_RMID) to rm
>> > it.
>>
>> There does indeed appear to be a leak there. But I'm feeling very
>> stupid, the leak of ~1200kB per run looks a lot more than the ~20kB
>> that each run of your test program would lose if the bug is as you say.
>> Maybe I can't count today.
>>
> Your 1200 figure looks accurate, I came up with the same figure. In any
> event, it would be interesting to know what page size is being used. It's
> not uncommon to see a 64kB PAGE_SIZE on a system with 64M of memory, but
> that still wouldn't account for that level of discrepancy.
>
I am very sorry that I attached the wrong test source file by mistake.
The loop "for ( i=0; i<2; ++i) {" should be "for (i = 0; i < 100; ++i) {".
I changed 100 to 2 for some tests, but I forgot it.
>
> My initial thought was that perhaps we were missing a
> truncate_pagecache() for a caller of ramfs_nommu_expand_for_mapping() on
> an existing inode with an established size (which assumes that one is
> always expanding from 0 up, and so doesn't bother with truncating), but
> the shmem user in this case is fetching a new inode on each iteration so
> this seems improbable, and the same 1200kB discrepancy is visible even
> after the initial shmget. I'm likely overlooking something obvious.
>
>> Yet it does look to me that you're right that ramfs_nommu_expand_for_mapping
>> forgets to release a reference to its pages; though it's hard to believe
>> that could go unnoticed for so long - more likely we're both overlooking
>> something.
>>
> page refcounting on nommu has a rather tenuous relationship with reality
> at the best of times; surprise was indeed not the first thought that came
> to mind.
>
> My guess is that this used to be caught by virtue of the __put_page()
> hack we used to have in __free_pages_ok() for the nommu case, prior to
> the conversion to compound pages.
>
>> Here's my own suggestion for a patch; but I've not even tried to
>> compile it, let alone test it, so I'm certainly not signing it.
>>
> This definitely looks like an improvement, but I wonder if it's not
> easier to simply use alloc_pages_exact() and throw out the bulk of the
> function entirely (a __GFP_ZERO would further simplify things, too)?
>
>> @@ -114,11 +110,14 @@ int ramfs_nommu_expand_for_mapping(struc
>> unlock_page(page);
>> }
>>
>> - return 0;
>> + /*
>> + * release our reference to the pages now added to cache,
>> + * and trim off any pages we don't actually require.
>> + * truncate inode back to 0 if not all pages could be added??
>> + */
>> + for (loop = 0; loop < xpages; loop++)
>> + put_page(pages + loop);
>>
> Unless you have some callchain in mind that I'm not aware of, an error is
> handed back when add_to_page_cache_lru() fails and the inode is destroyed
> by the caller in each case. As such, we should make it down to
> truncate_inode_pages(..., 0) via natural iput() eviction.
>
What about this is?
-----------
--- a/fs/ramfs/file-nommu.c
+++ b/fs/ramfs/file-nommu.c
@@ -112,6 +112,7 @@ int ramfs_nommu_expand_for_mapping(struct inode
*inode, size_t newsize)
SetPageDirty(page);
unlock_page(page);
+ put_page(page);
}
Thanks
--
Regards,
--Bob
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-03-23 3:23 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-03-08 9:17 [BUG?] shmem: memory leak on NO-MMU arch Bob Liu
2011-03-20 20:35 ` Hugh Dickins
2011-03-21 6:26 ` Bob Liu
2011-03-22 11:47 ` Paul Mundt
2011-03-23 3:23 ` Bob Liu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).