On Fri, 1 Apr 2011, Bob Liu wrote:
> Hi, Andrew
>
> cc'd some folks working on nommu.
>
> On Tue, Mar 29, 2011 at 8:02 AM, Andrew Morton
> <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> > On Mon, 28 Mar 2011 13:32:35 +0800
> > Bob Liu <lliubbo@xxxxxxxxx> wrote:
> >
> >> On no-mmu arch, there is a memleak duirng shmem test.
> >> The cause of this memleak is ramfs_nommu_expand_for_mapping() added page
> >> refcount to 2 which makes iput() can't free that pages.
> >>
> >> The simple test file is like this:
> >> int main(void)
> >> {
> >> Â Â Â int i;
> >> Â Â Â key_t k = ftok("/etc", 42);
> >>
> >> Â Â Â for ( i=0; i<100; ++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;
> >> }
> >>
> >> ...
> >>
> >> diff --git a/fs/ramfs/file-nommu.c b/fs/ramfs/file-nommu.c
> >> index 9eead2c..fbb0b47 100644
> >> --- 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);
> >> Â Â Â }
> >>
> >> Â Â Â return 0;
> >
> > Something is still wrong here.
> >
> > A live, in-use page should have a refcount of three. ÂOne for the
> > existence of the page, one for its presence on the page LRU and one for
> > its existence in the pagecache radix tree.
> >
> > So allocation should do:
> >
> > Â Â Â Âalloc_pages()
> > Â Â Â Âadd_to_page_cache()
> > Â Â Â Âadd_to_lru()
> >
> > and deallocation should do
> >
> > Â Â Â Âremove_from_lru()
> > Â Â Â Âremove_from_page_cache()
> > Â Â Â Âput_page()
> >
> > If this protocol is followed correctly, there is no need to do a
> > put_page() during the allocation/setup phase!
> >
> > I suspect that the problem in nommu really lies in the
> > deallocation/teardown phase.
> >
>
> What about below patch ?
>
> BTW: It seems that in MMU cases shmem pages are freed during memory reclaim,
> since I didn't find the direct free place.
> I am not sure maybe I got something wrong ?
>
> Signed-off-by: Bob Liu <lliubbo@xxxxxxxxx>
No, I really think this ramfs_evict_inode() thing is a quite
unnecessary complication: your little put_page() patch much nicer.
Hugh
> ---
> fs/ramfs/file-nommu.c | 1 +
> fs/ramfs/inode.c | 22 ++++++++++++++++++++++
> 2 files changed, 23 insertions(+), 0 deletions(-)
>
> diff --git a/fs/ramfs/file-nommu.c b/fs/ramfs/file-nommu.c
> index fbb0b47..11f48eb 100644
> --- a/fs/ramfs/file-nommu.c
> +++ b/fs/ramfs/file-nommu.c
> @@ -114,6 +114,7 @@ int ramfs_nommu_expand_for_mapping(struct inode
> *inode, size_t newsize)
> unlock_page(page);
> put_page(page);
> }
> + inode->i_private = pages;
>
> return 0;
>
> diff --git a/fs/ramfs/inode.c b/fs/ramfs/inode.c
> index eacb166..e446d9f 100644
> --- a/fs/ramfs/inode.c
> +++ b/fs/ramfs/inode.c
> @@ -151,9 +151,31 @@ static const struct inode_operations
> ramfs_dir_inode_operations = {
> .rename = simple_rename,
> };
>
> +#ifndef CONFIG_MMU
> +static void ramfs_evict_inode(struct inode *inode)
> +{
> + int i;
> + struct page *free_pages = (struct page *)inode->i_private;
> +
> + /*
> + * for nommu arch, need an extra put_page so that pages gotten
> + * by ramfs_nommu_expand_for_mapping() can be freed
> + */
> + for (i = 0; i < inode->i_data.nrpages; i++)
> + put_page(free_pages + i);
> +
> + if (inode->i_data.nrpages)
> + truncate_inode_pages(&inode->i_data, 0);
> + end_writeback(inode);
> +}
> +#endif
> +
> static const struct super_operations ramfs_ops = {
> .statfs = simple_statfs,
> .drop_inode = generic_delete_inode,
> +#ifndef CONFIG_MMU
> + .evict_inode = ramfs_evict_inode,
> +#endif
> .show_options = generic_show_options,
> };
>
> --
> 1.6.3.3