diff mbox series

[v2,2/3] mm: use PF_NO_TAIL for PG_lru

Message ID 20210226091718.2927291-3-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
Trying to set or clear PG_lru on tail pages has been considered buggy.
Enforce this rule by changing the policy for PG_lru from PF_HEAD to
PF_NO_TAIL. This means setting or clearing PG_lru on tail pages won't
be "corrected" by compound_page(). Such "correction" isn't helpful --
even if a piece of buggy code has gotten away with
compound_page(tail)->flags, it will run into trouble with lru list
addition and deletion because they use tail->lru rather than
compound_page(tail)->lru.

bloat-o-meter result:
  add/remove: 0/0 grow/shrink: 0/11 up/down: 0/-535 (-535)

Signed-off-by: Yu Zhao <yuzhao@google.com>
---
 include/linux/page-flags.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Yu Zhao Feb. 26, 2021, 8:22 p.m. UTC | #1
On Fri, Feb 26, 2021 at 02:17:17AM -0700, Yu Zhao wrote:
> Trying to set or clear PG_lru on tail pages has been considered buggy.
> Enforce this rule by changing the policy for PG_lru from PF_HEAD to
> PF_NO_TAIL. This means setting or clearing PG_lru on tail pages won't
> be "corrected" by compound_page(). Such "correction" isn't helpful --
> even if a piece of buggy code has gotten away with
> compound_page(tail)->flags, it will run into trouble with lru list
> addition and deletion because they use tail->lru rather than
> compound_page(tail)->lru.
> 
> bloat-o-meter result:
>   add/remove: 0/0 grow/shrink: 0/11 up/down: 0/-535 (-535)
> 
> Signed-off-by: Yu Zhao <yuzhao@google.com>
> ---
>  include/linux/page-flags.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> index 1995208a3763..c9626e54df8d 100644
> --- a/include/linux/page-flags.h
> +++ b/include/linux/page-flags.h
> @@ -333,8 +333,8 @@ PAGEFLAG(Referenced, referenced, PF_HEAD)
>  	__SETPAGEFLAG(Referenced, referenced, PF_HEAD)
>  PAGEFLAG(Dirty, dirty, PF_HEAD) TESTSCFLAG(Dirty, dirty, PF_HEAD)
>  	__CLEARPAGEFLAG(Dirty, dirty, PF_HEAD)
> -PAGEFLAG(LRU, lru, PF_HEAD) __CLEARPAGEFLAG(LRU, lru, PF_HEAD)
> -	TESTCLEARFLAG(LRU, lru, PF_HEAD)

As a side note, IMO, testing PG_lru on compound_head(tail)->flags is
a bug because it defeats the purpose of the following pattern when,
e.g., racing with compound page creations.

This pattern is intended to avoid dirtying struct page cache line when
scanning PFNs speculatively in isolate_migratepages_block() and
page_idle_get_page(). Without compound_head(), it works well when it
misses head pages. But with compound_head(), get_page_unless_zero()
will run unnecessarily on tail pages.

  if (!PageLRU(page) || !get_page_unless_zero(page))
    continue;

  if (!PageLRU(page)) {
    put_page(page);
    continue;
  }

  do_something();
diff mbox series

Patch

diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index 1995208a3763..c9626e54df8d 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -333,8 +333,8 @@  PAGEFLAG(Referenced, referenced, PF_HEAD)
 	__SETPAGEFLAG(Referenced, referenced, PF_HEAD)
 PAGEFLAG(Dirty, dirty, PF_HEAD) TESTSCFLAG(Dirty, dirty, PF_HEAD)
 	__CLEARPAGEFLAG(Dirty, dirty, PF_HEAD)
-PAGEFLAG(LRU, lru, PF_HEAD) __CLEARPAGEFLAG(LRU, lru, PF_HEAD)
-	TESTCLEARFLAG(LRU, lru, 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(Workingset, workingset, PF_HEAD)