Re: [PATCH] vmscan: scan pages until it founds eligible pages

[Michal Hocko <mhocko@kernel.org>]

On Wed 10-05-17 10:46:54, Minchan Kim wrote:
> On Wed, May 03, 2017 at 08:00:44AM +0200, Michal Hocko wrote:
[...]
> > @@ -1486,6 +1486,12 @@ static unsigned long isolate_lru_pages(unsigned long nr_to_scan,
> >  			continue;
> >  		}
> >  
> > +		/*
> > +		 * Do not count skipped pages because we do want to isolate
> > +		 * some pages even when the LRU mostly contains ineligible
> > +		 * pages
> > +		 */
> 
> How about adding comment about "why"?
> 
> /*
>  * Do not count skipped pages because it makes the function to return with
>  * none isolated pages if the LRU mostly contains inelgible pages so that
>  * VM cannot reclaim any pages and trigger premature OOM.
>  */

I am not sure this is necessarily any better. Mentioning a pre-mature
OOM would require a much better explanation because a first immediate
question would be "why don't we scan those pages at priority 0". Also
decision about the OOM is at a different layer and it might change in
future when this doesn't apply any more. But it is not like I would
insist...

> > +		scan++;
> >  		switch (__isolate_lru_page(page, mode)) {
> >  		case 0:
> >  			nr_pages = hpage_nr_pages(page);
> 
> Confirmed.

Hmm. I can clearly see how we could skip over too many pages and hit
small reclaim priorities too quickly but I am still scratching my head
about how we could hit the OOM killer as a result. The amount of pages
on the active anonymous list suggests that we are not able to rotate
pages quickly enough. I have to keep thinking about that.

> It works as expected but it changed scan counter's behavior.  How
> about this?

OK, it looks good to me. I believe the main motivation of the original
patch from Johannes was to drop the magical total_skipped.
 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 2314aca47d12..846922d7942e 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1469,7 +1469,7 @@ static __always_inline void update_lru_sizes(struct lruvec *lruvec,
>   *
>   * Appropriate locks must be held before calling this function.
>   *
> - * @nr_to_scan:	The number of pages to look through on the list.
> + * @nr_to_scan:	The number of eligible pages to look through on the list.
>   * @lruvec:	The LRU vector to pull pages from.
>   * @dst:	The temp list to put pages on to.
>   * @nr_scanned:	The number of pages that were scanned.
> @@ -1489,11 +1489,13 @@ static unsigned long isolate_lru_pages(unsigned long nr_to_scan,
>  	unsigned long nr_zone_taken[MAX_NR_ZONES] = { 0 };
>  	unsigned long nr_skipped[MAX_NR_ZONES] = { 0, };
>  	unsigned long skipped = 0;
> -	unsigned long scan, nr_pages;
> +	unsigned long scan, total_scan, nr_pages;
>  	LIST_HEAD(pages_skipped);
>  
> -	for (scan = 0; scan < nr_to_scan && nr_taken < nr_to_scan &&
> -					!list_empty(src); scan++) {
> +	for (total_scan = scan = 0; scan < nr_to_scan &&
> +					nr_taken < nr_to_scan &&
> +					!list_empty(src);
> +					total_scan++) {
>  		struct page *page;
>  
>  		page = lru_to_page(src);
> @@ -1507,6 +1509,13 @@ static unsigned long isolate_lru_pages(unsigned long nr_to_scan,
>  			continue;
>  		}
>  
> +		/*
> +		 * Do not count skipped pages because it makes the function to
> +		 * return with none isolated pages if the LRU mostly contains
> +		 * inelgible pages so that VM cannot reclaim any pages and
> +		 * trigger premature OOM.
> +		 */
> +		scan++;
>  		switch (__isolate_lru_page(page, mode)) {
>  		case 0:
>  			nr_pages = hpage_nr_pages(page);
> @@ -1544,9 +1553,9 @@ static unsigned long isolate_lru_pages(unsigned long nr_to_scan,
>  			skipped += nr_skipped[zid];
>  		}
>  	}
> -	*nr_scanned = scan;
> +	*nr_scanned = total_scan;
>  	trace_mm_vmscan_lru_isolate(sc->reclaim_idx, sc->order, nr_to_scan,
> -				    scan, skipped, nr_taken, mode, lru);
> +				    total_scan, skipped, nr_taken, mode, lru);
>  	update_lru_sizes(lruvec, lru, nr_zone_taken);
>  	return nr_taken;
>  }

-- 
Michal Hocko
SUSE Labs