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

[Paul Mundt <lethal@linux-sh.org>]

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>