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
