Re: [linux-next:master 9137/9689] mm/gup.c:476:25: error: implicit declaration of function 'try_grab_folio_fast'; did you mean 'try_grab_folio'?

[Yang Shi <shy828301@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4914 bytes --]

On Fri, Jun 28, 2024 at 5:01 PM kernel test robot <lkp@intel.com> wrote:
>
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
> head:   1eb586a9782cde8e5091b9de74603e0a8386b09e
> commit: ee3fe497bdf0c7cb6fbec346f36aa1f9b1dfe6dd [9137/9689] mm: gup: do not call try_grab_folio() in slow path
> config: powerpc-randconfig-r005-20221108
> compiler: powerpc-linux-gcc (GCC) 13.2.0
> reproduce (this is a W=1 build):
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202406290729.21FzkVlI-lkp@intel.com/
>
> All error/warnings (new ones prefixed by >>):
>
>    mm/gup.c: In function 'gup_hugepte':
> >> mm/gup.c:476:25: error: implicit declaration of function 'try_grab_folio_fast'; did you mean 'try_grab_folio'? [-Werror=implicit-function-declaration]

The definition of try_grab_folio_fast() should be protected by
CONFIG_ARCH_HAS_HUGEPD || CONFIG_HAVE_GUP_FAST.

Andrew,

Please apply the attached fix on top of it. The reproducer requires
x86 machine since the cross build tool is on x86, but I don't have a
x86 machine by hand right now. But I think the patch should fix it.

>      476 |                 folio = try_grab_folio_fast(page, refs, flags);
>          |                         ^~~~~~~~~~~~~~~~~~~
>          |                         try_grab_folio
> >> mm/gup.c:476:23: warning: assignment to 'struct folio *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
>      476 |                 folio = try_grab_folio_fast(page, refs, flags);
>          |                       ^
>    mm/gup.c: At top level:
> >> mm/gup.c:2748:22: error: conflicting types for 'try_grab_folio_fast'; have 'struct folio *(struct page *, int,  unsigned int)'
>     2748 | static struct folio *try_grab_folio_fast(struct page *page, int refs,
>          |                      ^~~~~~~~~~~~~~~~~~~
>    mm/gup.c:476:25: note: previous implicit declaration of 'try_grab_folio_fast' with type 'int()'
>      476 |                 folio = try_grab_folio_fast(page, refs, flags);
>          |                         ^~~~~~~~~~~~~~~~~~~
>    cc1: some warnings being treated as errors
>
>
> vim +476 mm/gup.c
>
>    441
>    442  /*
>    443   * Returns 1 if succeeded, 0 if failed, -EMLINK if unshare needed.
>    444   *
>    445   * NOTE: for the same entry, gup-fast and gup-slow can return different
>    446   * results (0 v.s. -EMLINK) depending on whether vma is available.  This is
>    447   * the expected behavior, where we simply want gup-fast to fallback to
>    448   * gup-slow to take the vma reference first.
>    449   */
>    450  static int gup_hugepte(struct vm_area_struct *vma, pte_t *ptep, unsigned long sz,
>    451                         unsigned long addr, unsigned long end, unsigned int flags,
>    452                         struct page **pages, int *nr, bool fast)
>    453  {
>    454          unsigned long pte_end;
>    455          struct page *page;
>    456          struct folio *folio;
>    457          pte_t pte;
>    458          int refs;
>    459
>    460          pte_end = (addr + sz) & ~(sz-1);
>    461          if (pte_end < end)
>    462                  end = pte_end;
>    463
>    464          pte = huge_ptep_get(ptep);
>    465
>    466          if (!pte_access_permitted(pte, flags & FOLL_WRITE))
>    467                  return 0;
>    468
>    469          /* hugepages are never "special" */
>    470          VM_BUG_ON(!pfn_valid(pte_pfn(pte)));
>    471
>    472          page = pte_page(pte);
>    473          refs = record_subpages(page, sz, addr, end, pages + *nr);
>    474
>    475          if (fast) {
>  > 476                  folio = try_grab_folio_fast(page, refs, flags);
>    477                  if (!folio)
>    478                          return 0;
>    479          } else {
>    480                  folio = page_folio(page);
>    481                  if (try_grab_folio(folio, refs, flags))
>    482                          return 0;
>    483          }
>    484
>    485          if (unlikely(pte_val(pte) != pte_val(ptep_get(ptep)))) {
>    486                  gup_put_folio(folio, refs, flags);
>    487                  return 0;
>    488          }
>    489
>    490          if (!pte_write(pte) && gup_must_unshare(vma, flags, &folio->page)) {
>    491                  gup_put_folio(folio, refs, flags);
>    492                  return -EMLINK;
>    493          }
>    494
>    495          *nr += refs;
>    496          folio_set_referenced(folio);
>    497          return 1;
>    498  }
>    499
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki

[-- Attachment #2: mm-gup-stop-abusing-try_grab_folio-fix.patch --]
[-- Type: application/octet-stream, Size: 6771 bytes --]

diff --git a/mm/gup.c b/mm/gup.c
index e65773ce4622..fdd3b2719f33 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -427,6 +427,102 @@ static int record_subpages(struct page *page, unsigned long sz,
 
 	return nr;
 }
+
+/**
+ * try_grab_folio_fast() - Attempt to get or pin a folio in fast path.
+ * @page:  pointer to page to be grabbed
+ * @refs:  the value to (effectively) add to the folio's refcount
+ * @flags: gup flags: these are the FOLL_* flag values.
+ *
+ * "grab" names in this file mean, "look at flags to decide whether to use
+ * FOLL_PIN or FOLL_GET behavior, when incrementing the folio's refcount.
+ *
+ * Either FOLL_PIN or FOLL_GET (or neither) must be set, but not both at the
+ * same time. (That's true throughout the get_user_pages*() and
+ * pin_user_pages*() APIs.) Cases:
+ *
+ *    FOLL_GET: folio's refcount will be incremented by @refs.
+ *
+ *    FOLL_PIN on large folios: folio's refcount will be incremented by
+ *    @refs, and its pincount will be incremented by @refs.
+ *
+ *    FOLL_PIN on single-page folios: folio's refcount will be incremented by
+ *    @refs * GUP_PIN_COUNTING_BIAS.
+ *
+ * Return: The folio containing @page (with refcount appropriately
+ * incremented) for success, or NULL upon failure. If neither FOLL_GET
+ * nor FOLL_PIN was set, that's considered failure, and furthermore,
+ * a likely bug in the caller, so a warning is also emitted.
+ *
+ * It uses add ref unless zero to elevate the folio refcount and must be called
+ * in fast path only.
+ */
+static struct folio *try_grab_folio_fast(struct page *page, int refs,
+					 unsigned int flags)
+{
+	struct folio *folio;
+
+	/* Raise warn if it is not called in fast GUP */
+	VM_WARN_ON_ONCE(!irqs_disabled());
+
+	if (WARN_ON_ONCE((flags & (FOLL_GET | FOLL_PIN)) == 0))
+		return NULL;
+
+	if (unlikely(!(flags & FOLL_PCI_P2PDMA) && is_pci_p2pdma_page(page)))
+		return NULL;
+
+	if (flags & FOLL_GET)
+		return try_get_folio(page, refs);
+
+	/* FOLL_PIN is set */
+
+	/*
+	 * Don't take a pin on the zero page - it's not going anywhere
+	 * and it is used in a *lot* of places.
+	 */
+	if (is_zero_page(page))
+		return page_folio(page);
+
+	folio = try_get_folio(page, refs);
+	if (!folio)
+		return NULL;
+
+	/*
+	 * Can't do FOLL_LONGTERM + FOLL_PIN gup fast path if not in a
+	 * right zone, so fail and let the caller fall back to the slow
+	 * path.
+	 */
+	if (unlikely((flags & FOLL_LONGTERM) &&
+		     !folio_is_longterm_pinnable(folio))) {
+		if (!put_devmap_managed_folio_refs(folio, refs))
+			folio_put_refs(folio, refs);
+		return NULL;
+	}
+
+	/*
+	 * When pinning a large folio, use an exact count to track it.
+	 *
+	 * However, be sure to *also* increment the normal folio
+	 * refcount field at least once, so that the folio really
+	 * is pinned.  That's why the refcount from the earlier
+	 * try_get_folio() is left intact.
+	 */
+	if (folio_test_large(folio))
+		atomic_add(refs, &folio->_pincount);
+	else
+		folio_ref_add(folio,
+				refs * (GUP_PIN_COUNTING_BIAS - 1));
+	/*
+	 * Adjust the pincount before re-checking the PTE for changes.
+	 * This is essentially a smp_mb() and is paired with a memory
+	 * barrier in folio_try_share_anon_rmap_*().
+	 */
+	smp_mb__after_atomic();
+
+	node_stat_mod_folio(folio, NR_FOLL_PIN_ACQUIRED, refs);
+
+	return folio;
+}
 #endif	/* CONFIG_ARCH_HAS_HUGEPD || CONFIG_HAVE_GUP_FAST */
 
 #ifdef CONFIG_ARCH_HAS_HUGEPD
@@ -2715,102 +2811,6 @@ EXPORT_SYMBOL(get_user_pages_unlocked);
  * This code is based heavily on the PowerPC implementation by Nick Piggin.
  */
 #ifdef CONFIG_HAVE_GUP_FAST
-/**
- * try_grab_folio_fast() - Attempt to get or pin a folio in fast path.
- * @page:  pointer to page to be grabbed
- * @refs:  the value to (effectively) add to the folio's refcount
- * @flags: gup flags: these are the FOLL_* flag values.
- *
- * "grab" names in this file mean, "look at flags to decide whether to use
- * FOLL_PIN or FOLL_GET behavior, when incrementing the folio's refcount.
- *
- * Either FOLL_PIN or FOLL_GET (or neither) must be set, but not both at the
- * same time. (That's true throughout the get_user_pages*() and
- * pin_user_pages*() APIs.) Cases:
- *
- *    FOLL_GET: folio's refcount will be incremented by @refs.
- *
- *    FOLL_PIN on large folios: folio's refcount will be incremented by
- *    @refs, and its pincount will be incremented by @refs.
- *
- *    FOLL_PIN on single-page folios: folio's refcount will be incremented by
- *    @refs * GUP_PIN_COUNTING_BIAS.
- *
- * Return: The folio containing @page (with refcount appropriately
- * incremented) for success, or NULL upon failure. If neither FOLL_GET
- * nor FOLL_PIN was set, that's considered failure, and furthermore,
- * a likely bug in the caller, so a warning is also emitted.
- *
- * It uses add ref unless zero to elevate the folio refcount and must be called
- * in fast path only.
- */
-static struct folio *try_grab_folio_fast(struct page *page, int refs,
-					 unsigned int flags)
-{
-	struct folio *folio;
-
-	/* Raise warn if it is not called in fast GUP */
-	VM_WARN_ON_ONCE(!irqs_disabled());
-
-	if (WARN_ON_ONCE((flags & (FOLL_GET | FOLL_PIN)) == 0))
-		return NULL;
-
-	if (unlikely(!(flags & FOLL_PCI_P2PDMA) && is_pci_p2pdma_page(page)))
-		return NULL;
-
-	if (flags & FOLL_GET)
-		return try_get_folio(page, refs);
-
-	/* FOLL_PIN is set */
-
-	/*
-	 * Don't take a pin on the zero page - it's not going anywhere
-	 * and it is used in a *lot* of places.
-	 */
-	if (is_zero_page(page))
-		return page_folio(page);
-
-	folio = try_get_folio(page, refs);
-	if (!folio)
-		return NULL;
-
-	/*
-	 * Can't do FOLL_LONGTERM + FOLL_PIN gup fast path if not in a
-	 * right zone, so fail and let the caller fall back to the slow
-	 * path.
-	 */
-	if (unlikely((flags & FOLL_LONGTERM) &&
-		     !folio_is_longterm_pinnable(folio))) {
-		if (!put_devmap_managed_folio_refs(folio, refs))
-			folio_put_refs(folio, refs);
-		return NULL;
-	}
-
-	/*
-	 * When pinning a large folio, use an exact count to track it.
-	 *
-	 * However, be sure to *also* increment the normal folio
-	 * refcount field at least once, so that the folio really
-	 * is pinned.  That's why the refcount from the earlier
-	 * try_get_folio() is left intact.
-	 */
-	if (folio_test_large(folio))
-		atomic_add(refs, &folio->_pincount);
-	else
-		folio_ref_add(folio,
-				refs * (GUP_PIN_COUNTING_BIAS - 1));
-	/*
-	 * Adjust the pincount before re-checking the PTE for changes.
-	 * This is essentially a smp_mb() and is paired with a memory
-	 * barrier in folio_try_share_anon_rmap_*().
-	 */
-	smp_mb__after_atomic();
-
-	node_stat_mod_folio(folio, NR_FOLL_PIN_ACQUIRED, refs);
-
-	return folio;
-}
-
 /*
  * Used in the GUP-fast path to determine whether GUP is permitted to work on
  * a specific folio.