[PATCH v2 0/5] Replace is_longterm_pinnable_page()

[PATCH v2 0/5] Replace is_longterm_pinnable_page()

[Vishal Moola (Oracle)]

This patchset introduces some more helper functions for the folio
conversions, and converts all callers of is_longterm_pinnable_page() to
use folios.

--
v2:
  Fix a syntax issue
  Move flags check to top of try_grab_folio

Vishal Moola (Oracle) (5):
  mmzone: Introduce folio_is_zone_movable()
  mmzone: Introduce folio_migratetype()
  mm/gup_test.c: Convert verify_dma_pinned() to us folios
  mm/gup.c: Reorganize try_get_folio()
  mm: Remove is_longterm_pinnable_page() and Reimplement
    folio_is_longterm_pinnable()

 include/linux/mm.h     | 22 +++++------
 include/linux/mmzone.h |  8 ++++
 mm/gup.c               | 86 +++++++++++++++++++++---------------------
 mm/gup_test.c          | 13 ++++---
 4 files changed, 67 insertions(+), 62 deletions(-)

-- 
2.40.1

[PATCH v2 1/5] mmzone: Introduce folio_is_zone_movable()

[Vishal Moola (Oracle)]

Introduce folio_is_zone_movable() to act as a folio equivalent for
is_zone_movable_page(). This is to assist in later folio conversions.

Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>
Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 include/linux/mmzone.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index a4889c9d4055..744bf32e48a8 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -1116,6 +1116,11 @@ static inline bool is_zone_movable_page(const struct page *page)
 {
 	return page_zonenum(page) == ZONE_MOVABLE;
 }
+
+static inline bool folio_is_zone_movable(const struct folio *folio)
+{
+	return folio_zonenum(folio) == ZONE_MOVABLE;
+}
 #endif
 
 /*
-- 
2.40.1

[PATCH v2 2/5] mmzone: Introduce folio_migratetype()

[Vishal Moola (Oracle)]

Introduce folio_migratetype() as a folio equivalent for
get_pageblock_migratetype(). This function intends to return the
migratetype the folio is located in, hence the name choice.

Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>
Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 include/linux/mmzone.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 744bf32e48a8..b58c76e68ac7 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -105,6 +105,9 @@ extern int page_group_by_mobility_disabled;
 #define get_pageblock_migratetype(page)					\
 	get_pfnblock_flags_mask(page, page_to_pfn(page), MIGRATETYPE_MASK)
 
+#define folio_migratetype(folio)				\
+	get_pfnblock_flags_mask(&folio->page, folio_pfn(folio),		\
+			MIGRATETYPE_MASK)
 struct free_area {
 	struct list_head	free_list[MIGRATE_TYPES];
 	unsigned long		nr_free;
-- 
2.40.1

Re: [PATCH v2 2/5] mmzone: Introduce folio_migratetype()

[Andrew Morton]

On Tue, 13 Jun 2023 19:13:09 -0700 "Vishal Moola (Oracle)" <vishal.moola@gmail.com> wrote:

> Introduce folio_migratetype() as a folio equivalent for
> get_pageblock_migratetype(). This function intends to return the
> migratetype the folio is located in, hence the name choice.
> 
> ...
>
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -105,6 +105,9 @@ extern int page_group_by_mobility_disabled;
>  #define get_pageblock_migratetype(page)					\
>  	get_pfnblock_flags_mask(page, page_to_pfn(page), MIGRATETYPE_MASK)
>  
> +#define folio_migratetype(folio)				\
> +	get_pfnblock_flags_mask(&folio->page, folio_pfn(folio),		\
> +			MIGRATETYPE_MASK)

Theoretically this is risky because it evaluates its argument more than
once.  Although folio_migratetype(folio++) seems an unlikely thing to do.

An inlined C function is always preferable.  My quick attempt at that
reveals that the header files are All Messed Up As Usual.

Re: [PATCH v2 2/5] mmzone: Introduce folio_migratetype()

[Matthew Wilcox]

On Wed, Jun 14, 2023 at 01:13:05PM -0700, Andrew Morton wrote:
> On Tue, 13 Jun 2023 19:13:09 -0700 "Vishal Moola (Oracle)" <vishal.moola@gmail.com> wrote:
> 
> > Introduce folio_migratetype() as a folio equivalent for
> > get_pageblock_migratetype(). This function intends to return the
> > migratetype the folio is located in, hence the name choice.
> > 
> > ...
> >
> > --- a/include/linux/mmzone.h
> > +++ b/include/linux/mmzone.h
> > @@ -105,6 +105,9 @@ extern int page_group_by_mobility_disabled;
> >  #define get_pageblock_migratetype(page)					\
> >  	get_pfnblock_flags_mask(page, page_to_pfn(page), MIGRATETYPE_MASK)
> >  
> > +#define folio_migratetype(folio)				\
> > +	get_pfnblock_flags_mask(&folio->page, folio_pfn(folio),		\
> > +			MIGRATETYPE_MASK)
> 
> Theoretically this is risky because it evaluates its argument more than
> once.  Although folio_migratetype(folio++) seems an unlikely thing to do.

folio++ is always an unsafe thing to do.  folios are not consecutive
in memory (unless we know they're order-0).

> An inlined C function is always preferable.  My quick attempt at that
> reveals that the header files are All Messed Up As Usual.

The page-equivalent of this also evaluates its arguments more than once,
so it doesn't see too risky for now?

Re: [PATCH v2 2/5] mmzone: Introduce folio_migratetype()

[Andrew Morton]

On Wed, 14 Jun 2023 21:17:34 +0100 Matthew Wilcox <willy@infradead.org> wrote:

> On Wed, Jun 14, 2023 at 01:13:05PM -0700, Andrew Morton wrote:
> > On Tue, 13 Jun 2023 19:13:09 -0700 "Vishal Moola (Oracle)" <vishal.moola@gmail.com> wrote:
> > 
> > > Introduce folio_migratetype() as a folio equivalent for
> > > get_pageblock_migratetype(). This function intends to return the
> > > migratetype the folio is located in, hence the name choice.
> > > 
> > > ...
> > >
> > > --- a/include/linux/mmzone.h
> > > +++ b/include/linux/mmzone.h
> > > @@ -105,6 +105,9 @@ extern int page_group_by_mobility_disabled;
> > >  #define get_pageblock_migratetype(page)					\
> > >  	get_pfnblock_flags_mask(page, page_to_pfn(page), MIGRATETYPE_MASK)
> > >  
> > > +#define folio_migratetype(folio)				\
> > > +	get_pfnblock_flags_mask(&folio->page, folio_pfn(folio),		\
> > > +			MIGRATETYPE_MASK)
> > 
> > Theoretically this is risky because it evaluates its argument more than
> > once.  Although folio_migratetype(folio++) seems an unlikely thing to do.
> 
> folio++ is always an unsafe thing to do.  folios are not consecutive
> in memory (unless we know they're order-0).

OK, folio_migratetype(expensive_function_which_returns_a_folio()) or
folio_migratetype(function_with_side_effects_which_returns_a_folio()). 
There are many failure modes.

> > An inlined C function is always preferable.  My quick attempt at that
> > reveals that the header files are All Messed Up As Usual.
> 
> The page-equivalent of this also evaluates its arguments more than once,
> so it doesn't see too risky for now?

We got lucky.   It's just bad practice.

[PATCH v2 3/5] mm/gup_test.c: Convert verify_dma_pinned() to us folios

[Vishal Moola (Oracle)]

verify_dma_pinned() checks that pages are dma-pinned. We can convert
this to use folios.

Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>
Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 mm/gup_test.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/mm/gup_test.c b/mm/gup_test.c
index 8ae7307a1bb6..860b093b4b3e 100644
--- a/mm/gup_test.c
+++ b/mm/gup_test.c
@@ -40,24 +40,25 @@ static void verify_dma_pinned(unsigned int cmd, struct page **pages,
 			      unsigned long nr_pages)
 {
 	unsigned long i;
-	struct page *page;
+	struct folio *folio;
 
 	switch (cmd) {
 	case PIN_FAST_BENCHMARK:
 	case PIN_BASIC_TEST:
 	case PIN_LONGTERM_BENCHMARK:
 		for (i = 0; i < nr_pages; i++) {
-			page = pages[i];
-			if (WARN(!page_maybe_dma_pinned(page),
+			folio = page_folio(pages[i]);
+
+			if (WARN(!folio_maybe_dma_pinned(folio),
 				 "pages[%lu] is NOT dma-pinned\n", i)) {
 
-				dump_page(page, "gup_test failure");
+				dump_page(&folio->page, "gup_test failure");
 				break;
 			} else if (cmd == PIN_LONGTERM_BENCHMARK &&
-				WARN(!is_longterm_pinnable_page(page),
+				WARN(!folio_is_longterm_pinnable(folio),
 				     "pages[%lu] is NOT pinnable but pinned\n",
 				     i)) {
-				dump_page(page, "gup_test failure");
+				dump_page(&folio->page, "gup_test failure");
 				break;
 			}
 		}
-- 
2.40.1

[PATCH v2 4/5] mm/gup.c: Reorganize try_get_folio()

[Vishal Moola (Oracle)]

try_get_folio() takes in a page, then chooses to do some folio
operations based on the flags (either FOLL_GET or FOLL_PIN).
We can rewrite this function to be more purpose oriented.

After calling try_get_folio(), if neither FOLL_GET nor FOLL_PIN are set,
warn and fail. If FOLL_GET is set we can return the result. If FOLL_GET
is not set then FOLL_PIN is set, so we pin the folio.

This change assists with folio conversions, and makes the function more
readable.

Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>
---
 mm/gup.c | 86 ++++++++++++++++++++++++++++----------------------------
 1 file changed, 43 insertions(+), 43 deletions(-)

diff --git a/mm/gup.c b/mm/gup.c
index bbe416236593..6ec5c9629dc9 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -123,58 +123,58 @@ static inline struct folio *try_get_folio(struct page *page, int refs)
  */
 struct folio *try_grab_folio(struct page *page, int refs, unsigned int flags)
 {
+	struct folio *folio;
+
+	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);
-	else if (flags & FOLL_PIN) {
-		struct folio *folio;
+	folio = try_get_folio(page, refs);
 
-		/*
-		 * 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) &&
-			     !is_longterm_pinnable_page(page)))
-			return NULL;
+	if (flags & FOLL_GET)
+		return folio;
 
-		/*
-		 * CAUTION: Don't use compound_head() on the page before this
-		 * point, the result won't be stable.
-		 */
-		folio = try_get_folio(page, refs);
-		if (!folio)
-			return NULL;
+	/* FOLL_PIN is set */
+	if (!folio)
+		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 page_try_share_anon_rmap().
-		 */
-		smp_mb__after_atomic();
+	/*
+	 * 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_page_refs(&folio->page, refs))
+			folio_put_refs(folio, refs);
+		return NULL;
+	}
 
-		node_stat_mod_folio(folio, NR_FOLL_PIN_ACQUIRED, refs);
+	/*
+	 * 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 page_try_share_anon_rmap().
+	 */
+	smp_mb__after_atomic();
 
-		return folio;
-	}
+	node_stat_mod_folio(folio, NR_FOLL_PIN_ACQUIRED, refs);
 
-	WARN_ON_ONCE(1);
-	return NULL;
+	return folio;
 }
 
 static void gup_put_folio(struct folio *folio, int refs, unsigned int flags)
-- 
2.40.1

[PATCH v2 5/5] mm: Remove is_longterm_pinnable_page() and Reimplement folio_is_longterm_pinnable()

[Vishal Moola (Oracle)]

folio_is_longterm_pinnable() already exists as a wrapper function. Now
that the whole implementation of is_longterm_pinnable_page() can be
implemented using folios, folio_is_longterm_pinnable() can be made its
own standalone function - and we can remove is_longterm_pinnable_page().

Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>
Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 include/linux/mm.h | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 27ce77080c79..e2d35e272e07 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1910,39 +1910,35 @@ static inline bool page_needs_cow_for_dma(struct vm_area_struct *vma,
 	return page_maybe_dma_pinned(page);
 }
 
-/* MIGRATE_CMA and ZONE_MOVABLE do not allow pin pages */
+/* MIGRATE_CMA and ZONE_MOVABLE do not allow pin folios */
 #ifdef CONFIG_MIGRATION
-static inline bool is_longterm_pinnable_page(struct page *page)
+static inline bool folio_is_longterm_pinnable(struct folio *folio)
 {
 #ifdef CONFIG_CMA
-	int mt = get_pageblock_migratetype(page);
+	int mt = folio_migratetype(folio);
 
 	if (mt == MIGRATE_CMA || mt == MIGRATE_ISOLATE)
 		return false;
 #endif
 	/* The zero page may always be pinned */
-	if (is_zero_pfn(page_to_pfn(page)))
+	if (is_zero_pfn(folio_pfn(folio)))
 		return true;
 
 	/* Coherent device memory must always allow eviction. */
-	if (is_device_coherent_page(page))
+	if (folio_is_device_coherent(folio))
 		return false;
 
-	/* Otherwise, non-movable zone pages can be pinned. */
-	return !is_zone_movable_page(page);
+	/* Otherwise, non-movable zone folios can be pinned. */
+	return !folio_is_zone_movable(folio);
+
 }
 #else
-static inline bool is_longterm_pinnable_page(struct page *page)
+static inline bool folio_is_longterm_pinnable(struct folio *folio)
 {
 	return true;
 }
 #endif
 
-static inline bool folio_is_longterm_pinnable(struct folio *folio)
-{
-	return is_longterm_pinnable_page(&folio->page);
-}
-
 static inline void set_page_zone(struct page *page, enum zone_type zone)
 {
 	page->flags &= ~(ZONES_MASK << ZONES_PGSHIFT);
-- 
2.40.1

Re: [PATCH v2 5/5] mm: Remove is_longterm_pinnable_page() and Reimplement folio_is_longterm_pinnable()

[Lorenzo Stoakes]

On Wed, 14 Jun 2023 at 03:14, Vishal Moola (Oracle)
<vishal.moola@gmail.com> wrote:
>
> folio_is_longterm_pinnable() already exists as a wrapper function. Now
> that the whole implementation of is_longterm_pinnable_page() can be
> implemented using folios, folio_is_longterm_pinnable() can be made its
> own standalone function - and we can remove is_longterm_pinnable_page().
>
> Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>
> Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  include/linux/mm.h | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 27ce77080c79..e2d35e272e07 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1910,39 +1910,35 @@ static inline bool page_needs_cow_for_dma(struct vm_area_struct *vma,
>         return page_maybe_dma_pinned(page);
>  }
>
> -/* MIGRATE_CMA and ZONE_MOVABLE do not allow pin pages */
> +/* MIGRATE_CMA and ZONE_MOVABLE do not allow pin folios */
>  #ifdef CONFIG_MIGRATION
> -static inline bool is_longterm_pinnable_page(struct page *page)
> +static inline bool folio_is_longterm_pinnable(struct folio *folio)
>  {
>  #ifdef CONFIG_CMA
> -       int mt = get_pageblock_migratetype(page);
> +       int mt = folio_migratetype(folio);
>
>         if (mt == MIGRATE_CMA || mt == MIGRATE_ISOLATE)
>                 return false;
>  #endif
>         /* The zero page may always be pinned */
> -       if (is_zero_pfn(page_to_pfn(page)))
> +       if (is_zero_pfn(folio_pfn(folio)))
>                 return true;
>
>         /* Coherent device memory must always allow eviction. */
> -       if (is_device_coherent_page(page))
> +       if (folio_is_device_coherent(folio))
>                 return false;
>
> -       /* Otherwise, non-movable zone pages can be pinned. */
> -       return !is_zone_movable_page(page);
> +       /* Otherwise, non-movable zone folios can be pinned. */
> +       return !folio_is_zone_movable(folio);
> +

Nit, but you're adding a newline here.

>  }
>  #else
> -static inline bool is_longterm_pinnable_page(struct page *page)
> +static inline bool folio_is_longterm_pinnable(struct folio *folio)
>  {
>         return true;
>  }
>  #endif
>
> -static inline bool folio_is_longterm_pinnable(struct folio *folio)
> -{
> -       return is_longterm_pinnable_page(&folio->page);
> -}
> -
>  static inline void set_page_zone(struct page *page, enum zone_type zone)
>  {
>         page->flags &= ~(ZONES_MASK << ZONES_PGSHIFT);
> --
> 2.40.1
>
>

Reviewed-by: Lorenzo Stoakes <lstoakes@gmail.com>