Re: [PATCH 1/5] mm, vmscan: Do not account skipped pages as scanned

[Mel Gorman]

On Thu, Jul 21, 2016 at 02:16:48PM +0900, Minchan Kim wrote:
> On Wed, Jul 20, 2016 at 04:21:47PM +0100, Mel Gorman wrote:
> > Page reclaim determines whether a pgdat is unreclaimable by examining how
> > many pages have been scanned since a page was freed and comparing that
> > to the LRU sizes. Skipped pages are not considered reclaim candidates but
> > contribute to scanned. This can prematurely mark a pgdat as unreclaimable
> > and trigger an OOM kill.
> > 
> > While this does not fix an OOM kill message reported by Joonsoo Kim,
> > it did stop pgdat being marked unreclaimable.
> > 
> > Signed-off-by: Mel Gorman <mgorman@xxxxxxxxxxxxxxxxxxx>
> > ---
> >  mm/vmscan.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index 22aec2bcfeec..b16d578ce556 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -1415,7 +1415,7 @@ static unsigned long isolate_lru_pages(unsigned long nr_to_scan,
> >  	LIST_HEAD(pages_skipped);
> >  
> >  	for (scan = 0; scan < nr_to_scan && nr_taken < nr_to_scan &&
> > -					!list_empty(src); scan++) {
> > +					!list_empty(src);) {
> >  		struct page *page;
> >  
> >  		page = lru_to_page(src);
> > @@ -1429,6 +1429,9 @@ static unsigned long isolate_lru_pages(unsigned long nr_to_scan,
> >  			continue;
> >  		}
> >  
> > +		/* Pages skipped do not contribute to scan */
> 
> The comment should explain why.
> 
> /* Pages skipped do not contribute to scan to prevent premature OOM */
> 

Specifically, it's to prevent pgdat being considered unreclaimable
prematurely. I'll update the comment.

> 
> > +		scan++;
> > +
> 
> 
> The one of my concern about node-lru is to add more lru lock contetion
> in multiple zone system so such unbounded skip scanning under the lock
> should have a limit to prevent latency spike and serialization of
> current reclaim work.
> 

The LRU lock already was quite a large lock, particularly on NUMA systems,
with contention raising the more direct reclaimers that are active. It's
worth remembering that the series also shows much lower system CPU time
in some tests. This is the current CPU usage breakdown for a parallel dd test

           4.7.0-rc4   4.7.0-rc7   4.7.0-rc7
        mmotm-20160623mm1-followup-v3r1mm1-oomfix-v4r2
User         1548.01      927.23      777.74
System       8609.71     5540.02     4445.56
Elapsed      3587.10     3598.00     3498.54

The LRU lock is held during skips but it's also doing no real work.

> Another concern is big mismatch between the number of pages from list and
> LRU stat count because lruvec_lru_size call sites don't take the stat
> under the lock while isolate_lru_pages moves many pages from lru list
> to temporal skipped list.
> 

It's already known that the reading of the LRU size can mismatch the
actual size. It's why inactive_list_is_low() in the last patch has
checks like

inactive -= min(inactive, inactive_zone);

It's watching for underflows

-- 
Mel Gorman
SUSE Labs