Instead, let's use a page flag. As the page flag can result in
false-positives, glue it to the page types for which we
support/implement movable_ops page migration.
The flag reused by PageMovableOps() might be sued by other pages, so
warning in case it is set in page_has_movable_ops() might result in
false-positive warnings.
Reviewed-by: Zi Yan <ziy@nvidia.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
include/linux/balloon_compaction.h | 2 +-
include/linux/migrate.h | 8 -----
include/linux/page-flags.h | 52 ++++++++++++++++++++++++------
mm/compaction.c | 6 ----
mm/zpdesc.h | 2 +-
5 files changed, 44 insertions(+), 26 deletions(-)
diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h
index a8a1706cc56f3..b222b0737c466 100644
--- a/include/linux/balloon_compaction.h
+++ b/include/linux/balloon_compaction.h
@@ -92,7 +92,7 @@ static inline void balloon_page_insert(struct balloon_dev_info *balloon,
struct page *page)
{
__SetPageOffline(page);
- __SetPageMovable(page);
+ SetPageMovableOps(page);
set_page_private(page, (unsigned long)balloon);
list_add(&page->lru, &balloon->pages);
}
diff --git a/include/linux/migrate.h b/include/linux/migrate.h
index 6aece3f3c8be8..acadd41e0b5cf 100644
--- a/include/linux/migrate.h
+++ b/include/linux/migrate.h
@@ -103,14 +103,6 @@ static inline int migrate_huge_page_move_mapping(struct address_space *mapping,
#endif /* CONFIG_MIGRATION */
-#ifdef CONFIG_COMPACTION
-void __SetPageMovable(struct page *page);
-#else
-static inline void __SetPageMovable(struct page *page)
-{
-}
-#endif
-
#ifdef CONFIG_NUMA_BALANCING
int migrate_misplaced_folio_prepare(struct folio *folio,
struct vm_area_struct *vma, int node);
diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index 4c27ebb689e3c..016a6e6fa428a 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -170,6 +170,11 @@ enum pageflags {
/* non-lru isolated movable page */
PG_isolated = PG_reclaim,
+#ifdef CONFIG_MIGRATION
+ /* this is a movable_ops page (for selected typed pages only) */
+ PG_movable_ops = PG_uptodate,
+#endif
+
/* Only valid for buddy pages. Used to track pages that are reported */
PG_reported = PG_uptodate,
@@ -698,9 +703,6 @@ PAGEFLAG_FALSE(VmemmapSelfHosted, vmemmap_self_hosted)
* bit; and then folio->mapping points, not to an anon_vma, but to a private
* structure which KSM associates with that merged page. See ksm.h.
*
- * PAGE_MAPPING_KSM without PAGE_MAPPING_ANON is used for non-lru movable
- * page and then folio->mapping points to a struct movable_operations.
- *
* Please note that, confusingly, "folio_mapping" refers to the inode
* address_space which maps the folio from disk; whereas "folio_mapped"
* refers to user virtual address space into which the folio is mapped.
@@ -743,13 +745,6 @@ static __always_inline bool PageAnon(const struct page *page)
{
return folio_test_anon(page_folio(page));
}
-
-static __always_inline bool page_has_movable_ops(const struct page *page)
-{
- return ((unsigned long)page->mapping & PAGE_MAPPING_FLAGS) ==
- PAGE_MAPPING_MOVABLE;
-}
-
#ifdef CONFIG_KSM
/*
* A KSM page is one of those write-protected "shared pages" or "merged pages"
@@ -1133,6 +1128,43 @@ bool is_free_buddy_page(const struct page *page);
PAGEFLAG(Isolated, isolated, PF_ANY);
+#ifdef CONFIG_MIGRATION
+/*
+ * This page is migratable through movable_ops (for selected typed pages
+ * only).
+ *
+ * Page migration of such pages might fail, for example, if the page is
+ * already isolated by somebody else, or if the page is about to get freed.
+ *
+ * While a subsystem might set selected typed pages that support page migration
+ * as being movable through movable_ops, it must never clear this flag.
+ *
+ * This flag is only cleared when the page is freed back to the buddy.
+ *
+ * Only selected page types support this flag (see page_movable_ops()) and
+ * the flag might be used in other context for other pages. Always use
+ * page_has_movable_ops() instead.
+ */
+PAGEFLAG(MovableOps, movable_ops, PF_NO_TAIL);
+#else
+PAGEFLAG_FALSE(MovableOps, movable_ops);
+#endif
+
+/**
+ * page_has_movable_ops - test for a movable_ops page
+ * @page The page to test.
+ *
+ * Test whether this is a movable_ops page. Such pages will stay that
+ * way until freed.
+ *
+ * Returns true if this is a movable_ops page, otherwise false.
+ */
+static inline bool page_has_movable_ops(const struct page *page)
+{
+ return PageMovableOps(page) &&
+ (PageOffline(page) || PageZsmalloc(page));
+}
+
static __always_inline int PageAnonExclusive(const struct page *page)
{
VM_BUG_ON_PGFLAGS(!PageAnon(page), page);
diff --git a/mm/compaction.c b/mm/compaction.c
index 348eb754cb227..349f4ea0ec3e5 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -114,12 +114,6 @@ static unsigned long release_free_list(struct list_head *freepages)
}
#ifdef CONFIG_COMPACTION
-void __SetPageMovable(struct page *page)
-{
- VM_BUG_ON_PAGE(!PageLocked(page), page);
- page->mapping = (void *)(PAGE_MAPPING_MOVABLE);
-}
-EXPORT_SYMBOL(__SetPageMovable);
/* Do not skip compaction more than 64 times */
#define COMPACT_MAX_DEFER_SHIFT 6
diff --git a/mm/zpdesc.h b/mm/zpdesc.h
index 6855d9e2732d8..25bf5ea0beb83 100644
--- a/mm/zpdesc.h
+++ b/mm/zpdesc.h
@@ -154,7 +154,7 @@ static inline struct zpdesc *pfn_zpdesc(unsigned long pfn)
static inline void __zpdesc_set_movable(struct zpdesc *zpdesc)
{
- __SetPageMovable(zpdesc_page(zpdesc));
+ SetPageMovableOps(zpdesc_page(zpdesc));
}
static inline void __zpdesc_set_zsmalloc(struct zpdesc *zpdesc)
--
2.49.0On Mon, Jun 30, 2025 at 03:00:01PM +0200, David Hildenbrand wrote: > Instead, let's use a page flag. As the page flag can result in > false-positives, glue it to the page types for which we > support/implement movable_ops page migration. > > The flag reused by PageMovableOps() might be sued by other pages, so > warning in case it is set in page_has_movable_ops() might result in > false-positive warnings. > > Reviewed-by: Zi Yan <ziy@nvidia.com> > Signed-off-by: David Hildenbrand <david@redhat.com> > --- LGTM, Reviewed-by: Harry Yoo <harry.yoo@oracle.com> With a question: is there any reason to change the page flag operations to use atomic bit ops? (.e.g, using SetPageMovableOps instead of __SetPageMovableOps) -- Cheers, Harry / Hyeonggon
On 02.07.25 13:54, Harry Yoo wrote: > On Mon, Jun 30, 2025 at 03:00:01PM +0200, David Hildenbrand wrote: >> Instead, let's use a page flag. As the page flag can result in >> false-positives, glue it to the page types for which we >> support/implement movable_ops page migration. >> >> The flag reused by PageMovableOps() might be sued by other pages, so >> warning in case it is set in page_has_movable_ops() might result in >> false-positive warnings. >> >> Reviewed-by: Zi Yan <ziy@nvidia.com> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- > > LGTM, > Reviewed-by: Harry Yoo <harry.yoo@oracle.com> > > With a question: is there any reason to change the page flag > operations to use atomic bit ops? As we have the page lock in there, it's complicated. I thought about this when writing that code, and was not able to convince myself that it is safe. But that was when I was prototyping and reshuffling patches, and we would still have code that would clear the flag. Given that we only allow setting the flag, it might be okay to use the non-atomic variant as long as there can be nobody racing with us when modifying flags. Especially trying to lock the folio concurrently is the big problem. In isolate_movable_ops_page(), there is a comment about checking the flag before grabbing the page lock, so that should be handled. I'll have to check some other cases in balloon/zsmalloc code. -- Cheers, David / dhildenb
On Wed, Jul 02, 2025 at 02:01:33PM +0200, David Hildenbrand wrote: > On 02.07.25 13:54, Harry Yoo wrote: > > On Mon, Jun 30, 2025 at 03:00:01PM +0200, David Hildenbrand wrote: > > > Instead, let's use a page flag. As the page flag can result in > > > false-positives, glue it to the page types for which we > > > support/implement movable_ops page migration. > > > > > > The flag reused by PageMovableOps() might be sued by other pages, so > > > warning in case it is set in page_has_movable_ops() might result in > > > false-positive warnings. > > > > > > Reviewed-by: Zi Yan <ziy@nvidia.com> > > > Signed-off-by: David Hildenbrand <david@redhat.com> > > > --- > > > > LGTM, > > Reviewed-by: Harry Yoo <harry.yoo@oracle.com> > > > > With a question: is there any reason to change the page flag > > operations to use atomic bit ops? > > As we have the page lock in there, it's complicated. I thought about this > when writing that code, and was not able to convince myself that it is safe. > > But that was when I was prototyping and reshuffling patches, and we would > still have code that would clear the flag. > Given that we only allow setting the flag, it might be okay to use the > non-atomic variant as long as there can be nobody racing with us when > modifying flags. Especially trying to lock the folio concurrently is the big > problem. > > In isolate_movable_ops_page(), there is a comment about checking the flag > before grabbing the page lock, so that should be handled. Right. > I'll have to check some other cases in balloon/zsmalloc code. Okay, it's totally fine to go with atomic version and then switching back to non atomic ops when we're sure it's safe. -- Cheers, Harry / Hyeonggon
On 02.07.25 15:01, Harry Yoo wrote: > On Wed, Jul 02, 2025 at 02:01:33PM +0200, David Hildenbrand wrote: >> On 02.07.25 13:54, Harry Yoo wrote: >>> On Mon, Jun 30, 2025 at 03:00:01PM +0200, David Hildenbrand wrote: >>>> Instead, let's use a page flag. As the page flag can result in >>>> false-positives, glue it to the page types for which we >>>> support/implement movable_ops page migration. >>>> >>>> The flag reused by PageMovableOps() might be sued by other pages, so >>>> warning in case it is set in page_has_movable_ops() might result in >>>> false-positive warnings. >>>> >>>> Reviewed-by: Zi Yan <ziy@nvidia.com> >>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>> --- >>> >>> LGTM, >>> Reviewed-by: Harry Yoo <harry.yoo@oracle.com> >>> >>> With a question: is there any reason to change the page flag >>> operations to use atomic bit ops? >> >> As we have the page lock in there, it's complicated. I thought about this >> when writing that code, and was not able to convince myself that it is safe. >> >> But that was when I was prototyping and reshuffling patches, and we would >> still have code that would clear the flag. > >> Given that we only allow setting the flag, it might be okay to use the >> non-atomic variant as long as there can be nobody racing with us when >> modifying flags. Especially trying to lock the folio concurrently is the big >> problem. >> >> In isolate_movable_ops_page(), there is a comment about checking the flag >> before grabbing the page lock, so that should be handled. > > Right. > >> I'll have to check some other cases in balloon/zsmalloc code. > > Okay, it's totally fine to go with atomic version and then > switching back to non atomic ops when we're sure it's safe. > I'll definitely do the following: diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h index 8b23a74963feb..5f2b570735852 100644 --- a/include/linux/page-flags.h +++ b/include/linux/page-flags.h @@ -1145,9 +1145,11 @@ PAGEFLAG(Isolated, isolated, PF_ANY); * the flag might be used in other context for other pages. Always use * page_has_movable_ops() instead. */ -PAGEFLAG(MovableOps, movable_ops, PF_NO_TAIL); +TESTPAGEFLAG(MovableOps, movable_ops, PF_NO_TAIL); +SETPAGEFLAG(MovableOps, movable_ops, PF_NO_TAIL); #else /* !CONFIG_MIGRATION */ -PAGEFLAG_FALSE(MovableOps, movable_ops); +TESTPAGEFLAG_FALSE(MovableOps, movable_ops); +SETPAGEFLAG_NOOP(MovableOps, movable_ops); #endif /* CONFIG_MIGRATION */ /** Because the flag must not get cleared. There is no __SETPAGEFLAG_NOOP yet, unfortunately. -- Cheers, David / dhildenb
On Mon, Jun 30, 2025 at 03:00:01PM +0200, David Hildenbrand wrote:
> Instead, let's use a page flag. As the page flag can result in
> false-positives, glue it to the page types for which we
> support/implement movable_ops page migration.
>
> The flag reused by PageMovableOps() might be sued by other pages, so
I assume 'used' not 'sued' :P
> warning in case it is set in page_has_movable_ops() might result in
> false-positive warnings.
Worth mentioning that it's PG_uptodate. Also probably worth putting a proviso
here that we're safe to use it for movable ops pages because it's used to track
file system state.
>
> Reviewed-by: Zi Yan <ziy@nvidia.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
Seems reasonable though, so:
Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---
> include/linux/balloon_compaction.h | 2 +-
> include/linux/migrate.h | 8 -----
> include/linux/page-flags.h | 52 ++++++++++++++++++++++++------
> mm/compaction.c | 6 ----
> mm/zpdesc.h | 2 +-
> 5 files changed, 44 insertions(+), 26 deletions(-)
>
> diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h
> index a8a1706cc56f3..b222b0737c466 100644
> --- a/include/linux/balloon_compaction.h
> +++ b/include/linux/balloon_compaction.h
> @@ -92,7 +92,7 @@ static inline void balloon_page_insert(struct balloon_dev_info *balloon,
> struct page *page)
> {
> __SetPageOffline(page);
> - __SetPageMovable(page);
> + SetPageMovableOps(page);
> set_page_private(page, (unsigned long)balloon);
> list_add(&page->lru, &balloon->pages);
> }
> diff --git a/include/linux/migrate.h b/include/linux/migrate.h
> index 6aece3f3c8be8..acadd41e0b5cf 100644
> --- a/include/linux/migrate.h
> +++ b/include/linux/migrate.h
> @@ -103,14 +103,6 @@ static inline int migrate_huge_page_move_mapping(struct address_space *mapping,
>
> #endif /* CONFIG_MIGRATION */
>
> -#ifdef CONFIG_COMPACTION
> -void __SetPageMovable(struct page *page);
> -#else
> -static inline void __SetPageMovable(struct page *page)
> -{
> -}
> -#endif
> -
> #ifdef CONFIG_NUMA_BALANCING
> int migrate_misplaced_folio_prepare(struct folio *folio,
> struct vm_area_struct *vma, int node);
> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> index 4c27ebb689e3c..016a6e6fa428a 100644
> --- a/include/linux/page-flags.h
> +++ b/include/linux/page-flags.h
> @@ -170,6 +170,11 @@ enum pageflags {
> /* non-lru isolated movable page */
> PG_isolated = PG_reclaim,
>
> +#ifdef CONFIG_MIGRATION
> + /* this is a movable_ops page (for selected typed pages only) */
> + PG_movable_ops = PG_uptodate,
> +#endif
> +
> /* Only valid for buddy pages. Used to track pages that are reported */
> PG_reported = PG_uptodate,
>
> @@ -698,9 +703,6 @@ PAGEFLAG_FALSE(VmemmapSelfHosted, vmemmap_self_hosted)
> * bit; and then folio->mapping points, not to an anon_vma, but to a private
> * structure which KSM associates with that merged page. See ksm.h.
> *
> - * PAGE_MAPPING_KSM without PAGE_MAPPING_ANON is used for non-lru movable
> - * page and then folio->mapping points to a struct movable_operations.
> - *
> * Please note that, confusingly, "folio_mapping" refers to the inode
> * address_space which maps the folio from disk; whereas "folio_mapped"
> * refers to user virtual address space into which the folio is mapped.
> @@ -743,13 +745,6 @@ static __always_inline bool PageAnon(const struct page *page)
> {
> return folio_test_anon(page_folio(page));
> }
> -
> -static __always_inline bool page_has_movable_ops(const struct page *page)
> -{
> - return ((unsigned long)page->mapping & PAGE_MAPPING_FLAGS) ==
> - PAGE_MAPPING_MOVABLE;
> -}
> -
> #ifdef CONFIG_KSM
> /*
> * A KSM page is one of those write-protected "shared pages" or "merged pages"
> @@ -1133,6 +1128,43 @@ bool is_free_buddy_page(const struct page *page);
>
> PAGEFLAG(Isolated, isolated, PF_ANY);
>
> +#ifdef CONFIG_MIGRATION
> +/*
> + * This page is migratable through movable_ops (for selected typed pages
> + * only).
> + *
> + * Page migration of such pages might fail, for example, if the page is
> + * already isolated by somebody else, or if the page is about to get freed.
> + *
> + * While a subsystem might set selected typed pages that support page migration
> + * as being movable through movable_ops, it must never clear this flag.
> + *
> + * This flag is only cleared when the page is freed back to the buddy.
> + *
> + * Only selected page types support this flag (see page_movable_ops()) and
> + * the flag might be used in other context for other pages. Always use
> + * page_has_movable_ops() instead.
> + */
> +PAGEFLAG(MovableOps, movable_ops, PF_NO_TAIL);
> +#else
> +PAGEFLAG_FALSE(MovableOps, movable_ops);
> +#endif
> +
> +/**
> + * page_has_movable_ops - test for a movable_ops page
> + * @page The page to test.
> + *
> + * Test whether this is a movable_ops page. Such pages will stay that
> + * way until freed.
> + *
> + * Returns true if this is a movable_ops page, otherwise false.
> + */
> +static inline bool page_has_movable_ops(const struct page *page)
> +{
> + return PageMovableOps(page) &&
> + (PageOffline(page) || PageZsmalloc(page));
> +}
> +
> static __always_inline int PageAnonExclusive(const struct page *page)
> {
> VM_BUG_ON_PGFLAGS(!PageAnon(page), page);
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 348eb754cb227..349f4ea0ec3e5 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -114,12 +114,6 @@ static unsigned long release_free_list(struct list_head *freepages)
> }
>
> #ifdef CONFIG_COMPACTION
> -void __SetPageMovable(struct page *page)
> -{
> - VM_BUG_ON_PAGE(!PageLocked(page), page);
> - page->mapping = (void *)(PAGE_MAPPING_MOVABLE);
> -}
> -EXPORT_SYMBOL(__SetPageMovable);
>
> /* Do not skip compaction more than 64 times */
> #define COMPACT_MAX_DEFER_SHIFT 6
> diff --git a/mm/zpdesc.h b/mm/zpdesc.h
> index 6855d9e2732d8..25bf5ea0beb83 100644
> --- a/mm/zpdesc.h
> +++ b/mm/zpdesc.h
> @@ -154,7 +154,7 @@ static inline struct zpdesc *pfn_zpdesc(unsigned long pfn)
>
> static inline void __zpdesc_set_movable(struct zpdesc *zpdesc)
> {
> - __SetPageMovable(zpdesc_page(zpdesc));
> + SetPageMovableOps(zpdesc_page(zpdesc));
> }
>
> static inline void __zpdesc_set_zsmalloc(struct zpdesc *zpdesc)
> --
> 2.49.0
>
On 01.07.25 14:44, Lorenzo Stoakes wrote: > On Mon, Jun 30, 2025 at 03:00:01PM +0200, David Hildenbrand wrote: >> Instead, let's use a page flag. As the page flag can result in >> false-positives, glue it to the page types for which we >> support/implement movable_ops page migration. >> >> The flag reused by PageMovableOps() might be sued by other pages, so > > I assume 'used' not 'sued' :P :) > >> warning in case it is set in page_has_movable_ops() might result in >> false-positive warnings. > > Worth mentioning that it's PG_uptodate. Also probably worth putting a proviso > here that we're safe to use it for movable ops pages because it's used to track > file system state. Will do. > >> >> Reviewed-by: Zi Yan <ziy@nvidia.com> >> Signed-off-by: David Hildenbrand <david@redhat.com> > > Seems reasonable though, so: > > Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> Thanks! -- Cheers, David / dhildenb
© 2016 - 2025 Red Hat, Inc.