Hi, Hugh
On Mon, Mar 21, 2011 at 4:35 AM, Hugh Dickins <hughd@xxxxxxxxxx> 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@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.
>
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@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