Skip to contentSkip to conversation

mm: page_cache_add_speculative(): refactor away duplicate code

PATCH In review [email protected] 2 participants 3 messages Feb 6, 2019 – Feb 8, 2019

Subsystems (2)
Memory management Page cache
lore
Share & tools
lore

[email protected]

author
#0 lore raw

cover mm: page_cache_add_speculative(): refactor away duplicate code

From: John Hubbard <[email protected]>

Hi,

I ran across this while working on the get_user_pages() + [R]DMA problem, but we might as well remove the small bit of code duplication, independent of gup/dma (which is going to take "a little bit" longer to get submitted, ha).

John Hubbard (1):
mm: page_cache_add_speculative(): refactor out some code duplication

 include/linux/pagemap.h | 31 +++++++++----------------------
 1 file changed, 9 insertions(+), 22 deletions(-)
Signature · 1 line
2.20.1
author
#1 lore raw

PATCH 1/1 mm: page_cache_add_speculative(): refactor out some code duplication

From: John Hubbard <[email protected]>

This combines the common elements of these routines:

page_cache_get_speculative()
page_cache_add_speculative()

This was anticipated by the original author, as shown by the comment in commit ce0ad7f095258 ("powerpc/mm: Lockless get_user_pages_fast() for 64-bit (v3)"):

"Same as above, but add instead of inc (could just be merged)"

There is no intention to introduce any behavioral change, but there is a small risk of that, due to slightly differing ways of expressing the TINY_RCU and related configurations.

This also removes the VM_BUG_ON(in_interrupt()) that was in page_cache_add_speculative(), but not in page_cache_get_speculative(). This provides slightly less detection of such bugs, but it given that it was only there on the "add" path anyway, we can likely do without it just fine.

Cc Andrew Morton <[email protected]>
Cc Benjamin Herrenschmidt <[email protected]>
Cc Dave Kleikamp <[email protected]>
Cc Hugh Dickins <[email protected]>
Cc Jeff Layton <[email protected]>
Cc Matthew Wilcox <[email protected]>
Cc Nick Piggin <[email protected]>
Cc Vlastimil Babka <[email protected]>
Signed-off-by John Hubbard <[email protected]>
---
 include/linux/pagemap.h | 31 +++++++++----------------------
 1 file changed, 9 insertions(+), 22 deletions(-)
include/linux/pagemap.h +9 -22
index e2d7039af6a3..b477a70cc2e4 100644
@@ -164,7 +164,7 @@ void release_pages(struct page **pages, int nr);
  * will find the page or it will not. Likewise, the old find_get_page could run
  * either before the insertion or afterwards, depending on timing.
  */
-static inline int page_cache_get_speculative(struct page *page)
+static inline int __page_cache_add_speculative(struct page *page, int count)
 {
 #ifdef CONFIG_TINY_RCU
 # ifdef CONFIG_PREEMPT_COUNT

@@ -180,10 +180,10 @@ static inline int page_cache_get_speculative(struct page *page)
 	 * SMP requires.
 	 */
 	VM_BUG_ON_PAGE(page_count(page) == 0, page);
-	page_ref_inc(page);
+	page_ref_add(page, count);
 
 #else
-	if (unlikely(!get_page_unless_zero(page))) {
+	if (unlikely(!page_ref_add_unless(page, count, 0))) {
 		/*
 		 * Either the page has been freed, or will be freed.
 		 * In either case, retry here and the caller should

@@ -197,27 +197,14 @@ static inline int page_cache_get_speculative(struct page *page)
 	return 1;
 }
 
-/*
- * Same as above, but add instead of inc (could just be merged)
- */
-static inline int page_cache_add_speculative(struct page *page, int count)
+static inline int page_cache_get_speculative(struct page *page)
 {
-	VM_BUG_ON(in_interrupt());
-
-#if !defined(CONFIG_SMP) && defined(CONFIG_TREE_RCU)
-# ifdef CONFIG_PREEMPT_COUNT
-	VM_BUG_ON(!in_atomic() && !irqs_disabled());
-# endif
-	VM_BUG_ON_PAGE(page_count(page) == 0, page);
-	page_ref_add(page, count);
-
-#else
-	if (unlikely(!page_ref_add_unless(page, count, 0)))
-		return 0;
-#endif
-	VM_BUG_ON_PAGE(PageCompound(page) && page != compound_head(page), page);
+	return __page_cache_add_speculative(page, 1);
+}
 
-	return 1;
+static inline int page_cache_add_speculative(struct page *page, int count)
+{
+	return __page_cache_add_speculative(page, count);
 }
 
 #ifdef CONFIG_NUMA
Signature · 1 line
2.20.1
maintainer
#2 lore raw

Re: [PATCH 1/1] mm: page_cache_add_speculative(): refactor out some code duplication

Quoted 13 lines · On Wed, 6 Feb 2019 15:10:16 -0800 [email protected] wrote:

From: John Hubbard <[email protected]>

This combines the common elements of these routines:

page_cache_get_speculative()
page_cache_add_speculative()

This was anticipated by the original author, as shown by the comment in commit ce0ad7f095258 ("powerpc/mm: Lockless get_user_pages_fast() for 64-bit (v3)"):

"Same as above, but add instead of inc (could just be merged)"

Quoted 8 lines

There is no intention to introduce any behavioral change, but there is a small risk of that, due to slightly differing ways of expressing the TINY_RCU and related configurations.

This also removes the VM_BUG_ON(in_interrupt()) that was in page_cache_add_speculative(), but not in page_cache_get_speculative(). This provides slightly less detection of such bugs, but it given that it was only there on the "add" path anyway, we can likely do without it just fine.

It removes the
VM_BUG_ON_PAGE(PageCompound(page) && page != compound_head(page), page); also.

We'll live ;)