[PATCH v2 0/5] Make PageWriteback use the PageLocked optimisation

[PATCH v2 0/5] Make PageWriteback use the PageLocked optimisation

[Matthew Wilcox]

From: "Matthew Wilcox (Oracle)" <willy@infradead.org>

PageWaiters is used by PageWriteback and PageLocked (and no other page
flags), so it makes sense to use the same codepaths that have already been
optimised for PageLocked, even if there's probably no real performance
benefit to be had.

v2: Rebased to 5.7-rc1
 - Split up patches better
 - Moved the BUG() from end_page_writeback() to __clear_page_writeback()
   as requested by Jan Kara.
 - Converted the BUG() to WARN_ON()
 - Removed TestClearPageWriteback

Matthew Wilcox (Oracle) (5):
  mm: Remove definition of clear_bit_unlock_is_negative_byte
  mm: Move PG_writeback into the bottom byte
  mm: Convert writeback BUG to WARN_ON
  mm: Use clear_bit_unlock_is_negative_byte for PageWriteback
  mm: Remove TestClearPageWriteback

 include/linux/page-flags.h |  8 ++++----
 mm/filemap.c               | 41 +++++--------------------------------
 mm/page-writeback.c        | 42 +++++++++++++++++++++++---------------
 3 files changed, 34 insertions(+), 57 deletions(-)

-- 
2.25.1

[PATCH v2 1/5] mm: Remove definition of clear_bit_unlock_is_negative_byte

[Matthew Wilcox]

From: "Matthew Wilcox (Oracle)" <willy@infradead.org>

This local definition hasn't been used since commit 84c6591103db
("locking/atomics, asm-generic/bitops/lock.h: Rewrite using
atomic_fetch_*()") which provided a default definition.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: William Kucharski <william.kucharski@oracle.com>
Cc: Will Deacon <will@kernel.org>
---
 mm/filemap.c | 23 -----------------------
 1 file changed, 23 deletions(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index 23a051a7ef0f..e475117e89eb 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -1248,29 +1248,6 @@ void add_page_wait_queue(struct page *page, wait_queue_entry_t *waiter)
 }
 EXPORT_SYMBOL_GPL(add_page_wait_queue);
 
-#ifndef clear_bit_unlock_is_negative_byte
-
-/*
- * PG_waiters is the high bit in the same byte as PG_lock.
- *
- * On x86 (and on many other architectures), we can clear PG_lock and
- * test the sign bit at the same time. But if the architecture does
- * not support that special operation, we just do this all by hand
- * instead.
- *
- * The read of PG_waiters has to be after (or concurrently with) PG_locked
- * being cleared, but a memory barrier should be unneccssary since it is
- * in the same byte as PG_locked.
- */
-static inline bool clear_bit_unlock_is_negative_byte(long nr, volatile void *mem)
-{
-	clear_bit_unlock(nr, mem);
-	/* smp_mb__after_atomic(); */
-	return test_bit(PG_waiters, mem);
-}
-
-#endif
-
 /**
  * unlock_page - unlock a locked page
  * @page: the page
-- 
2.25.1

[PATCH v2 2/5] mm: Move PG_writeback into the bottom byte

[Matthew Wilcox]

From: "Matthew Wilcox (Oracle)" <willy@infradead.org>

By placing PG_writeback in the bottom byte along with PG_locked and
PG_waiters, we will be able to use the same optimisation as PG_locked
in a subsequent patch.  Add BUILD_BUGs to ensure that all three bits
stay in the bottom byte.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: William Kucharski <william.kucharski@oracle.com>
---
 include/linux/page-flags.h | 4 ++--
 mm/filemap.c               | 4 +++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index 222f6f7b2bb3..af7c0ff5f517 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -103,13 +103,14 @@
  */
 enum pageflags {
 	PG_locked,		/* Page is locked. Don't touch. */
+	PG_writeback,		/* Page is under writeback */
 	PG_referenced,
 	PG_uptodate,
 	PG_dirty,
 	PG_lru,
 	PG_active,
+	PG_waiters,		/* Page has waiters, check its waitqueue */
 	PG_workingset,
-	PG_waiters,		/* Page has waiters, check its waitqueue. Must be bit #7 and in the same byte as "PG_locked" */
 	PG_error,
 	PG_slab,
 	PG_owner_priv_1,	/* Owner use. If pagecache, fs may use*/
@@ -117,7 +118,6 @@ enum pageflags {
 	PG_reserved,
 	PG_private,		/* If pagecache, has fs-private data */
 	PG_private_2,		/* If pagecache, has fs aux data */
-	PG_writeback,		/* Page is under writeback */
 	PG_head,		/* A head page */
 	PG_mappedtodisk,	/* Has blocks allocated on-disk */
 	PG_reclaim,		/* To be reclaimed asap */
diff --git a/mm/filemap.c b/mm/filemap.c
index e475117e89eb..b7c5d2402370 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -1266,6 +1266,7 @@ EXPORT_SYMBOL_GPL(add_page_wait_queue);
 void unlock_page(struct page *page)
 {
 	BUILD_BUG_ON(PG_waiters != 7);
+	BUILD_BUG_ON(PG_locked > 7);
 	page = compound_head(page);
 	VM_BUG_ON_PAGE(!PageLocked(page), page);
 	if (clear_bit_unlock_is_negative_byte(PG_locked, &page->flags))
@@ -1279,12 +1280,13 @@ EXPORT_SYMBOL(unlock_page);
  */
 void end_page_writeback(struct page *page)
 {
+	BUILD_BUG_ON(PG_writeback > 7);
 	/*
 	 * TestClearPageReclaim could be used here but it is an atomic
 	 * operation and overkill in this particular case. Failing to
 	 * shuffle a page marked for immediate reclaim is too mild to
 	 * justify taking an atomic operation penalty at the end of
-	 * ever page writeback.
+	 * every page writeback.
 	 */
 	if (PageReclaim(page)) {
 		ClearPageReclaim(page);
-- 
2.25.1

[PATCH v2 3/5] mm: Convert writeback BUG to WARN_ON

[Matthew Wilcox]

From: "Matthew Wilcox (Oracle)" <willy@infradead.org>

If this BUG() ever triggers, we'll have a dead system with no particular
information.  Dumping the page will give us a fighting chance of debugging
the problem, and I think it's safe for us to just continue if we try
to clear the writeback bit on a page which already has the writeback
bit clear.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 mm/filemap.c        | 4 +---
 mm/page-writeback.c | 5 +++++
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index b7c5d2402370..401b24d980ba 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -1293,9 +1293,7 @@ void end_page_writeback(struct page *page)
 		rotate_reclaimable_page(page);
 	}
 
-	if (!test_clear_page_writeback(page))
-		BUG();
-
+	test_clear_page_writeback(page);
 	smp_mb__after_atomic();
 	wake_up_page(page, PG_writeback);
 }
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index 7326b54ab728..ebaf0d8263a6 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -2718,6 +2718,11 @@ int test_clear_page_writeback(struct page *page)
 	struct lruvec *lruvec;
 	int ret;
 
+	if (WARN_ON(!PageWriteback(page))) {
+		dump_page(page, "!writeback");
+		return false;
+	}
+
 	memcg = lock_page_memcg(page);
 	lruvec = mem_cgroup_page_lruvec(page, page_pgdat(page));
 	if (mapping && mapping_use_writeback_tags(mapping)) {
-- 
2.25.1

[PATCH v2 4/5] mm: Use clear_bit_unlock_is_negative_byte for PageWriteback

[Matthew Wilcox]

From: "Matthew Wilcox (Oracle)" <willy@infradead.org>

We can use clear_bit_unlock_is_negative_byte() for writeback as well as
the lock bit.  wake_up_page() then has no more callers and can be removed.

Given the other code being executed between the clear and the test,
this is not going to be as dramatic a win as it was for PageLocked,
but symmetry between the two is nice and lets us remove some code.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: William Kucharski <william.kucharski@oracle.com>
---
 include/linux/page-flags.h |  2 +-
 mm/filemap.c               | 12 ++----------
 mm/page-writeback.c        | 37 ++++++++++++++++++++-----------------
 3 files changed, 23 insertions(+), 28 deletions(-)

diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index af7c0ff5f517..96c7d220c8cf 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -545,7 +545,7 @@ static __always_inline void SetPageUptodate(struct page *page)
 
 CLEARPAGEFLAG(Uptodate, uptodate, PF_NO_TAIL)
 
-int test_clear_page_writeback(struct page *page);
+bool __clear_page_writeback(struct page *page);
 int __test_set_page_writeback(struct page *page, bool keep_write);
 
 #define test_set_page_writeback(page)			\
diff --git a/mm/filemap.c b/mm/filemap.c
index 401b24d980ba..c704d333d3bf 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -1084,13 +1084,6 @@ static void wake_up_page_bit(struct page *page, int bit_nr)
 	spin_unlock_irqrestore(&q->lock, flags);
 }
 
-static void wake_up_page(struct page *page, int bit)
-{
-	if (!PageWaiters(page))
-		return;
-	wake_up_page_bit(page, bit);
-}
-
 /*
  * A choice of three behaviors for wait_on_page_bit_common():
  */
@@ -1293,9 +1286,8 @@ void end_page_writeback(struct page *page)
 		rotate_reclaimable_page(page);
 	}
 
-	test_clear_page_writeback(page);
-	smp_mb__after_atomic();
-	wake_up_page(page, PG_writeback);
+	if (__clear_page_writeback(page))
+		wake_up_page_bit(page, PG_writeback);
 }
 EXPORT_SYMBOL(end_page_writeback);
 
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index ebaf0d8263a6..d019d86fc21f 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -595,7 +595,7 @@ static void wb_domain_writeout_inc(struct wb_domain *dom,
 
 /*
  * Increment @wb's writeout completion count and the global writeout
- * completion count. Called from test_clear_page_writeback().
+ * completion count.
  */
 static inline void __wb_writeout_inc(struct bdi_writeback *wb)
 {
@@ -2711,12 +2711,19 @@ int clear_page_dirty_for_io(struct page *page)
 }
 EXPORT_SYMBOL(clear_page_dirty_for_io);
 
-int test_clear_page_writeback(struct page *page)
+#define clear_writeback_bit(page)	\
+	clear_bit_unlock_is_negative_byte(PG_writeback, &page->flags)
+
+/*
+ * The return value is whether there are waiters pending, not whether
+ * the flag was set.
+ */
+bool __clear_page_writeback(struct page *page)
 {
 	struct address_space *mapping = page_mapping(page);
 	struct mem_cgroup *memcg;
 	struct lruvec *lruvec;
-	int ret;
+	bool ret;
 
 	if (WARN_ON(!PageWriteback(page))) {
 		dump_page(page, "!writeback");
@@ -2731,16 +2738,14 @@ int test_clear_page_writeback(struct page *page)
 		unsigned long flags;
 
 		xa_lock_irqsave(&mapping->i_pages, flags);
-		ret = TestClearPageWriteback(page);
-		if (ret) {
-			__xa_clear_mark(&mapping->i_pages, page_index(page),
+		ret = clear_writeback_bit(page);
+		__xa_clear_mark(&mapping->i_pages, page_index(page),
 						PAGECACHE_TAG_WRITEBACK);
-			if (bdi_cap_account_writeback(bdi)) {
-				struct bdi_writeback *wb = inode_to_wb(inode);
+		if (bdi_cap_account_writeback(bdi)) {
+			struct bdi_writeback *wb = inode_to_wb(inode);
 
-				dec_wb_stat(wb, WB_WRITEBACK);
-				__wb_writeout_inc(wb);
-			}
+			dec_wb_stat(wb, WB_WRITEBACK);
+			__wb_writeout_inc(wb);
 		}
 
 		if (mapping->host && !mapping_tagged(mapping,
@@ -2749,7 +2754,7 @@ int test_clear_page_writeback(struct page *page)
 
 		xa_unlock_irqrestore(&mapping->i_pages, flags);
 	} else {
-		ret = TestClearPageWriteback(page);
+		ret = clear_writeback_bit(page);
 	}
 	/*
 	 * NOTE: Page might be free now! Writeback doesn't hold a page
@@ -2757,11 +2762,9 @@ int test_clear_page_writeback(struct page *page)
 	 * the clearing of PG_writeback. The below can only access
 	 * page state that is static across allocation cycles.
 	 */
-	if (ret) {
-		dec_lruvec_state(lruvec, NR_WRITEBACK);
-		dec_zone_page_state(page, NR_ZONE_WRITE_PENDING);
-		inc_node_page_state(page, NR_WRITTEN);
-	}
+	dec_lruvec_state(lruvec, NR_WRITEBACK);
+	dec_zone_page_state(page, NR_ZONE_WRITE_PENDING);
+	inc_node_page_state(page, NR_WRITTEN);
 	__unlock_page_memcg(memcg);
 	return ret;
 }
-- 
2.25.1

[PATCH v2 5/5] mm: Remove TestClearPageWriteback

[Matthew Wilcox]

From: "Matthew Wilcox (Oracle)" <willy@infradead.org>

The last commit removed the only caller of TestClearPageWriteback(),
so remove the definition.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 include/linux/page-flags.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index 96c7d220c8cf..c6ad04f4863c 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -365,7 +365,7 @@ PAGEFLAG(OwnerPriv1, owner_priv_1, PF_ANY)
  * risky: they bypass page accounting.
  */
 TESTPAGEFLAG(Writeback, writeback, PF_NO_TAIL)
-	TESTSCFLAG(Writeback, writeback, PF_NO_TAIL)
+	TESTSETFLAG(Writeback, writeback, PF_NO_TAIL)
 PAGEFLAG(MappedToDisk, mappedtodisk, PF_NO_TAIL)
 
 /* PG_readahead is only used for reads; PG_reclaim is only for writes */
-- 
2.25.1

Re: [PATCH v2 1/5] mm: Remove definition of clear_bit_unlock_is_negative_byte

[Will Deacon]

On Thu, Apr 16, 2020 at 08:46:02AM -0700, Matthew Wilcox wrote:
> From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
> 
> This local definition hasn't been used since commit 84c6591103db
> ("locking/atomics, asm-generic/bitops/lock.h: Rewrite using
> atomic_fetch_*()") which provided a default definition.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> Reviewed-by: William Kucharski <william.kucharski@oracle.com>
> Cc: Will Deacon <will@kernel.org>
> ---
>  mm/filemap.c | 23 -----------------------
>  1 file changed, 23 deletions(-)

Ok, for my own curiosity I tried building for Alpha because I couldn't for
the life of me figure it out, and behold:

mm/filemap.c: In function 'unlock_page':
mm/filemap.c:1271:6: error: implicit declaration of function 'clear_bit_unlock_is_negative_byte' [-Werror=implicit-function-declaration]
  if (clear_bit_unlock_is_negative_byte(PG_locked, &page->flags))
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
make[1]: *** [scripts/Makefile.build:267: mm/filemap.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1722: mm] Error 2
make: *** Waiting for unfinished jobs....

I had to enable CONFIG_SMP, so maybe the robot doesn't do that?

Anyway, it's somewhat reassuring that it broke, if not unfortunate at the same
time!

Will

Re: [PATCH v2 1/5] mm: Remove definition of clear_bit_unlock_is_negative_byte

[Matthew Wilcox]

On Thu, Apr 16, 2020 at 06:02:24PM +0100, Will Deacon wrote:
> On Thu, Apr 16, 2020 at 08:46:02AM -0700, Matthew Wilcox wrote:
> > From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
> > This local definition hasn't been used since commit 84c6591103db
> > ("locking/atomics, asm-generic/bitops/lock.h: Rewrite using
> > atomic_fetch_*()") which provided a default definition.
> 
> Ok, for my own curiosity I tried building for Alpha because I couldn't for
> the life of me figure it out, and behold:
> 
> mm/filemap.c: In function 'unlock_page':
> mm/filemap.c:1271:6: error: implicit declaration of function 'clear_bit_unlock_is_negative_byte' [-Werror=implicit-function-declaration]
>   if (clear_bit_unlock_is_negative_byte(PG_locked, &page->flags))
>       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: some warnings being treated as errors
> 
> I had to enable CONFIG_SMP, so maybe the robot doesn't do that?
> 
> Anyway, it's somewhat reassuring that it broke, if not unfortunate at the same
> time!

Thanks!  The robot says it built two alpha configs,
randconfig-a001-20200325 and defconfig.  I imagine neither has SMP set.

kbuild people, please can you add SMP and non-SMP options to the configs
you test?