Re: + mm-gup-track-foll_pin-pages-fix-2.patch added to -mm tree

* Re: + mm-gup-track-foll_pin-pages-fix-2.patch added to -mm tree
       [not found] <20200307232517.0eSNFUWfN%akpm@linux-foundation.org>
@ 2020-03-11  1:46 ` John Hubbard
  2020-03-11 10:13   ` Claudio Imbrenda
  0 siblings, 1 reply; 4+ messages in thread
From: John Hubbard @ 2020-03-11  1:46 UTC (permalink / raw)
  To: Andrew Morton, imbrenda, mm-commits, Linux-MM

On 3/7/20 3:25 PM, Andrew Morton wrote:
> 
> The patch titled
>      Subject: mm/gup: fixup for 9947ea2c1e608e32 "mm/gup: track FOLL_PIN pages"
> has been added to the -mm tree.  Its filename is
>      mm-gup-track-foll_pin-pages-fix-2.patch
> 

This appears to be causing a build failure in mm/gup.c, in mmotm, as of today anyway:

    commit 5d9ee416b570 ("pci: test for unexpectedly disabled bridges")


I vaguely recall reviewing an earlier version that moved put_compound_head() up higher in 
gup.c, in order to avoid this. In any case, that's what this needs in order to build. 
Sorry for missing it in the review.

Claudio, did you compile-test the patch? I'm not sure how this made it all the way to mmotm
without it triggering something, either, but I can't see anything obviously wrong with my
tree, which has no local changes applied.

Here's what I'm seeing, and it makes sense because put_compound_head() is actually at 
line 2323:

mm/gup.c: In function ‘gup_pte_range’:
mm/gup.c:2177:4: error: implicit declaration of function ‘put_compound_head’; did you mean ‘set_compound_head’? [-Werror=implicit-function-declaration]
 2177 |    put_compound_head(head, 1, flags);
      |    ^~~~~~~~~~~~~~~~~
      |    set_compound_head
mm/gup.c: At top level:
mm/gup.c:2323:13: warning: conflicting types for ‘put_compound_head’
 2323 | static void put_compound_head(struct page *page, int refs, unsigned int flags)
      |             ^~~~~~~~~~~~~~~~~
mm/gup.c:2323:13: error: static declaration of ‘put_compound_head’ follows non-static declaration
mm/gup.c:2177:4: note: previous implicit declaration of ‘put_compound_head’ was here
 2177 |    put_compound_head(head, 1, flags);
      |    ^~~~~~~~~~~~~~~~~

And here's a compile-tested (only) proposed fix:

diff --git a/mm/gup.c b/mm/gup.c
index 722fcdd32ab8..6344252f0632 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -2116,6 +2116,28 @@ static inline pte_t gup_get_pte(pte_t *ptep)
 }
 #endif /* CONFIG_GUP_GET_PTE_LOW_HIGH */
 
+static void put_compound_head(struct page *page, int refs, unsigned int flags)
+{
+	if (flags & FOLL_PIN) {
+		mod_node_page_state(page_pgdat(page), NR_FOLL_PIN_RELEASED,
+				    refs);
+
+		if (hpage_pincount_available(page))
+			hpage_pincount_sub(page, refs);
+		else
+			refs *= GUP_PIN_COUNTING_BIAS;
+	}
+
+	VM_BUG_ON_PAGE(page_ref_count(page) < refs, page);
+	/*
+	 * Calling put_page() for each ref is unnecessarily slow. Only the last
+	 * ref needs a put_page().
+	 */
+	if (refs > 1)
+		page_ref_sub(page, refs - 1);
+	put_page(page);
+}
+
 static void __maybe_unused undo_dev_pagemap(int *nr, int nr_start,
 					    unsigned int flags,
 					    struct page **pages)
@@ -2320,28 +2342,6 @@ static int record_subpages(struct page *page, unsigned long addr,
 	return nr;
 }
 
-static void put_compound_head(struct page *page, int refs, unsigned int flags)
-{
-	if (flags & FOLL_PIN) {
-		mod_node_page_state(page_pgdat(page), NR_FOLL_PIN_RELEASED,
-				    refs);
-
-		if (hpage_pincount_available(page))
-			hpage_pincount_sub(page, refs);
-		else
-			refs *= GUP_PIN_COUNTING_BIAS;
-	}
-
-	VM_BUG_ON_PAGE(page_ref_count(page) < refs, page);
-	/*
-	 * Calling put_page() for each ref is unnecessarily slow. Only the last
-	 * ref needs a put_page().
-	 */
-	if (refs > 1)
-		page_ref_sub(page, refs - 1);
-	put_page(page);
-}
-
 #ifdef CONFIG_ARCH_HAS_HUGEPD
 static unsigned long hugepte_addr_end(unsigned long addr, unsigned long end,
 				      unsigned long sz)



thanks,
-- 
John Hubbard
NVIDIA

> This patch should soon appear at
>     http://ozlabs.org/~akpm/mmots/broken-out/mm-gup-track-foll_pin-pages-fix-2.patch
> and later at
>     http://ozlabs.org/~akpm/mmotm/broken-out/mm-gup-track-foll_pin-pages-fix-2.patch
> 
> Before you just go and hit "reply", please:
>    a) Consider who else should be cc'ed
>    b) Prefer to cc a suitable mailing list as well
>    c) Ideally: find the original patch on the mailing list and do a
>       reply-to-all to that, adding suitable additional cc's
> 
> *** Remember to use Documentation/process/submit-checklist.rst when testing your code ***
> 
> The -mm tree is included into linux-next and is updated
> there every 3-4 working days
> 
> ------------------------------------------------------
> From: Claudio Imbrenda <imbrenda@linux.ibm.com>
> Subject: mm/gup: fixup for 9947ea2c1e608e32 "mm/gup: track FOLL_PIN pages"
> 
> In case pin fails, we need to unpin, a simple put_page will not be enough
> 
> fixup for commit 9947ea2c1e608e32 ("mm/gup: track FOLL_PIN pages")
> 
> it can be simply squashed in
> 
> Link: http://lkml.kernel.org/r/20200306132537.783769-2-imbrenda@linux.ibm.com
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> Reviewed-by: John Hubbard <jhubbard@nvidia.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  mm/gup.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- a/mm/gup.c~mm-gup-track-foll_pin-pages-fix-2
> +++ a/mm/gup.c
> @@ -2065,7 +2065,7 @@ static int gup_pte_range(pmd_t pmd, unsi
>  			goto pte_unmap;
>  
>  		if (unlikely(pte_val(pte) != pte_val(*ptep))) {
> -			put_page(head);
> +			put_compound_head(head, 1, flags);
>  			goto pte_unmap;
>  		}
>  
> _
> 
> Patches currently in -mm which might be from imbrenda@linux.ibm.com are
> 
> mm-gup-track-foll_pin-pages-fix-2.patch
> mm-gup-writeback-add-callbacks-for-inaccessible-pages.patch
> 


^ permalink raw reply related	[flat|nested] 4+ messages in thread