Re: [PATCH] mm: test page->flags directly in page_lru()

From: Matthew Wilcox <willy@infradead.org>
To: Yu Zhao <yuzhao@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	vbabka@suse.cz, alex.shi@linux.alibaba.com, guro@fb.com,
	hannes@cmpxchg.org, hughd@google.com,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	mhocko@kernel.org, vdavydov.dev@gmail.com
Subject: Re: [PATCH] mm: test page->flags directly in page_lru()
Date: Thu, 25 Feb 2021 12:12:07 +0000	[thread overview]
Message-ID: <20210225121207.GY2858050@casper.infradead.org> (raw)
In-Reply-To: <YDc0Hqk+A4wvN7jg@google.com>

On Wed, Feb 24, 2021 at 10:22:38PM -0700, Yu Zhao wrote:
> On Thu, Feb 25, 2021 at 03:55:53AM +0000, Matthew Wilcox wrote:
> > On Wed, Feb 24, 2021 at 04:50:39PM -0700, Yu Zhao wrote:
> > > Let me work out something *conceptually* smaller first, and if you
> > > think folio is absolutely more suitable even for this specific issue,
> > > I'll go review and test the four patches you listed. Sounds good?
> > 
> > Umm.  It seems to me that no matter what you do, it'll be equivalent to
> > this, only without the type-safety?
> 
> I'm thinking about something trivial but still very effective. So far
> I've only tested it with PG_{active,unevictable}, and I'm already
> seeing a 4KB gain less the 2KB loss from page_lru().
> 
> I didn't go with this at the beginning because it's also time-
> consuming. I need to go over every single use of
> PG_{active,unevictable,swapbacked,lru}.

Well, yes.  If you went with the folio, it'd also be typesafe.
What you've done here makes it a runtime error, and it's only detected
if you enable CONFIG_DEBUG_VM_PGFLAGS, which people don't do, in general.

> +++ b/fs/proc/task_mmu.c
> @@ -1712,6 +1712,7 @@ static void gather_stats(struct page *page, struct numa_maps *md, int pte_dirty,
>                         unsigned long nr_pages)
>  {
>         int count = page_mapcount(page);
> +       struct page *head = compound_head(page);
>  
>         md->pages += nr_pages;
>         if (pte_dirty || PageDirty(page))

... if you went full-on folio in this function, you could also make this
FolioDirty, saving another call to compound_head.

> @@ -1720,7 +1721,7 @@ static void gather_stats(struct page *page, struct numa_maps *md, int pte_dirty,
>         if (PageSwapCache(page))
... ditto ...
>                 md->swapcache += nr_pages;
>  
> -       if (PageActive(page) || PageUnevictable(page))
> +       if (PageActive(head) || PageUnevictable(head))
>                 md->active += nr_pages;
>  
>         if (PageWriteback(page))
... ditto...