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 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx"> email@xxxxxxxxx </a>