[BUG?] shmem: memory leak on NO-MMU arch

[BUG?] shmem: memory leak on NO-MMU arch

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

Re: [BUG?] shmem: memory leak on NO-MMU arch

[Hugh Dickins]

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>

Re: [BUG?] shmem: memory leak on NO-MMU arch

[Bob Liu]

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>

Re: [BUG?] shmem: memory leak on NO-MMU arch

[Paul Mundt]

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>

Re: [BUG?] shmem: memory leak on NO-MMU arch

[Bob Liu]

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>