Re: [PATCH] mm: fix unevictable page reclaim when calling madvise_pageout

[Johannes Weiner <hannes@cmpxchg.org>]

On Wed, Oct 30, 2019 at 06:45:33PM +0100, Michal Hocko wrote:
> On Wed 30-10-19 09:52:39, Minchan Kim wrote:
> > On Tue, Oct 29, 2019 at 06:45:12PM +0800, zhong jiang wrote:
> > > On 2019/10/29 17:40, Michal Hocko wrote:
> > > > On Tue 29-10-19 17:30:57, zhong jiang wrote:
> > > >> On 2019/10/29 16:11, Michal Hocko wrote:
> > > >>> [Cc Minchan]
> > > > [...]
> > > >>> Removing a long existing BUG_ON begs for a much better explanation.
> > > >>> shrink_page_list is not a trivial piece of code but I _suspect_ that
> > > >>> removing it should be ok for mapped pages at least (try_to_unmap) but I
> > > >>> am not so sure how unmapped unevictable pages are handled from top of my
> > > >>> head.
> > > >> As to the unmapped unevictable pages.  shrink_page_list has taken that into account.
> > > >>
> > > >> shinkr_page_list
> > > >>      page_evictable     --> will filter the unevictable pages to putback its lru.
> > > > Ohh, it is right there at the top. Missed it. The check has been added
> > > > by Nick along with the BUG_ON. So it is sounds more like a "this
> > > > shouldn't happen" bugon. I wouldn't mind to remove it with that
> > > > justification.
> > > As you has said,   Minchan fix the same kind of bug by checking PageUnevictable (I did not notice before)
> > > Wait for Minchan to see whether  he has better reason. thanks,
> > 
> > madvise_pageout could work with a shared page and one of the vmas among processes
> > could do mlock so it could pass Unevictable LRU pages into shrink_page_list.
> > It's pointless to try reclaim unevictable pages from the beginning so I want to fix
> > madvise_pageout via introducing only_evictable flag into the API so that
> > madvise_pageout uses it as "true".
> > 
> > If we want to remove the PageUnevictable VM_BUG_ON_PAGE in shrink_page_list,
> > I want to see more strong reason why it happens and why caller couldn't
> > filter them out from the beginning.
> 
> Why is this preferable over removing the VM_BUG_ON condition? In other
> words why should we keep PageUnevictable check there?

The mlock LRU shuffling is a bit tricky and can race with page reclaim
or others isolating the page from the LRU list. If another isolator
wins, it has to move the page during putback on behalf of mlock.

See the implementation and comments in __pagevec_lru_add_fn().

That's why page reclaim can see !page_evictable(), but it must not see
pages that have the PageUnevictable lru bit already set. Because that
would mean the isolation/putback machinery messed up somewhere and the
page LRU state is corrupt.

As that machinery is non-trivial, it's useful to have that sanity
check in page reclaim.