Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 26 Feb 2021 13:22:08 -0700
From:   Yu Zhao <yuzhao@google.com>
To:     akpm@linux-foundation.org, alex.shi@linux.alibaba.com,
        vbabka@suse.cz, willy@infradead.org,
        "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Cc:     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 v2 2/3] mm: use PF_NO_TAIL for PG_lru
Message-ID: <YDlYcB7NyNG4N3c6@google.com>
References: <20210224084807.2179942-1-yuzhao@google.com>
 <20210226091718.2927291-1-yuzhao@google.com>
 <20210226091718.2927291-3-yuzhao@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210226091718.2927291-3-yuzhao@google.com>
Precedence: bulk

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();