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@xxxxxxxxx>
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@xxxxxxxxxx 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@xxxxxxxxx";> email@xxxxxxxxx </a>