* [PATCH v2 1/5] mm: Remove definition of clear_bit_unlock_is_negative_byte
2020-04-16 15:46 [PATCH v2 0/5] Make PageWriteback use the PageLocked optimisation Matthew Wilcox
@ 2020-04-16 15:46 ` Matthew Wilcox
2020-04-16 17:02 ` Will Deacon
2020-04-16 15:46 ` [PATCH v2 2/5] mm: Move PG_writeback into the bottom byte Matthew Wilcox
` (3 subsequent siblings)
4 siblings, 1 reply; 8+ messages in thread
From: Matthew Wilcox @ 2020-04-16 15:46 UTC (permalink / raw)
To: linux-mm, linux-fsdevel
Cc: Matthew Wilcox (Oracle), William Kucharski, Will Deacon
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/5] mm: Remove definition of clear_bit_unlock_is_negative_byte
2020-04-16 15:46 ` [PATCH v2 1/5] mm: Remove definition of clear_bit_unlock_is_negative_byte Matthew Wilcox
@ 2020-04-16 17:02 ` Will Deacon
2020-04-16 17:13 ` Matthew Wilcox
0 siblings, 1 reply; 8+ messages in thread
From: Will Deacon @ 2020-04-16 17:02 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: linux-mm, linux-fsdevel, William Kucharski
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/5] mm: Remove definition of clear_bit_unlock_is_negative_byte
2020-04-16 17:02 ` Will Deacon
@ 2020-04-16 17:13 ` Matthew Wilcox
0 siblings, 0 replies; 8+ messages in thread
From: Matthew Wilcox @ 2020-04-16 17:13 UTC (permalink / raw)
To: Will Deacon; +Cc: linux-mm, linux-fsdevel, William Kucharski, lkp
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?
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 2/5] mm: Move PG_writeback into the bottom byte
2020-04-16 15:46 [PATCH v2 0/5] Make PageWriteback use the PageLocked optimisation Matthew Wilcox
2020-04-16 15:46 ` [PATCH v2 1/5] mm: Remove definition of clear_bit_unlock_is_negative_byte Matthew Wilcox
@ 2020-04-16 15:46 ` Matthew Wilcox
2020-04-16 15:46 ` [PATCH v2 3/5] mm: Convert writeback BUG to WARN_ON Matthew Wilcox
` (2 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: Matthew Wilcox @ 2020-04-16 15:46 UTC (permalink / raw)
To: linux-mm, linux-fsdevel
Cc: Matthew Wilcox (Oracle), Jan Kara, William Kucharski
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 3/5] mm: Convert writeback BUG to WARN_ON
2020-04-16 15:46 [PATCH v2 0/5] Make PageWriteback use the PageLocked optimisation Matthew Wilcox
2020-04-16 15:46 ` [PATCH v2 1/5] mm: Remove definition of clear_bit_unlock_is_negative_byte Matthew Wilcox
2020-04-16 15:46 ` [PATCH v2 2/5] mm: Move PG_writeback into the bottom byte Matthew Wilcox
@ 2020-04-16 15:46 ` Matthew Wilcox
2020-04-16 15:46 ` [PATCH v2 4/5] mm: Use clear_bit_unlock_is_negative_byte for PageWriteback Matthew Wilcox
2020-04-16 15:46 ` [PATCH v2 5/5] mm: Remove TestClearPageWriteback Matthew Wilcox
4 siblings, 0 replies; 8+ messages in thread
From: Matthew Wilcox @ 2020-04-16 15:46 UTC (permalink / raw)
To: linux-mm, linux-fsdevel; +Cc: Matthew Wilcox (Oracle)
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 4/5] mm: Use clear_bit_unlock_is_negative_byte for PageWriteback
2020-04-16 15:46 [PATCH v2 0/5] Make PageWriteback use the PageLocked optimisation Matthew Wilcox
` (2 preceding siblings ...)
2020-04-16 15:46 ` [PATCH v2 3/5] mm: Convert writeback BUG to WARN_ON Matthew Wilcox
@ 2020-04-16 15:46 ` Matthew Wilcox
2020-04-16 15:46 ` [PATCH v2 5/5] mm: Remove TestClearPageWriteback Matthew Wilcox
4 siblings, 0 replies; 8+ messages in thread
From: Matthew Wilcox @ 2020-04-16 15:46 UTC (permalink / raw)
To: linux-mm, linux-fsdevel
Cc: Matthew Wilcox (Oracle), Jan Kara, William Kucharski
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 5/5] mm: Remove TestClearPageWriteback
2020-04-16 15:46 [PATCH v2 0/5] Make PageWriteback use the PageLocked optimisation Matthew Wilcox
` (3 preceding siblings ...)
2020-04-16 15:46 ` [PATCH v2 4/5] mm: Use clear_bit_unlock_is_negative_byte for PageWriteback Matthew Wilcox
@ 2020-04-16 15:46 ` Matthew Wilcox
4 siblings, 0 replies; 8+ messages in thread
From: Matthew Wilcox @ 2020-04-16 15:46 UTC (permalink / raw)
To: linux-mm, linux-fsdevel; +Cc: Matthew Wilcox (Oracle)
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
^ permalink raw reply related [flat|nested] 8+ messages in thread