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

[Claudio Imbrenda <imbrenda@linux.ibm.com>]

On Tue, 10 Mar 2020 18:46:40 -0700
John Hubbard <jhubbard@nvidia.com> wrote:

> 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

yes, that is what I had sent

> any case, that's what this needs in order to build. Sorry for missing
> it in the review.

I don't think you missed it, because the patch I sent out to be
squashed did move it up

> Claudio, did you compile-test the patch? I'm not sure how this made

not only compile tested, I even actually tested-tested it.

> 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:

the patch I had sent out did move put_compound_head to around line 116

> 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:

I think the most appropriate place would be right after
try_grab_compound_head, which is where I had put it.

> 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,