diff mbox series

[v2,3/3] mm: use PF_ONLY_HEAD for PG_active and PG_unevictable

Message ID 20210226091718.2927291-4-yuzhao@google.com (mailing list archive)
State New
Headers show
Series trim the uses of compound_head() | expand

Commit Message

Yu Zhao Feb. 26, 2021, 9:17 a.m. UTC
All places but one test, set or clear PG_active and PG_unevictable on
small or head pages. Use compound_head() explicitly for that singleton
so the rest can rid of redundant compound_head().

bloat-o-meter result:
  add/remove: 0/0 grow/shrink: 3/38 up/down: 388/-4270 (-3882)

Signed-off-by: Yu Zhao <yuzhao@google.com>
---
 fs/proc/task_mmu.c         |  3 ++-
 include/linux/page-flags.h | 10 +++++-----
 2 files changed, 7 insertions(+), 6 deletions(-)

Comments

Matthew Wilcox Feb. 26, 2021, 12:13 p.m. UTC | #1
On Fri, Feb 26, 2021 at 02:17:18AM -0700, Yu Zhao wrote:
> All places but one test, set or clear PG_active and PG_unevictable on
> small or head pages. Use compound_head() explicitly for that singleton
> so the rest can rid of redundant compound_head().

How do you know it's only one place?  I really wish you'd work with me
on folios.  They make the compiler prove that it's not a tail page.
Yu Zhao Feb. 26, 2021, 7:49 p.m. UTC | #2
On Fri, Feb 26, 2021 at 12:13:14PM +0000, Matthew Wilcox wrote:
> On Fri, Feb 26, 2021 at 02:17:18AM -0700, Yu Zhao wrote:
> > All places but one test, set or clear PG_active and PG_unevictable on
> > small or head pages. Use compound_head() explicitly for that singleton
> > so the rest can rid of redundant compound_head().
> 
> How do you know it's only one place?  I really wish you'd work with me
> on folios.  They make the compiler prove that it's not a tail page.

I hasn't been following the effort closely, so I'm rereading the very
first discussion "Are THPs the right model for the pagecache?" and
then I need to rewatch the recorded Zoom meeting. As I said I'm on
board with the idea, but I can't create a roadmap based on my current
rough understanding, unless you prefer me to just randomly throw some
reviewed-bys at your patches in the next few days. (Our long-term plan
for folios is to support arbitrary anon page sizes because anon memory
is more than 90% of total user memory on Android, Chrome OS and in our
data centers.)

That said, if you have something ready to test, we could do it for you
in our runtime environments immediately.
Matthew Wilcox Feb. 26, 2021, 8:27 p.m. UTC | #3
On Fri, Feb 26, 2021 at 12:49:41PM -0700, Yu Zhao wrote:
> On Fri, Feb 26, 2021 at 12:13:14PM +0000, Matthew Wilcox wrote:
> > On Fri, Feb 26, 2021 at 02:17:18AM -0700, Yu Zhao wrote:
> > > All places but one test, set or clear PG_active and PG_unevictable on
> > > small or head pages. Use compound_head() explicitly for that singleton
> > > so the rest can rid of redundant compound_head().
> > 
> > How do you know it's only one place?  I really wish you'd work with me
> > on folios.  They make the compiler prove that it's not a tail page.
> 
> I hasn't been following the effort closely, so I'm rereading the very
> first discussion "Are THPs the right model for the pagecache?" and
> then I need to rewatch the recorded Zoom meeting. As I said I'm on
> board with the idea, but I can't create a roadmap based on my current
> rough understanding, unless you prefer me to just randomly throw some
> reviewed-bys at your patches in the next few days. (Our long-term plan
> for folios is to support arbitrary anon page sizes because anon memory
> is more than 90% of total user memory on Android, Chrome OS and in our
> data centers.)
> 
> That said, if you have something ready to test, we could do it for you
> in our runtime environments immediately.

I don't have anything ready to test for anonymous memory; indeed I have no
plans to work on anonymous memory myself.  My focus is on the page cache.

But, once we get the folio _concept_ into the kernel (ie a struct page
which is definitely not a tail page), it can be used more widely than
the page cache, and independently from anything I'm working on.  The
biggest risk is that we end up duplicating work ...
Kirill A. Shutemov March 1, 2021, 11:50 a.m. UTC | #4
On Fri, Feb 26, 2021 at 12:13:14PM +0000, Matthew Wilcox wrote:
> On Fri, Feb 26, 2021 at 02:17:18AM -0700, Yu Zhao wrote:
> > All places but one test, set or clear PG_active and PG_unevictable on
> > small or head pages. Use compound_head() explicitly for that singleton
> > so the rest can rid of redundant compound_head().
> 
> How do you know it's only one place?  I really wish you'd work with me
> on folios.  They make the compiler prove that it's not a tail page.

+1 to this.

The problem with compound_head() is systemic and ad-hoc solution to few
page flags will only complicate the picture.
Yu Zhao March 1, 2021, 7:58 p.m. UTC | #5
On Mon, Mar 01, 2021 at 02:50:07PM +0300, Kirill A. Shutemov wrote:
> On Fri, Feb 26, 2021 at 12:13:14PM +0000, Matthew Wilcox wrote:
> > On Fri, Feb 26, 2021 at 02:17:18AM -0700, Yu Zhao wrote:
> > > All places but one test, set or clear PG_active and PG_unevictable on
> > > small or head pages. Use compound_head() explicitly for that singleton
> > > so the rest can rid of redundant compound_head().
> > 
> > How do you know it's only one place?  I really wish you'd work with me
> > on folios.  They make the compiler prove that it's not a tail page.
> 
> +1 to this.
> 
> The problem with compound_head() is systemic and ad-hoc solution to few
> page flags will only complicate the picture.

Well, I call it an incremental improvement, and how exactly does it
complicate the picture?

I see your point: you prefer a complete replacement. But my point is
not about the preference; it's about presenting an option: I'm not
saying we have to go with this series; I'm saying if you don't want
to wait, here is something quick but not perfect.
Hugh Dickins March 1, 2021, 8:16 p.m. UTC | #6
On Mon, 1 Mar 2021, Yu Zhao wrote:
> On Mon, Mar 01, 2021 at 02:50:07PM +0300, Kirill A. Shutemov wrote:
> > On Fri, Feb 26, 2021 at 12:13:14PM +0000, Matthew Wilcox wrote:
> > > On Fri, Feb 26, 2021 at 02:17:18AM -0700, Yu Zhao wrote:
> > > > All places but one test, set or clear PG_active and PG_unevictable on
> > > > small or head pages. Use compound_head() explicitly for that singleton
> > > > so the rest can rid of redundant compound_head().
> > > 
> > > How do you know it's only one place?  I really wish you'd work with me
> > > on folios.  They make the compiler prove that it's not a tail page.
> > 
> > +1 to this.
> > 
> > The problem with compound_head() is systemic and ad-hoc solution to few
> > page flags will only complicate the picture.
> 
> Well, I call it an incremental improvement, and how exactly does it
> complicate the picture?
> 
> I see your point: you prefer a complete replacement. But my point is
> not about the preference; it's about presenting an option: I'm not
> saying we have to go with this series; I'm saying if you don't want
> to wait, here is something quick but not perfect.

+1 to this.

Hugh
Matthew Wilcox March 1, 2021, 8:26 p.m. UTC | #7
On Mon, Mar 01, 2021 at 12:16:19PM -0800, Hugh Dickins wrote:
> On Mon, 1 Mar 2021, Yu Zhao wrote:
> > On Mon, Mar 01, 2021 at 02:50:07PM +0300, Kirill A. Shutemov wrote:
> > > On Fri, Feb 26, 2021 at 12:13:14PM +0000, Matthew Wilcox wrote:
> > > > On Fri, Feb 26, 2021 at 02:17:18AM -0700, Yu Zhao wrote:
> > > > > All places but one test, set or clear PG_active and PG_unevictable on
> > > > > small or head pages. Use compound_head() explicitly for that singleton
> > > > > so the rest can rid of redundant compound_head().
> > > > 
> > > > How do you know it's only one place?  I really wish you'd work with me
> > > > on folios.  They make the compiler prove that it's not a tail page.
> > > 
> > > +1 to this.
> > > 
> > > The problem with compound_head() is systemic and ad-hoc solution to few
> > > page flags will only complicate the picture.
> > 
> > Well, I call it an incremental improvement, and how exactly does it
> > complicate the picture?
> > 
> > I see your point: you prefer a complete replacement. But my point is
> > not about the preference; it's about presenting an option: I'm not
> > saying we have to go with this series; I'm saying if you don't want
> > to wait, here is something quick but not perfect.
> 
> +1 to this.

page folios are here and ready to go.  I'm doing another pass on them,
quantifying the improvements to text with each patch.  So far I'm
at 4357 bytes of text saved, in the first 10 patches (many of which
look as if they're not going to produce any savings).

Yu Zhao's patches seem risky.  The only way to know if any places have
been missed is by enabling CONFIG_DEBUG_VM_PGFLAGS, which we do not
recommend for production environments.
diff mbox series

Patch

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 3cec6fbef725..c866c363bb41 100644
--- a/fs/proc/task_mmu.c
+++ 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))
@@ -1720,7 +1721,7 @@  static void gather_stats(struct page *page, struct numa_maps *md, int pte_dirty,
 	if (PageSwapCache(page))
 		md->swapcache += nr_pages;
 
-	if (PageActive(page) || PageUnevictable(page))
+	if (PageActive(head) || PageUnevictable(head))
 		md->active += nr_pages;
 
 	if (PageWriteback(page))
diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index c9626e54df8d..b7fe855a6ee9 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -335,8 +335,8 @@  PAGEFLAG(Dirty, dirty, PF_HEAD) TESTSCFLAG(Dirty, dirty, PF_HEAD)
 	__CLEARPAGEFLAG(Dirty, dirty, PF_HEAD)
 PAGEFLAG(LRU, lru, PF_NO_TAIL) __CLEARPAGEFLAG(LRU, lru, PF_NO_TAIL)
 	TESTCLEARFLAG(LRU, lru, PF_NO_TAIL)
-PAGEFLAG(Active, active, PF_HEAD) __CLEARPAGEFLAG(Active, active, PF_HEAD)
-	TESTCLEARFLAG(Active, active, PF_HEAD)
+PAGEFLAG(Active, active, PF_ONLY_HEAD) __CLEARPAGEFLAG(Active, active, PF_ONLY_HEAD)
+	TESTCLEARFLAG(Active, active, PF_ONLY_HEAD)
 PAGEFLAG(Workingset, workingset, PF_HEAD)
 	TESTCLEARFLAG(Workingset, workingset, PF_HEAD)
 __PAGEFLAG(Slab, slab, PF_NO_TAIL)
@@ -407,9 +407,9 @@  CLEARPAGEFLAG(SwapCache, swapcache, PF_NO_TAIL)
 PAGEFLAG_FALSE(SwapCache)
 #endif
 
-PAGEFLAG(Unevictable, unevictable, PF_HEAD)
-	__CLEARPAGEFLAG(Unevictable, unevictable, PF_HEAD)
-	TESTCLEARFLAG(Unevictable, unevictable, PF_HEAD)
+PAGEFLAG(Unevictable, unevictable, PF_ONLY_HEAD)
+	__CLEARPAGEFLAG(Unevictable, unevictable, PF_ONLY_HEAD)
+	TESTCLEARFLAG(Unevictable, unevictable, PF_ONLY_HEAD)
 
 #ifdef CONFIG_MMU
 PAGEFLAG(Mlocked, mlocked, PF_NO_TAIL)