RE: [RFC] mm: change mm_advise_free to clear page dirty

["Wang, Yalin" <Yalin.Wang@sonymobile.com>]

> -----Original Message-----
> From: Minchan Kim [mailto:minchan.kim@gmail.com] On Behalf Of Minchan Kim
> Sent: Friday, February 27, 2015 1:28 PM
> To: Wang, Yalin
> Cc: Michal Hocko; Andrew Morton; linux-kernel@vger.kernel.org; linux-
> mm@kvack.org; Rik van Riel; Johannes Weiner; Mel Gorman; Shaohua Li
> Subject: Re: [RFC] mm: change mm_advise_free to clear page dirty
> 
> Hello,
> 
> On Fri, Feb 27, 2015 at 11:37:18AM +0800, Wang, Yalin wrote:
> > This patch add ClearPageDirty() to clear AnonPage dirty flag,
> > the Anonpage mapcount must be 1, so that this page is only used by
> > the current process, not shared by other process like fork().
> > if not clear page dirty for this anon page, the page will never be
> > treated as freeable.
> 
> In case of anonymous page, it has PG_dirty when VM adds it to
> swap cache and clear it in clear_page_dirty_for_io. That's why
> I added ClearPageDirty if we found it in swapcache.
> What case am I missing? It would be better to understand if you
> describe specific scenario.
> 
> Thanks.
> 
> >
> > Signed-off-by: Yalin Wang <yalin.wang@sonymobile.com>
> > ---
> >  mm/madvise.c | 15 +++++----------
> >  1 file changed, 5 insertions(+), 10 deletions(-)
> >
> > diff --git a/mm/madvise.c b/mm/madvise.c
> > index 6d0fcb8..257925a 100644
> > --- a/mm/madvise.c
> > +++ b/mm/madvise.c
> > @@ -297,22 +297,17 @@ static int madvise_free_pte_range(pmd_t *pmd,
> unsigned long addr,
> >  			continue;
> >
> >  		page = vm_normal_page(vma, addr, ptent);
> > -		if (!page)
> > +		if (!page || !PageAnon(page) || !trylock_page(page))
> >  			continue;
> >
> >  		if (PageSwapCache(page)) {
> > -			if (!trylock_page(page))
> > +			if (!try_to_free_swap(page))
> >  				continue;
> > -
> > -			if (!try_to_free_swap(page)) {
> > -				unlock_page(page);
> > -				continue;
> > -			}
> > -
> > -			ClearPageDirty(page);
> > -			unlock_page(page);
> >  		}
> >
> > +		if (page_mapcount(page) == 1)
> > +			ClearPageDirty(page);
> > +		unlock_page(page);
> >  		/*
> >  		 * Some of architecture(ex, PPC) don't update TLB
> >  		 * with set_pte_at and tlb_remove_tlb_entry so for
> > --
Yes, for page which is in SwapCache, it is correct,
But for anon page which is not in SwapCache, it is always
PageDirty(), so we should also clear dirty bit to make it freeable,

Another problem  is that if an anon page is shared by more than one process,
This happened when fork(), the anon page will be copy on write,
In this case, we should not clear page dirty,
Otherwise, other process will get zero page if the page is freed by reclaim path,
This is not correct for other process which don't call MADV_FREE syscall.

Thanks