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

[Yu Zhao]

On Fri, Sep 04, 2020 at 12:50:01PM +0200, Michal Hocko wrote:
> On Thu 03-09-20 21:24:00, Yu Zhao wrote:
> > 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.
> 
> No, I was referring to __ClearPageActive. You are right that this is
> cleared in page_off_lru but that is hidden in a release path and e.g.
> compound pages are released via their destructor which for some might
> not involve releasing the page - e.g. hugetlb pages. This should be fine
> because hugetlb pages are not on LRU so as I've said this is fine but it
> belongs to its own patch because it is not a pure mechanical change like
> the rest of the patch.

Please bear with me. This is the change in question:

 		if (put_page_testzero(page)) {
 			__ClearPageLRU(page);
-			__ClearPageActive(page);
-			del_page_from_lru_list(page, lruvec, lru);
+			del_page_from_lru_list(page, lruvec, page_off_lru(page));

 			if (unlikely(PageCompound(page))) {

If the PageUnevictable() check in page_off_lru() is not a concern,
I'm trying to understand what else is different between them:

	Before this path:		After this patch:

					page_off_lru()
	__ClearPageActive()			__ClearPageActive()
	add_page_to_lru_list()		add_page_to_lru_list()

And why is page_off_lru() hidden in a release path?