linux-kernel - Re: [PATCH v2 1/2] mm: use add_page_to_lru_list()/page_lru()/page_off

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20200904165437.GA77761@google.com>
Date:   Fri, 4 Sep 2020 10:54:37 -0600
From:   Yu Zhao <yuzhao@...gle.com>
To:     Michal Hocko <mhocko@...e.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Alex Shi <alex.shi@...ux.alibaba.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] mm: use
 add_page_to_lru_list()/page_lru()/page_off_lru()

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?