[PATCH 0/1] mm: page_cache_add_speculative(): refactor away duplicate code

[PATCH 0/1] mm: page_cache_add_speculative(): refactor away duplicate code

[john.hubbard]

From: John Hubbard <jhubbard@nvidia.com>

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(-)

-- 
2.20.1

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

[john.hubbard]

From: John Hubbard <jhubbard@nvidia.com>

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 <akpm@linux-foundation.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Dave Kleikamp <shaggy@linux.vnet.ibm.com>
Cc: Hugh Dickins <hughd@google.com>
Cc: Jeff Layton <jlayton@kernel.org>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Nick Piggin <npiggin@suse.de>
Cc: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 include/linux/pagemap.h | 31 +++++++++----------------------
 1 file changed, 9 insertions(+), 22 deletions(-)

diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index e2d7039af6a3..b477a70cc2e4 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -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
-- 
2.20.1

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

[Andrew Morton]

On Wed,  6 Feb 2019 15:10:16 -0800 john.hubbard@gmail.com wrote:

> From: John Hubbard <jhubbard@nvidia.com>
> 
> 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.

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

We'll live ;)