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:   Tue, 10 Nov 2020 23:27:54 -0800 (PST)
From:   Hugh Dickins <hughd@google.com>
To:     Johannes Weiner <hannes@cmpxchg.org>
cc:     Alex Shi <alex.shi@linux.alibaba.com>, akpm@linux-foundation.org,
        mgorman@techsingularity.net, tj@kernel.org, hughd@google.com,
        khlebnikov@yandex-team.ru, daniel.m.jordan@oracle.com,
        willy@infradead.org, lkp@intel.com, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, cgroups@vger.kernel.org,
        shakeelb@google.com, iamjoonsoo.kim@lge.com,
        richard.weiyang@gmail.com, kirill@shutemov.name,
        alexander.duyck@gmail.com, rong.a.chen@intel.com, mhocko@suse.com,
        vdavydov.dev@gmail.com, shy828301@gmail.com,
        Vlastimil Babka <vbabka@suse.cz>,
        Minchan Kim <minchan@kernel.org>
Subject: Re: [PATCH v20 08/20] mm: page_idle_get_page() does not need
 lru_lock
In-Reply-To: <20201102144110.GB724984@cmpxchg.org>
Message-ID: <alpine.LSU.2.11.2011102244420.1183@eggly.anvils>
References: <1603968305-8026-1-git-send-email-alex.shi@linux.alibaba.com> <1603968305-8026-9-git-send-email-alex.shi@linux.alibaba.com> <20201102144110.GB724984@cmpxchg.org>
User-Agent: Alpine 2.11 (LSU 23 2013-08-11)
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Precedence: bulk

On Mon, 2 Nov 2020, Johannes Weiner wrote:
> On Thu, Oct 29, 2020 at 06:44:53PM +0800, Alex Shi wrote:
> > From: Hugh Dickins <hughd@google.com>
> > 
> > It is necessary for page_idle_get_page() to recheck PageLRU() after
> > get_page_unless_zero(), but holding lru_lock around that serves no
> > useful purpose, and adds to lru_lock contention: delete it.
> > 
> > See https://lore.kernel.org/lkml/20150504031722.GA2768@blaptop for the
> > discussion that led to lru_lock there; but __page_set_anon_rmap() now
> > uses WRITE_ONCE(),
> 
> That doesn't seem to be the case in Linus's or Andrew's tree. Am I
> missing a dependent patch series?

Sorry, I was out of action, then slower than ever, for a while.

Many thanks for calling out my falsehood there, Johannes.

What led me to write that?  It has baffled me, but at last I see:
this patch to page_idle_get_page() was 0002 in my lru_lock patchset
against v5.3 last year, and 0001 was the patch which made it true.
Then when I checked against mainline, I must have got confused by
the similar WRITE_ONCE in page_move_anon_rmap().

Appended below, but not rediffed, and let's not hold up Alex's set
for the rest of it: it is all theoretical until the kernel gets to
be built with a suitably malicious compiler; but I'll follow up
with a fresh version of the below after his set is safely in.

From a1abcbc2aac70c6ba47b8991992bb85b86b4a160 Mon Sep 17 00:00:00 2001
From: Hugh Dickins <hughd@google.com>
Date: Thu, 22 Aug 2019 15:49:44 -0700
Subject: [PATCH 1/9] mm: more WRITE_ONCE and READ_ONCE on page->mapping

v4.2 commit 414e2fb8ce5a ("rmap: fix theoretical race between do_wp_page
and shrink_active_list") added a WRITE_ONCE() where page_move_anon_rmap()
composes page->mapping from anon_vma pointer and PAGE_MAPPING_ANON.

Now do the same where __page_set_anon_rmap() does the same, and where
compaction.c applies PAGE_MAPPING_MOVABLE, and ksm.c PAGE_MAPPING_KSM.

rmap.c already uses READ_ONCE(page->mapping), but util.c should too:
add READ_ONCE() in page_rmapping(), page_anon_vma() and page_mapping().
Delete the then unused helper __page_rmapping().

I doubt that this commit fixes anything, but it's harmless and
unintrusive, and makes reasoning about page mapping flags easier.

What if a compiler implements "page->mapping = mapping" in other places
by, say, first assigning the odd bits of mapping, then adding in the
even bits?  Then we shall not build the kernel with such a compiler.

Signed-off-by: Hugh Dickins <hughd@google.com>
Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Yu Zhao <yuzhao@google.com>
Cc: Alex Shi <alex.shi@linux.alibaba.com>
---
 mm/compaction.c |  7 ++++---
 mm/ksm.c        |  2 +-
 mm/rmap.c       |  7 ++++++-
 mm/util.c       | 24 ++++++++++--------------
 4 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 952dc2fb24e5..c405f4362624 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -113,7 +113,8 @@ void __SetPageMovable(struct page *page, struct address_space *mapping)
 {
 	VM_BUG_ON_PAGE(!PageLocked(page), page);
 	VM_BUG_ON_PAGE((unsigned long)mapping & PAGE_MAPPING_MOVABLE, page);
-	page->mapping = (void *)((unsigned long)mapping | PAGE_MAPPING_MOVABLE);
+	WRITE_ONCE(page->mapping,
+		   (unsigned long)mapping | PAGE_MAPPING_MOVABLE);
 }
 EXPORT_SYMBOL(__SetPageMovable);
 
@@ -126,8 +127,8 @@ void __ClearPageMovable(struct page *page)
 	 * flag so that VM can catch up released page by driver after isolation.
 	 * With it, VM migration doesn't try to put it back.
 	 */
-	page->mapping = (void *)((unsigned long)page->mapping &
-				PAGE_MAPPING_MOVABLE);
+	WRITE_ONCE(page->mapping,
+		   (unsigned long)page->mapping & PAGE_MAPPING_MOVABLE);
 }
 EXPORT_SYMBOL(__ClearPageMovable);
 
diff --git a/mm/ksm.c b/mm/ksm.c
index 3dc4346411e4..426b6a40ea41 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -865,7 +865,7 @@ static inline struct stable_node *page_stable_node(struct page *page)
 static inline void set_page_stable_node(struct page *page,
 					struct stable_node *stable_node)
 {
-	page->mapping = (void *)((unsigned long)stable_node | PAGE_MAPPING_KSM);
+	WRITE_ONCE(page->mapping, (unsigned long)stable_node | PAGE_MAPPING_KSM);
 }
 
 #ifdef CONFIG_SYSFS
diff --git a/mm/rmap.c b/mm/rmap.c
index 003377e24232..9480df437edc 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1044,7 +1044,12 @@ static void __page_set_anon_rmap(struct page *page,
 		anon_vma = anon_vma->root;
 
 	anon_vma = (void *) anon_vma + PAGE_MAPPING_ANON;
-	page->mapping = (struct address_space *) anon_vma;
+	/*
+	 * Ensure that anon_vma and the PAGE_MAPPING_ANON bit are written
+	 * simultaneously, so a concurrent reader (eg page_referenced()'s
+	 * PageAnon()) will not see one without the other.
+	 */
+	WRITE_ONCE(page->mapping, (struct address_space *) anon_vma);
 	page->index = linear_page_index(vma, address);
 }
 
diff --git a/mm/util.c b/mm/util.c
index e6351a80f248..09b9fcbedac3 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -489,21 +489,14 @@ void kvfree(const void *addr)
 }
 EXPORT_SYMBOL(kvfree);
 
-static inline void *__page_rmapping(struct page *page)
-{
-	unsigned long mapping;
-
-	mapping = (unsigned long)page->mapping;
-	mapping &= ~PAGE_MAPPING_FLAGS;
-
-	return (void *)mapping;
-}
-
 /* Neutral page->mapping pointer to address_space or anon_vma or other */
 void *page_rmapping(struct page *page)
 {
+	unsigned long mapping;
+
 	page = compound_head(page);
-	return __page_rmapping(page);
+	mapping = (unsigned long)READ_ONCE(page->mapping);
+	return (void *)(mapping & ~PAGE_MAPPING_FLAGS);
 }
 
 /*
@@ -534,10 +527,11 @@ struct anon_vma *page_anon_vma(struct page *page)
 	unsigned long mapping;
 
 	page = compound_head(page);
-	mapping = (unsigned long)page->mapping;
+	mapping = (unsigned long)READ_ONCE(page->mapping);
+	/* Return NULL if file or PageMovable or PageKsm */
 	if ((mapping & PAGE_MAPPING_FLAGS) != PAGE_MAPPING_ANON)
 		return NULL;
-	return __page_rmapping(page);
+	return (struct anon_vma *)(mapping & ~PAGE_MAPPING_FLAGS);
 }
 
 struct address_space *page_mapping(struct page *page)
@@ -557,10 +551,12 @@ struct address_space *page_mapping(struct page *page)
 		return swap_address_space(entry);
 	}
 
-	mapping = page->mapping;
+	mapping = READ_ONCE(page->mapping);
+	/* Return NULL if PageAnon (including PageKsm) */
 	if ((unsigned long)mapping & PAGE_MAPPING_ANON)
 		return NULL;
 
+	/* Return struct address_space pointer if file or PageMovable */
 	return (void *)((unsigned long)mapping & ~PAGE_MAPPING_FLAGS);
 }
 EXPORT_SYMBOL(page_mapping);
-- 
2.23.0.187.g17f5b7556c-goog