Re: [PATCH v2 1/2] mm: use add_page_to_lru_list()/page_lru()/page_off_lru()

[Yu Zhao <yuzhao@google.com>]

On Thu, Sep 03, 2020 at 10:28:32AM +0200, Michal Hocko wrote:
> On Mon 31-08-20 11:50:41, Yu Zhao wrote:
> [...]
> > @@ -1860,16 +1859,11 @@ static unsigned noinline_for_stack move_pages_to_lru(struct lruvec *lruvec,
> >  		lruvec = mem_cgroup_page_lruvec(page, pgdat);
> >  
> >  		SetPageLRU(page);
> > -		lru = page_lru(page);
> > -
> > -		nr_pages = thp_nr_pages(page);
> > -		update_lru_size(lruvec, lru, page_zonenum(page), nr_pages);
> > -		list_move(&page->lru, &lruvec->lists[lru]);
> > +		add_page_to_lru_list(page, lruvec, page_lru(page));
> >  
> >  		if (put_page_testzero(page)) {
> >  			__ClearPageLRU(page);
> > -			__ClearPageActive(page);
> 
> This should go into its own patch. The rest is a mechanical and clear.

Thanks for reviewing.

I assume you are worrying about PG_unevictable being set on the page
because page_off_lru() checks it first. Well, if this can happen,
wouldn't it have been triggering bad_page()? PG_unevictable is in
PAGE_FLAGS_CHECK_AT_FREE.

> This one needs some head scratching. E.g. Is it correct for all compound
> pages. I believe it should be but few words on why would be better.

It seems nothing special here when compared with other places that
follow the same pattern. So I'm not sure what the concern is.

> > -			del_page_from_lru_list(page, lruvec, lru);
> > +			del_page_from_lru_list(page, lruvec, page_off_lru(page));
> >  
> >  			if (unlikely(PageCompound(page))) {
> >  				spin_unlock_irq(&pgdat->lru_lock);
> > @@ -1878,6 +1872,7 @@ static unsigned noinline_for_stack move_pages_to_lru(struct lruvec *lruvec,
> >  			} else
> >  				list_add(&page->lru, &pages_to_free);
> >  		} else {
> > +			nr_pages = thp_nr_pages(page);
> >  			nr_moved += nr_pages;
> >  			if (PageActive(page))
> >  				workingset_age_nonresident(lruvec, nr_pages);
> > -- 
> > 2.28.0.402.g5ffc5be6b7-goog
> > 
> 
> -- 
> Michal Hocko
> SUSE Labs