[PATCH 0/2] Make PageWriteback use the PageLocked optimisation

[PATCH 0/2] 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.

Matthew Wilcox (Oracle) (2):
  mm: Remove definition of clear_bit_unlock_is_negative_byte
  mm: Use clear_bit_unlock_is_negative_byte for PageWriteback

 include/linux/page-flags.h |  6 +++---
 mm/filemap.c               | 42 ++++++--------------------------------
 mm/page-writeback.c        | 37 ++++++++++++++++++---------------
 3 files changed, 29 insertions(+), 56 deletions(-)


base-commit: 5149100c3aebe5e640d6ff68e0b5e5a7eb8638e0
-- 
2.25.1

[PATCH 1/2] 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>
Cc: Will Deacon <will.deacon@arm.com>
---
 mm/filemap.c | 23 -----------------------
 1 file changed, 23 deletions(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index 80f7e1ae744c..312afbfcb49a 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

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

[Will Deacon]

Hi Matthew,

Sorry I missed this, I'm over on @kernel.org now and don't have access to
my old @arm.com address anymore.

On Thu, Mar 26, 2020 at 05:24:28AM -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>
> Cc: Will Deacon <will.deacon@arm.com>
> ---
>  mm/filemap.c | 23 -----------------------
>  1 file changed, 23 deletions(-)
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 80f7e1ae744c..312afbfcb49a 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
> -

I'd really like to do this, but I worry that the generic definition still
isn't available on all architectures depending on how they pull together
their bitops.h. Have you tried building for alpha or s390? At a quick
glance, they look like they might fall apart :(

Will

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

[Matthew Wilcox]

On Thu, Apr 16, 2020 at 01:45:39PM +0100, Will Deacon wrote:
> Sorry I missed this, I'm over on @kernel.org now and don't have access to
> my old @arm.com address anymore.

Oops.  Shame they haven't started bouncing it yet, so I didn't know.

> > -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);
> > -}
> 
> I'd really like to do this, but I worry that the generic definition still
> isn't available on all architectures depending on how they pull together
> their bitops.h. Have you tried building for alpha or s390? At a quick
> glance, they look like they might fall apart :(

I haven't, but fortunately the 0day build bot has!  It built both s390 and
alpha successfully (as well as 120+ other configurations).

[PATCH 2/2] mm: Use clear_bit_unlock_is_negative_byte for PageWriteback

[Matthew Wilcox]

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

By moving PG_writeback down into the low bits of the page flags, 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.  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>
---
 include/linux/page-flags.h |  6 +++---
 mm/filemap.c               | 19 ++++++-------------
 mm/page-writeback.c        | 37 ++++++++++++++++++++-----------------
 3 files changed, 29 insertions(+), 33 deletions(-)

diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index 222f6f7b2bb3..96c7d220c8cf 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 */
@@ -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 312afbfcb49a..bfe1782a7b98 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():
  */
@@ -1266,6 +1259,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,23 +1273,22 @@ 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);
 		rotate_reclaimable_page(page);
 	}
 
-	if (!test_clear_page_writeback(page))
-		BUG();
-
-	smp_mb__after_atomic();
-	wake_up_page(page, PG_writeback);
+	VM_BUG_ON_PAGE(!PageWriteback(page), page);
+	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 b7f3d0766a5f..4d675a7b81e6 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. Called from __clear_page_writeback().
  */
 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;
 
 	memcg = lock_page_memcg(page);
 	lruvec = mem_cgroup_page_lruvec(page, page_pgdat(page));
@@ -2726,16 +2733,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,
@@ -2744,7 +2749,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
@@ -2752,11 +2757,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

Re: [PATCH 2/2] mm: Use clear_bit_unlock_is_negative_byte for PageWriteback

[Jan Kara]

On Thu 26-03-20 05:24:29, Matthew Wilcox wrote:
> From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
> 
> By moving PG_writeback down into the low bits of the page flags, 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.  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>

The patch looks good to me. Just one nit:


> +	VM_BUG_ON_PAGE(!PageWriteback(page), page);
> +	if (__clear_page_writeback(page))
> +		wake_up_page_bit(page, PG_writeback);

Since __clear_page_writeback() isn't really prepared for PageWriteback()
not being set, can we move the VM_BUG_ON_PAGE() there? Otherwise feel free
to add:

Reviewed-by: Jan Kara <jack@suse.cz>

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

Re: [PATCH 2/2] mm: Use clear_bit_unlock_is_negative_byte for PageWriteback

[Matthew Wilcox]

On Thu, Mar 26, 2020 at 01:40:47PM +0100, Jan Kara wrote:
> On Thu 26-03-20 05:24:29, Matthew Wilcox wrote:
> > From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
> > 
> > By moving PG_writeback down into the low bits of the page flags, 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.  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>
> 
> The patch looks good to me. Just one nit:
> 
> > +	VM_BUG_ON_PAGE(!PageWriteback(page), page);
> > +	if (__clear_page_writeback(page))
> > +		wake_up_page_bit(page, PG_writeback);
> 
> Since __clear_page_writeback() isn't really prepared for PageWriteback()
> not being set, can we move the VM_BUG_ON_PAGE() there? Otherwise feel free
> to add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>

Thanks!  I put it there to be parallel to the PageLocked equivalent:

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))
                wake_up_page_bit(page, PG_locked);
}

but one could equally well argue it should be in __clear_page_writeback
instead.

Re: [PATCH 2/2] mm: Use clear_bit_unlock_is_negative_byte for PageWriteback

[William Kucharski]

I like it explicit in the code before __clear_page_writeback() is called,
and given it's the only caller I think it adds clarity to have it in
end_page_writeback() - but either approach is fine with me.

For the series:

Reviewed-by: William Kucharski <william.kucharski@oracle.com>

> On Mar 26, 2020, at 6:44 AM, Matthew Wilcox <willy@infradead.org> wrote:
> 
> On Thu, Mar 26, 2020 at 01:40:47PM +0100, Jan Kara wrote:
>> On Thu 26-03-20 05:24:29, Matthew Wilcox wrote:
>>> From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
>>> 
>>> By moving PG_writeback down into the low bits of the page flags, 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.  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>
>> 
>> The patch looks good to me. Just one nit:
>> 
>>> +	VM_BUG_ON_PAGE(!PageWriteback(page), page);
>>> +	if (__clear_page_writeback(page))
>>> +		wake_up_page_bit(page, PG_writeback);
>> 
>> Since __clear_page_writeback() isn't really prepared for PageWriteback()
>> not being set, can we move the VM_BUG_ON_PAGE() there? Otherwise feel free
>> to add:
>> 
>> Reviewed-by: Jan Kara <jack@suse.cz>
> 
> Thanks!  I put it there to be parallel to the PageLocked equivalent:
> 
> 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))
>                wake_up_page_bit(page, PG_locked);
> }
> 
> but one could equally well argue it should be in __clear_page_writeback
> instead.
> 

Re: [PATCH 2/2] mm: Use clear_bit_unlock_is_negative_byte for PageWriteback

[Christoph Hellwig]

On Thu, Mar 26, 2020 at 05:24:29AM -0700, Matthew Wilcox wrote:
> From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
> 
> By moving PG_writeback down into the low bits of the page flags, 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.  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>
> ---
>  include/linux/page-flags.h |  6 +++---
>  mm/filemap.c               | 19 ++++++-------------
>  mm/page-writeback.c        | 37 ++++++++++++++++++++-----------------
>  3 files changed, 29 insertions(+), 33 deletions(-)
> 
> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> index 222f6f7b2bb3..96c7d220c8cf 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 */

Do we need a comment why these need to be in the low bits?

Re: [PATCH 2/2] mm: Use clear_bit_unlock_is_negative_byte for PageWriteback

[Matthew Wilcox]

On Thu, Mar 26, 2020 at 10:11:14AM -0700, Christoph Hellwig wrote:
> On Thu, Mar 26, 2020 at 05:24:29AM -0700, Matthew Wilcox wrote:
> > From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
> > 
> > By moving PG_writeback down into the low bits of the page flags, 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.  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>
> > ---
> >  include/linux/page-flags.h |  6 +++---
> >  mm/filemap.c               | 19 ++++++-------------
> >  mm/page-writeback.c        | 37 ++++++++++++++++++++-----------------
> >  3 files changed, 29 insertions(+), 33 deletions(-)
> > 
> > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> > index 222f6f7b2bb3..96c7d220c8cf 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 */
> 
> Do we need a comment why these need to be in the low bits?

There's some nice build time assertions that they are, next to all the
documentation about why and how it all works:

@@ -1266,6 +1259,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,23 +1273,22 @@ EXPORT_SYMBOL(unlock_page);
  */
 void end_page_writeback(struct page *page)
 {
+       BUILD_BUG_ON(PG_writeback > 7);

so if anyone moves them out of the bottom byte, they'll see those
assertions fail and hopefully read this comment:

 * Note that this depends on PG_waiters being the sign bit in the byte
 * that contains PG_locked - thus the BUILD_BUG_ON(). That allows us to
 * clear the PG_locked bit and test PG_waiters at the same time fairly
 * portably (architectures that do LL/SC can test any bit, while x86 can
 * test the sign bit).

I'd expect any reviewer to notice a BUILD_BUG_ON() being removed and
query it if not explained in the changelog.