From: Bob Liu <lliubbo@gmail.com>
To: Hugh Dickins <hughd@google.com>
Cc: linux-mm@kvack.org, viro@zeniv.linux.org.uk, hch@lst.de,
npiggin@kernel.dk, tj@kernel.org,
David Howells <dhowells@redhat.com>,
Paul Mundt <lethal@linux-sh.org>,
Magnus Damm <magnus.damm@gmail.com>
Subject: Re: [BUG?] shmem: memory leak on NO-MMU arch
Date: Mon, 21 Mar 2011 14:26:03 +0800 [thread overview]
Message-ID: <AANLkTi=a2R+72rp2sO4gt72ATxHsE71zkCF6_ScSu_a+@mail.gmail.com> (raw)
In-Reply-To: <alpine.LSU.2.00.1103201258280.3776@sister.anvils>
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>
next prev parent reply other threads:[~2011-03-21 6:26 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-03-08 9:17 Bob Liu
2011-03-20 20:35 ` Hugh Dickins
2011-03-21 6:26 ` Bob Liu [this message]
2011-03-22 11:47 ` Paul Mundt
2011-03-23 3:23 ` Bob Liu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='AANLkTi=a2R+72rp2sO4gt72ATxHsE71zkCF6_ScSu_a+@mail.gmail.com' \
--to=lliubbo@gmail.com \
--cc=dhowells@redhat.com \
--cc=hch@lst.de \
--cc=hughd@google.com \
--cc=lethal@linux-sh.org \
--cc=linux-mm@kvack.org \
--cc=magnus.damm@gmail.com \
--cc=npiggin@kernel.dk \
--cc=tj@kernel.org \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox