[PATCH v3 00/11] Make PageWriteback use the PageLocked optimisation

[PATCH v3 00/11] 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.

Unfortunately, clear_bit_unlock_is_negative_byte() isn't present on every
architecture, and the default implementation is only available in filemap.c
while I want to use it in page-writeback.c.  Rather than move the default
implementation to a header file, I've done optimised implementations for
alpha and ia64.  I can't figure out optimised implementations for m68k,
mips, riscv and s390, so I've just replicated the effect of the generic
implementation in them.  I leave it to the experts to fix that (... or
convert over to using asm-generic/bitops/lock.h ...)

v3:
 - Added implementations of clear_bit_unlock_is_negative_byte()
   to architectures which need it

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) (11):
  alpha: Add clear_bit_unlock_is_negative_byte implementation
  ia64: Add clear_bit_unlock_is_negative_byte implementation
  m68k: Add clear_bit_unlock_is_negative_byte implementation
  mips: Add clear_bit_unlock_is_negative_byte implementation
  riscv: Add clear_bit_unlock_is_negative_byte implementation
  s390: Add clear_bit_unlock_is_negative_byte implementation
  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

 arch/alpha/include/asm/bitops.h | 23 ++++++++++++++++++
 arch/ia64/include/asm/bitops.h  | 20 ++++++++++++++++
 arch/m68k/include/asm/bitops.h  |  7 ++++++
 arch/mips/include/asm/bitops.h  |  7 ++++++
 arch/riscv/include/asm/bitops.h |  7 ++++++
 arch/s390/include/asm/bitops.h  |  9 +++++++
 include/linux/page-flags.h      |  8 +++----
 mm/filemap.c                    | 41 ++++----------------------------
 mm/page-writeback.c             | 42 ++++++++++++++++++++-------------
 9 files changed, 107 insertions(+), 57 deletions(-)

-- 
2.25.1

[PATCH v3 01/11] alpha: Add clear_bit_unlock_is_negative_byte implementation

[Matthew Wilcox]

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

Copy and paste the clear_bit_unlock() implementation, and test the temp
variable to see if it has bit 7 set.  Saves two instructions: a load
and a compare:

      860:      01 31 20 44     andnot  t0,0x1,t0
      864:      00 00 30 b8     stl_c   t0,0(a0)
      868:      67 1b 20 e4     beq     t0,7608 <generic_file_write_iter+0x218>
-     86c:      00 00 30 a0     ldl     t0,0(a0)
-     870:      01 10 30 44     and     t0,0x80,t0
-     874:      a1 03 e1 43     cmpult  zero,t0,t0
-     878:      01 00 20 f4     bne     t0,880 <unlock_page+0x40>
-     87c:      01 80 fa 6b     ret
+     86c:      01 10 30 44     and     t0,0x80,t0
+     870:      03 00 20 f4     bne     t0,880 <unlock_page+0x40>
+     874:      01 80 fa 6b     ret

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
Cc: Matt Turner <mattst88@gmail.com>
Cc: linux-alpha@vger.kernel.org
---
 arch/alpha/include/asm/bitops.h | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/arch/alpha/include/asm/bitops.h b/arch/alpha/include/asm/bitops.h
index 5adca78830b5..f9af2401bd23 100644
--- a/arch/alpha/include/asm/bitops.h
+++ b/arch/alpha/include/asm/bitops.h
@@ -79,6 +79,29 @@ clear_bit_unlock(unsigned long nr, volatile void * addr)
 	clear_bit(nr, addr);
 }
 
+static inline bool clear_bit_unlock_is_negative_byte(unsigned int nr,
+						     volatile unsigned long *p)
+{
+	unsigned long temp;
+	int *m = ((int *)p) + (nr >> 5);
+
+	smp_mb();
+	__asm__ __volatile__(
+	"1:	ldl_l %0,%3\n"
+	"	bic %0,%2,%0\n"
+	"	stl_c %0,%1\n"
+	"	beq %0,2f\n"
+	".subsection 2\n"
+	"2:	br 1b\n"
+	".previous"
+	:"=&r" (temp), "=m" (*m)
+	:"Ir" (1UL << (nr & 31)), "m" (*m));
+
+	return temp & 128;
+}
+#define clear_bit_unlock_is_negative_byte \
+	clear_bit_unlock_is_negative_byte
+
 /*
  * WARNING: non atomic version.
  */
-- 
2.25.1

[PATCH v3 02/11] ia64: Add clear_bit_unlock_is_negative_byte implementation

[Matthew Wilcox]

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

Copy and paste the clear_bit_unlock() implementation, and test the old
variable to see if it has bit 7 set.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: linux-ia64@vger.kernel.org
---
 arch/ia64/include/asm/bitops.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/ia64/include/asm/bitops.h b/arch/ia64/include/asm/bitops.h
index 2f24ee6459d2..ba92ca44731b 100644
--- a/arch/ia64/include/asm/bitops.h
+++ b/arch/ia64/include/asm/bitops.h
@@ -117,6 +117,26 @@ clear_bit_unlock (int nr, volatile void *addr)
 	} while (cmpxchg_rel(m, old, new) != old);
 }
 
+static inline bool clear_bit_unlock_is_negative_byte(unsigned int nr,
+						volatile unsigned long *p)
+{
+	__u32 mask, old, new;
+	volatile __u32 *m;
+	CMPXCHG_BUGCHECK_DECL
+
+	m = (volatile __u32 *) addr + (nr >> 5);
+	mask = ~(1 << (nr & 31));
+	do {
+		CMPXCHG_BUGCHECK(m);
+		old = *m;
+		new = old & mask;
+	} while (cmpxchg_rel(m, old, new) != old);
+
+	return old & (1 << 7);
+}
+#define clear_bit_unlock_is_negative_byte \
+	clear_bit_unlock_is_negative_byte
+
 /**
  * __clear_bit_unlock - Non-atomically clears a bit in memory with release
  * @nr: Bit to clear
-- 
2.25.1

[PATCH v3 03/11] m68k: Add clear_bit_unlock_is_negative_byte implementation

[Matthew Wilcox]

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

This is the generic implementation.  Someone who knows m68k assembly
can probably do better (the BFCLR instruction appears to set the N bit
appropriately).

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: linux-m68k@lists.linux-m68k.org
---
 arch/m68k/include/asm/bitops.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/m68k/include/asm/bitops.h b/arch/m68k/include/asm/bitops.h
index 10133a968c8e..bfb34e0748fe 100644
--- a/arch/m68k/include/asm/bitops.h
+++ b/arch/m68k/include/asm/bitops.h
@@ -524,6 +524,13 @@ static inline int __fls(int x)
 #define clear_bit_unlock	clear_bit
 #define __clear_bit_unlock	clear_bit_unlock
 
+static inline bool clear_bit_unlock_is_negative_byte(unsigned int nr,
+						volatile unsigned long *p)
+{
+	clear_bit_unlock(nr, p);
+	return test_bit(7, p);
+}
+
 #include <asm-generic/bitops/ext2-atomic.h>
 #include <asm-generic/bitops/le.h>
 #include <asm-generic/bitops/fls64.h>
-- 
2.25.1

[PATCH v3 04/11] mips: Add clear_bit_unlock_is_negative_byte implementation

[Matthew Wilcox]

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

This is the generic implementation.  I can't figure out an optimised
implementation for mips.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: linux-mips@vger.kernel.org
---
 arch/mips/include/asm/bitops.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/mips/include/asm/bitops.h b/arch/mips/include/asm/bitops.h
index a74769940fbd..23e9d36c2ffc 100644
--- a/arch/mips/include/asm/bitops.h
+++ b/arch/mips/include/asm/bitops.h
@@ -147,6 +147,13 @@ static inline void clear_bit_unlock(unsigned long nr, volatile unsigned long *ad
 	clear_bit(nr, addr);
 }
 
+static inline bool clear_bit_unlock_is_negative_byte(unsigned int nr,
+						volatile unsigned long *p)
+{
+	clear_bit_unlock(nr, p);
+	return test_bit(7, p);
+}
+
 /*
  * change_bit - Toggle a bit in memory
  * @nr: Bit to change
-- 
2.25.1

[PATCH v3 05/11] riscv: Add clear_bit_unlock_is_negative_byte implementation

[Matthew Wilcox]

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

This is the generic implementation.  I can't figure out an optimised
implementation for riscv.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Cc: Paul Walmsley <paul.walmsley@sifive.com>
Cc: Palmer Dabbelt <palmer@dabbelt.com>
Cc: Albert Ou <aou@eecs.berkeley.edu>
Cc: linux-riscv@lists.infradead.org
---
 arch/riscv/include/asm/bitops.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/riscv/include/asm/bitops.h b/arch/riscv/include/asm/bitops.h
index 396a3303c537..f2060c126a34 100644
--- a/arch/riscv/include/asm/bitops.h
+++ b/arch/riscv/include/asm/bitops.h
@@ -171,6 +171,13 @@ static inline void clear_bit_unlock(
 	__op_bit_ord(and, __NOT, nr, addr, .rl);
 }
 
+static inline bool clear_bit_unlock_is_negative_byte(unsigned int nr,
+						volatile unsigned long *p)
+{
+	clear_bit_unlock(nr, p);
+	return test_bit(7, p);
+}
+
 /**
  * __clear_bit_unlock - Clear a bit in memory, for unlock
  * @nr: the bit to set
-- 
2.25.1

[PATCH v3 06/11] s390: Add clear_bit_unlock_is_negative_byte implementation

[Matthew Wilcox]

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

This is the generic implementation.  I can't figure out an optimised
implementation for s390.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Vasily Gorbik <gor@linux.ibm.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: linux-s390@vger.kernel.org
---
 arch/s390/include/asm/bitops.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/s390/include/asm/bitops.h b/arch/s390/include/asm/bitops.h
index 431e208a5ea4..d313ca6affaf 100644
--- a/arch/s390/include/asm/bitops.h
+++ b/arch/s390/include/asm/bitops.h
@@ -241,6 +241,15 @@ static inline void arch___clear_bit_unlock(unsigned long nr,
 	arch___clear_bit(nr, ptr);
 }
 
+static inline bool arch_clear_bit_unlock_is_negative_byte(unsigned int nr,
+						volatile unsigned long *p)
+{
+	arch_clear_bit_unlock(nr, p);
+	return arch_test_bit(7, p);
+}
+#define arch_clear_bit_unlock_is_negative_byte \
+	arch_clear_bit_unlock_is_negative_byte
+
 #include <asm-generic/bitops/instrumented-atomic.h>
 #include <asm-generic/bitops/instrumented-non-atomic.h>
 #include <asm-generic/bitops/instrumented-lock.h>
-- 
2.25.1

[PATCH v3 07/11] mm: Remove definition of clear_bit_unlock_is_negative_byte

[Matthew Wilcox]

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

All architectures now have a definition of
clear_bit_unlock_is_negative_byte(), either their own or through
asm-generic/bitops/lock.h

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: William Kucharski <william.kucharski@oracle.com>
---
 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 v3 08/11] 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 the next 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 v3 09/11] 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

Re: [PATCH v3 09/11] mm: Convert writeback BUG to WARN_ON

[Jan Kara]

On Thu 16-04-20 15:01:28, Matthew Wilcox wrote:
> 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;
> +	}
> +

WARN_ON_ONCE() here perhaps? I don't think dumping more pages will bring
that much value...

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v3 10/11] 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 v3 11/11] 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 v3 00/11] Make PageWriteback use the PageLocked optimisation

[Geert Uytterhoeven]

Hi Matthew,

On Fri, Apr 17, 2020 at 12:01 AM Matthew Wilcox <willy@infradead.org> wrote:
> 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.
>
> Unfortunately, clear_bit_unlock_is_negative_byte() isn't present on every
> architecture, and the default implementation is only available in filemap.c
> while I want to use it in page-writeback.c.  Rather than move the default
> implementation to a header file, I've done optimised implementations for
> alpha and ia64.  I can't figure out optimised implementations for m68k,
> mips, riscv and s390, so I've just replicated the effect of the generic
> implementation in them.  I leave it to the experts to fix that (... or
> convert over to using asm-generic/bitops/lock.h ...)
>
> v3:
>  - Added implementations of clear_bit_unlock_is_negative_byte()
>    to architectures which need it

I have two questions here?
  1. Why not implement arch_clear_bit_unlock_is_negative_byte()
     instead, so the kasan check in asm-generic is used everywhere?
  2. Why not add the default implementation to
     include/asm-generic/bitops/instrumented-lock.h, in case an arch_*()
     variant is not provided yet?

Note that you did 1 for s390.
Thanks!

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 00/11] Make PageWriteback use the PageLocked optimisation

[Matthew Wilcox]

On Fri, Apr 17, 2020 at 09:28:14AM +0200, Geert Uytterhoeven wrote:
> On Fri, Apr 17, 2020 at 12:01 AM Matthew Wilcox <willy@infradead.org> wrote:
> > v3:
> >  - Added implementations of clear_bit_unlock_is_negative_byte()
> >    to architectures which need it
> 
> I have two questions here?
>   1. Why not implement arch_clear_bit_unlock_is_negative_byte()
>      instead, so the kasan check in asm-generic is used everywhere?

That would be a larger change.  As I understand it (and I may misunderstand
it), I would need to rename all the clear_bit(), __clear_bit(), change_bit(),
... functions to have an 'arch_' prefix and then include instrumented-lock.h

>   2. Why not add the default implementation to
>      include/asm-generic/bitops/instrumented-lock.h, in case an arch_*()
>      variant is not provided yet?
> 
> Note that you did 1 for s390.

Well, s390 already uses instrumented-lock.h so I followed along with
what they're doing.  I don't think instrumented-lock.h is used at all on
these other architectures, but the whole bitops header files are such a
mess that I could easily have built a completely wrong mental model of
what's going on.