During page isolation, the original migratetype is overwritten, since
MIGRATE_* are enums and stored in pageblock bitmaps. Change
MIGRATE_ISOLATE to be stored a standalone bit, PB_migrate_isolate, like
PB_migrate_skip, so that migratetype is not lost during pageblock
isolation. pageblock bits needs to be word aligned, so expand
the number of pageblock bits from 4 to 8 and make PB_migrate_isolate bit 7.

Signed-off-by: Zi Yan <ziy@nvidia.com>
---
 include/linux/mmzone.h          | 17 ++++++++++----
 include/linux/page-isolation.h  |  2 +-
 include/linux/pageblock-flags.h | 33 +++++++++++++++++++++++++-
 mm/page_alloc.c                 | 41 ++++++++++++++++++++++++++++++++-
 4 files changed, 86 insertions(+), 7 deletions(-)

diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index b19a98c20de8..9ec022a0b826 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -106,14 +106,23 @@ static inline bool migratetype_is_mergeable(int mt)
 
 extern int page_group_by_mobility_disabled;
 
-#define MIGRATETYPE_MASK ((1UL << PB_migratetype_bits) - 1)
+#ifdef CONFIG_MEMORY_ISOLATION
+#define MIGRATETYPE_MASK ((BIT(PB_migratetype_bits) - 1) | PB_migrate_isolate_bit)
+#else
+#define MIGRATETYPE_MASK (BIT(PB_migratetype_bits) - 1)
+#endif
 
+#ifdef CONFIG_MEMORY_ISOLATION
+unsigned long get_pageblock_migratetype(const struct page *page);
+#else
 #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)
+#endif
+
+#define folio_migratetype(folio)					\
+	get_pageblock_migratetype(&folio->page)
+
 struct free_area {
 	struct list_head	free_list[MIGRATE_TYPES];
 	unsigned long		nr_free;
diff --git a/include/linux/page-isolation.h b/include/linux/page-isolation.h
index 898bb788243b..51797dc39cbc 100644
--- a/include/linux/page-isolation.h
+++ b/include/linux/page-isolation.h
@@ -5,7 +5,7 @@
 #ifdef CONFIG_MEMORY_ISOLATION
 static inline bool is_migrate_isolate_page(struct page *page)
 {
-	return get_pageblock_migratetype(page) == MIGRATE_ISOLATE;
+	return get_pageblock_isolate(page);
 }
 static inline bool is_migrate_isolate(int migratetype)
 {
diff --git a/include/linux/pageblock-flags.h b/include/linux/pageblock-flags.h
index 0c4963339f0b..9fadae5892b2 100644
--- a/include/linux/pageblock-flags.h
+++ b/include/linux/pageblock-flags.h
@@ -20,7 +20,10 @@ enum pageblock_bits {
 	PB_migrate_end = PB_migrate + PB_migratetype_bits - 1,
 			/* 3 bits required for migrate types */
 	PB_migrate_skip,/* If set the block is skipped by compaction */
-
+#ifdef CONFIG_MEMORY_ISOLATION
+	PB_migrate_isolate = 7, /* If set the block is isolated */
+			/* set it to 7 to make pageblock bit word aligned */
+#endif
 	/*
 	 * Assume the bits will always align on a word. If this assumption
 	 * changes then get/set pageblock needs updating.
@@ -28,6 +31,10 @@ enum pageblock_bits {
 	NR_PAGEBLOCK_BITS
 };
 
+#ifdef CONFIG_MEMORY_ISOLATION
+#define PB_migrate_isolate_bit BIT(PB_migrate_isolate)
+#endif
+
 #if defined(CONFIG_PAGE_BLOCK_ORDER)
 #define PAGE_BLOCK_ORDER CONFIG_PAGE_BLOCK_ORDER
 #else
@@ -105,4 +112,28 @@ static inline void set_pageblock_skip(struct page *page)
 }
 #endif /* CONFIG_COMPACTION */
 
+#ifdef CONFIG_MEMORY_ISOLATION
+#define get_pageblock_isolate(page) \
+	get_pfnblock_flags_mask(page, page_to_pfn(page),	\
+			PB_migrate_isolate_bit)
+#define clear_pageblock_isolate(page) \
+	set_pfnblock_flags_mask(page, 0, page_to_pfn(page),	\
+			PB_migrate_isolate_bit)
+#define set_pageblock_isolate(page) \
+	set_pfnblock_flags_mask(page, PB_migrate_isolate_bit,	\
+			page_to_pfn(page),			\
+			PB_migrate_isolate_bit)
+#else
+static inline bool get_pageblock_isolate(struct page *page)
+{
+	return false;
+}
+static inline void clear_pageblock_isolate(struct page *page)
+{
+}
+static inline void set_pageblock_isolate(struct page *page)
+{
+}
+#endif /* CONFIG_MEMORY_ISOLATION */
+
 #endif	/* PAGEBLOCK_FLAGS_H */
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index c77592b22256..acf68ef041d8 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -381,12 +381,40 @@ unsigned long get_pfnblock_flags_mask(const struct page *page,
 	return (word >> bitidx) & mask;
 }
 
+#ifdef CONFIG_MEMORY_ISOLATION
+unsigned long get_pageblock_migratetype(const struct page *page)
+{
+	unsigned long flags;
+
+	flags = get_pfnblock_flags_mask(page, page_to_pfn(page),
+			MIGRATETYPE_MASK);
+	if (flags & PB_migrate_isolate_bit)
+		return MIGRATE_ISOLATE;
+
+	return flags;
+}
+
+static __always_inline int get_pfnblock_migratetype(const struct page *page,
+					unsigned long pfn)
+{
+	unsigned long flags;
+
+	flags = get_pfnblock_flags_mask(page, pfn,
+			MIGRATETYPE_MASK);
+	if (flags & PB_migrate_isolate_bit)
+		return MIGRATE_ISOLATE;
+
+	return flags;
+}
+#else
 static __always_inline int get_pfnblock_migratetype(const struct page *page,
 					unsigned long pfn)
 {
 	return get_pfnblock_flags_mask(page, pfn, MIGRATETYPE_MASK);
 }
 
+#endif
+
 /**
  * set_pfnblock_flags_mask - Set the requested group of flags for a pageblock_nr_pages block of pages
  * @page: The page within the block of interest
@@ -402,8 +430,14 @@ void set_pfnblock_flags_mask(struct page *page, unsigned long flags,
 	unsigned long bitidx, word_bitidx;
 	unsigned long word;
 
+#ifdef CONFIG_MEMORY_ISOLATION
+	BUILD_BUG_ON(NR_PAGEBLOCK_BITS != 8);
+	/* extra one for MIGRATE_ISOLATE */
+	BUILD_BUG_ON(MIGRATE_TYPES > (1 << PB_migratetype_bits) + 1);
+#else
 	BUILD_BUG_ON(NR_PAGEBLOCK_BITS != 4);
 	BUILD_BUG_ON(MIGRATE_TYPES > (1 << PB_migratetype_bits));
+#endif
 
 	bitmap = get_pageblock_bitmap(page, pfn);
 	bitidx = pfn_to_bitidx(page, pfn);
@@ -426,7 +460,12 @@ void set_pageblock_migratetype(struct page *page, int migratetype)
 		     migratetype < MIGRATE_PCPTYPES))
 		migratetype = MIGRATE_UNMOVABLE;
 
-	set_pfnblock_flags_mask(page, (unsigned long)migratetype,
+#ifdef CONFIG_MEMORY_ISOLATION
+	if (migratetype == MIGRATE_ISOLATE)
+		set_pageblock_isolate(page);
+	else
+#endif
+		set_pfnblock_flags_mask(page, (unsigned long)migratetype,
 				page_to_pfn(page), MIGRATETYPE_MASK);
 }
 
-- 
2.47.2

On 7 May 2025, at 17:10, Zi Yan wrote:

> During page isolation, the original migratetype is overwritten, since
> MIGRATE_* are enums and stored in pageblock bitmaps. Change
> MIGRATE_ISOLATE to be stored a standalone bit, PB_migrate_isolate, like
> PB_migrate_skip, so that migratetype is not lost during pageblock
> isolation. pageblock bits needs to be word aligned, so expand
> the number of pageblock bits from 4 to 8 and make PB_migrate_isolate bit 7.
>
> Signed-off-by: Zi Yan <ziy@nvidia.com>
> ---
>  include/linux/mmzone.h          | 17 ++++++++++----
>  include/linux/page-isolation.h  |  2 +-
>  include/linux/pageblock-flags.h | 33 +++++++++++++++++++++++++-
>  mm/page_alloc.c                 | 41 ++++++++++++++++++++++++++++++++-
>  4 files changed, 86 insertions(+), 7 deletions(-)
>

Here is the fixup 1/3 to address Johannes’ comments.

From 7eb1d9fa58fdab216862436181e5d2baf2958c54 Mon Sep 17 00:00:00 2001
From: Zi Yan <ziy@nvidia.com>
Date: Thu, 8 May 2025 12:05:59 -0400
Subject: [PATCH] fixup for mm/page_isolation: make page isolation a standalone
 bit

1. keep the original is_migrate_isolate_page()
2. move {get,set,clear}_pageblock_isolate() to mm/page_isolation.c
3. use a single version for get_pageblock_migratetype() and
   get_pfnblock_migratetype().

Signed-off-by: Zi Yan <ziy@nvidia.com>
---
 include/linux/mmzone.h          |  6 ------
 include/linux/page-isolation.h  |  2 +-
 include/linux/pageblock-flags.h | 24 ------------------------
 mm/page_alloc.c                 | 25 ++++++++++---------------
 mm/page_isolation.c             | 17 +++++++++++++++++
 5 files changed, 28 insertions(+), 46 deletions(-)

diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 9ec022a0b826..7ef01fe148ce 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -112,13 +112,7 @@ extern int page_group_by_mobility_disabled;
 #define MIGRATETYPE_MASK (BIT(PB_migratetype_bits) - 1)
 #endif

-#ifdef CONFIG_MEMORY_ISOLATION
 unsigned long get_pageblock_migratetype(const struct page *page);
-#else
-#define get_pageblock_migratetype(page)					\
-	get_pfnblock_flags_mask(page, page_to_pfn(page), MIGRATETYPE_MASK)
-
-#endif

 #define folio_migratetype(folio)					\
 	get_pageblock_migratetype(&folio->page)
diff --git a/include/linux/page-isolation.h b/include/linux/page-isolation.h
index 51797dc39cbc..898bb788243b 100644
--- a/include/linux/page-isolation.h
+++ b/include/linux/page-isolation.h
@@ -5,7 +5,7 @@
 #ifdef CONFIG_MEMORY_ISOLATION
 static inline bool is_migrate_isolate_page(struct page *page)
 {
-	return get_pageblock_isolate(page);
+	return get_pageblock_migratetype(page) == MIGRATE_ISOLATE;
 }
 static inline bool is_migrate_isolate(int migratetype)
 {
diff --git a/include/linux/pageblock-flags.h b/include/linux/pageblock-flags.h
index 9fadae5892b2..00040e7df8c8 100644
--- a/include/linux/pageblock-flags.h
+++ b/include/linux/pageblock-flags.h
@@ -112,28 +112,4 @@ static inline void set_pageblock_skip(struct page *page)
 }
 #endif /* CONFIG_COMPACTION */

-#ifdef CONFIG_MEMORY_ISOLATION
-#define get_pageblock_isolate(page) \
-	get_pfnblock_flags_mask(page, page_to_pfn(page),	\
-			PB_migrate_isolate_bit)
-#define clear_pageblock_isolate(page) \
-	set_pfnblock_flags_mask(page, 0, page_to_pfn(page),	\
-			PB_migrate_isolate_bit)
-#define set_pageblock_isolate(page) \
-	set_pfnblock_flags_mask(page, PB_migrate_isolate_bit,	\
-			page_to_pfn(page),			\
-			PB_migrate_isolate_bit)
-#else
-static inline bool get_pageblock_isolate(struct page *page)
-{
-	return false;
-}
-static inline void clear_pageblock_isolate(struct page *page)
-{
-}
-static inline void set_pageblock_isolate(struct page *page)
-{
-}
-#endif /* CONFIG_MEMORY_ISOLATION */
-
 #endif	/* PAGEBLOCK_FLAGS_H */
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index acf68ef041d8..04e301fb4879 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -381,16 +381,16 @@ unsigned long get_pfnblock_flags_mask(const struct page *page,
 	return (word >> bitidx) & mask;
 }

-#ifdef CONFIG_MEMORY_ISOLATION
 unsigned long get_pageblock_migratetype(const struct page *page)
 {
 	unsigned long flags;

 	flags = get_pfnblock_flags_mask(page, page_to_pfn(page),
 			MIGRATETYPE_MASK);
+#ifdef CONFIG_MEMORY_ISOLATION
 	if (flags & PB_migrate_isolate_bit)
 		return MIGRATE_ISOLATE;
-
+#endif
 	return flags;
 }

@@ -401,19 +401,12 @@ static __always_inline int get_pfnblock_migratetype(const struct page *page,

 	flags = get_pfnblock_flags_mask(page, pfn,
 			MIGRATETYPE_MASK);
+#ifdef CONFIG_MEMORY_ISOLATION
 	if (flags & PB_migrate_isolate_bit)
 		return MIGRATE_ISOLATE;
-
+#endif
 	return flags;
 }
-#else
-static __always_inline int get_pfnblock_migratetype(const struct page *page,
-					unsigned long pfn)
-{
-	return get_pfnblock_flags_mask(page, pfn, MIGRATETYPE_MASK);
-}
-
-#endif

 /**
  * set_pfnblock_flags_mask - Set the requested group of flags for a pageblock_nr_pages block of pages
@@ -461,11 +454,13 @@ void set_pageblock_migratetype(struct page *page, int migratetype)
 		migratetype = MIGRATE_UNMOVABLE;

 #ifdef CONFIG_MEMORY_ISOLATION
-	if (migratetype == MIGRATE_ISOLATE)
-		set_pageblock_isolate(page);
-	else
+	if (migratetype == MIGRATE_ISOLATE) {
+		set_pfnblock_flags_mask(page, PB_migrate_isolate_bit,
+				page_to_pfn(page), PB_migrate_isolate_bit);
+		return;
+	}
 #endif
-		set_pfnblock_flags_mask(page, (unsigned long)migratetype,
+	set_pfnblock_flags_mask(page, (unsigned long)migratetype,
 				page_to_pfn(page), MIGRATETYPE_MASK);
 }

diff --git a/mm/page_isolation.c b/mm/page_isolation.c
index b2fc5266e3d2..b6a62132e20e 100644
--- a/mm/page_isolation.c
+++ b/mm/page_isolation.c
@@ -15,6 +15,23 @@
 #define CREATE_TRACE_POINTS
 #include <trace/events/page_isolation.h>

+static inline bool get_pageblock_isolate(struct page *page)
+{
+	return get_pfnblock_flags_mask(page, page_to_pfn(page),
+			PB_migrate_isolate_bit);
+}
+static inline void clear_pageblock_isolate(struct page *page)
+{
+	set_pfnblock_flags_mask(page, 0, page_to_pfn(page),
+			PB_migrate_isolate_bit);
+}
+static inline void set_pageblock_isolate(struct page *page)
+{
+	set_pfnblock_flags_mask(page, PB_migrate_isolate_bit,
+			page_to_pfn(page),
+			PB_migrate_isolate_bit);
+}
+
 /*
  * This function checks whether the range [start_pfn, end_pfn) includes
  * unmovable pages or not. The range must fall into a single pageblock and
-- 
2.47.2



Best Regards,
Yan, Zi

On 8 May 2025, at 16:22, Zi Yan wrote:

> On 7 May 2025, at 17:10, Zi Yan wrote:
>
>> During page isolation, the original migratetype is overwritten, since
>> MIGRATE_* are enums and stored in pageblock bitmaps. Change
>> MIGRATE_ISOLATE to be stored a standalone bit, PB_migrate_isolate, like
>> PB_migrate_skip, so that migratetype is not lost during pageblock
>> isolation. pageblock bits needs to be word aligned, so expand
>> the number of pageblock bits from 4 to 8 and make PB_migrate_isolate bit 7.
>>
>> Signed-off-by: Zi Yan <ziy@nvidia.com>
>> ---
>>  include/linux/mmzone.h          | 17 ++++++++++----
>>  include/linux/page-isolation.h  |  2 +-
>>  include/linux/pageblock-flags.h | 33 +++++++++++++++++++++++++-
>>  mm/page_alloc.c                 | 41 ++++++++++++++++++++++++++++++++-
>>  4 files changed, 86 insertions(+), 7 deletions(-)
>>
>
> Here is the fixup 1/3 to address Johannes’ comments.
>
> From 7eb1d9fa58fdab216862436181e5d2baf2958c54 Mon Sep 17 00:00:00 2001
> From: Zi Yan <ziy@nvidia.com>
> Date: Thu, 8 May 2025 12:05:59 -0400
> Subject: [PATCH] fixup for mm/page_isolation: make page isolation a standalone
>  bit
>
> 1. keep the original is_migrate_isolate_page()
> 2. move {get,set,clear}_pageblock_isolate() to mm/page_isolation.c
> 3. use a single version for get_pageblock_migratetype() and
>    get_pfnblock_migratetype().
>
> Signed-off-by: Zi Yan <ziy@nvidia.com>
> ---
>  include/linux/mmzone.h          |  6 ------
>  include/linux/page-isolation.h  |  2 +-
>  include/linux/pageblock-flags.h | 24 ------------------------
>  mm/page_alloc.c                 | 25 ++++++++++---------------
>  mm/page_isolation.c             | 17 +++++++++++++++++
>  5 files changed, 28 insertions(+), 46 deletions(-)
>

Hi Andrew,

This fixup gets rid of the unused function warning[1].

BTW, this series gets several fixups. Let me know if you think a V4
is necessary. Thanks.

[1] https://lore.kernel.org/linux-mm/202505092126.yRGhLhg1-lkp@intel.com/

From 126d116cb737bf3d8c475460b9b48edc6696e6dc Mon Sep 17 00:00:00 2001
From: Zi Yan <ziy@nvidia.com>
Date: Fri, 9 May 2025 09:29:09 -0400
Subject: [PATCH] fix unused function warnings

1. add __maybe_unused to get_pageblock_isolate(), which is used in
   VM_BUG_ON only.
2. remove unused set_pageblock_isolate().

Signed-off-by: Zi Yan <ziy@nvidia.com>
---
 mm/page_isolation.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/mm/page_isolation.c b/mm/page_isolation.c
index b0f83a7c508d..ce7e38c9f839 100644
--- a/mm/page_isolation.c
+++ b/mm/page_isolation.c
@@ -15,7 +15,7 @@
 #define CREATE_TRACE_POINTS
 #include <trace/events/page_isolation.h>

-static inline bool get_pageblock_isolate(struct page *page)
+static inline bool __maybe_unused get_pageblock_isolate(struct page *page)
 {
 	return get_pfnblock_flags_mask(page, page_to_pfn(page),
 			PB_migrate_isolate_bit);
@@ -25,12 +25,6 @@ static inline void clear_pageblock_isolate(struct page *page)
 	set_pfnblock_flags_mask(page, 0, page_to_pfn(page),
 			PB_migrate_isolate_bit);
 }
-static inline void set_pageblock_isolate(struct page *page)
-{
-	set_pfnblock_flags_mask(page, PB_migrate_isolate_bit,
-			page_to_pfn(page),
-			PB_migrate_isolate_bit);
-}

 /*
  * This function checks whether the range [start_pfn, end_pfn) includes
-- 
2.47.2



--
Best Regards,
Yan, Zi

On Thu, May 08, 2025 at 04:22:43PM -0400, Zi Yan wrote:
> On 7 May 2025, at 17:10, Zi Yan wrote:
> 
> > During page isolation, the original migratetype is overwritten, since
> > MIGRATE_* are enums and stored in pageblock bitmaps. Change
> > MIGRATE_ISOLATE to be stored a standalone bit, PB_migrate_isolate, like
> > PB_migrate_skip, so that migratetype is not lost during pageblock
> > isolation. pageblock bits needs to be word aligned, so expand
> > the number of pageblock bits from 4 to 8 and make PB_migrate_isolate bit 7.
> >
> > Signed-off-by: Zi Yan <ziy@nvidia.com>

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

> > ---
> >  include/linux/mmzone.h          | 17 ++++++++++----
> >  include/linux/page-isolation.h  |  2 +-
> >  include/linux/pageblock-flags.h | 33 +++++++++++++++++++++++++-
> >  mm/page_alloc.c                 | 41 ++++++++++++++++++++++++++++++++-
> >  4 files changed, 86 insertions(+), 7 deletions(-)
> >
> 
> Here is the fixup 1/3 to address Johannes’ comments.

Thanks!

On Wed, May 07, 2025 at 05:10:56PM -0400, Zi Yan wrote:
> During page isolation, the original migratetype is overwritten, since
> MIGRATE_* are enums and stored in pageblock bitmaps. Change
> MIGRATE_ISOLATE to be stored a standalone bit, PB_migrate_isolate, like
> PB_migrate_skip, so that migratetype is not lost during pageblock
> isolation. pageblock bits needs to be word aligned, so expand
> the number of pageblock bits from 4 to 8 and make PB_migrate_isolate bit 7.
> 
> Signed-off-by: Zi Yan <ziy@nvidia.com>
> ---
>  include/linux/mmzone.h          | 17 ++++++++++----
>  include/linux/page-isolation.h  |  2 +-
>  include/linux/pageblock-flags.h | 33 +++++++++++++++++++++++++-
>  mm/page_alloc.c                 | 41 ++++++++++++++++++++++++++++++++-
>  4 files changed, 86 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index b19a98c20de8..9ec022a0b826 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -106,14 +106,23 @@ static inline bool migratetype_is_mergeable(int mt)
>  
>  extern int page_group_by_mobility_disabled;
>  
> -#define MIGRATETYPE_MASK ((1UL << PB_migratetype_bits) - 1)
> +#ifdef CONFIG_MEMORY_ISOLATION
> +#define MIGRATETYPE_MASK ((BIT(PB_migratetype_bits) - 1) | PB_migrate_isolate_bit)
> +#else
> +#define MIGRATETYPE_MASK (BIT(PB_migratetype_bits) - 1)
> +#endif
>  
> +#ifdef CONFIG_MEMORY_ISOLATION
> +unsigned long get_pageblock_migratetype(const struct page *page);
> +#else
>  #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)
> +#endif
> +
> +#define folio_migratetype(folio)					\
> +	get_pageblock_migratetype(&folio->page)
> +
>  struct free_area {
>  	struct list_head	free_list[MIGRATE_TYPES];
>  	unsigned long		nr_free;
> diff --git a/include/linux/page-isolation.h b/include/linux/page-isolation.h
> index 898bb788243b..51797dc39cbc 100644
> --- a/include/linux/page-isolation.h
> +++ b/include/linux/page-isolation.h
> @@ -5,7 +5,7 @@
>  #ifdef CONFIG_MEMORY_ISOLATION
>  static inline bool is_migrate_isolate_page(struct page *page)
>  {
> -	return get_pageblock_migratetype(page) == MIGRATE_ISOLATE;
> +	return get_pageblock_isolate(page);

The old version still works, right?

It would match is_migrate_isolate() a bit better, but no strong
feelings either way...

>  static inline bool is_migrate_isolate(int migratetype)
>  {
> diff --git a/include/linux/pageblock-flags.h b/include/linux/pageblock-flags.h
> index 0c4963339f0b..9fadae5892b2 100644
> --- a/include/linux/pageblock-flags.h
> +++ b/include/linux/pageblock-flags.h
> @@ -20,7 +20,10 @@ enum pageblock_bits {
>  	PB_migrate_end = PB_migrate + PB_migratetype_bits - 1,
>  			/* 3 bits required for migrate types */
>  	PB_migrate_skip,/* If set the block is skipped by compaction */
> -
> +#ifdef CONFIG_MEMORY_ISOLATION
> +	PB_migrate_isolate = 7, /* If set the block is isolated */
> +			/* set it to 7 to make pageblock bit word aligned */
> +#endif
>  	/*
>  	 * Assume the bits will always align on a word. If this assumption
>  	 * changes then get/set pageblock needs updating.
> @@ -28,6 +31,10 @@ enum pageblock_bits {
>  	NR_PAGEBLOCK_BITS
>  };
>  
> +#ifdef CONFIG_MEMORY_ISOLATION
> +#define PB_migrate_isolate_bit BIT(PB_migrate_isolate)
> +#endif
> +
>  #if defined(CONFIG_PAGE_BLOCK_ORDER)
>  #define PAGE_BLOCK_ORDER CONFIG_PAGE_BLOCK_ORDER
>  #else
> @@ -105,4 +112,28 @@ static inline void set_pageblock_skip(struct page *page)
>  }
>  #endif /* CONFIG_COMPACTION */
>  
> +#ifdef CONFIG_MEMORY_ISOLATION
> +#define get_pageblock_isolate(page) \
> +	get_pfnblock_flags_mask(page, page_to_pfn(page),	\
> +			PB_migrate_isolate_bit)
> +#define clear_pageblock_isolate(page) \
> +	set_pfnblock_flags_mask(page, 0, page_to_pfn(page),	\
> +			PB_migrate_isolate_bit)
> +#define set_pageblock_isolate(page) \
> +	set_pfnblock_flags_mask(page, PB_migrate_isolate_bit,	\
> +			page_to_pfn(page),			\
> +			PB_migrate_isolate_bit)

Would it make sense to move these to page_isolation.c? Then they
wouldn't have to be macros.

> +#else
> +static inline bool get_pageblock_isolate(struct page *page)
> +{
> +	return false;
> +}
> +static inline void clear_pageblock_isolate(struct page *page)
> +{
> +}
> +static inline void set_pageblock_isolate(struct page *page)
> +{
> +}
> +#endif /* CONFIG_MEMORY_ISOLATION */
> +
>  #endif	/* PAGEBLOCK_FLAGS_H */
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index c77592b22256..acf68ef041d8 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -381,12 +381,40 @@ unsigned long get_pfnblock_flags_mask(const struct page *page,
>  	return (word >> bitidx) & mask;
>  }
>  
> +#ifdef CONFIG_MEMORY_ISOLATION
> +unsigned long get_pageblock_migratetype(const struct page *page)
> +{
> +	unsigned long flags;
> +
> +	flags = get_pfnblock_flags_mask(page, page_to_pfn(page),
> +			MIGRATETYPE_MASK);
> +	if (flags & PB_migrate_isolate_bit)
> +		return MIGRATE_ISOLATE;
> +
> +	return flags;
> +}

Since MIGRATETYPE_MASK includes the isolate bit if it exists, I think
you can share the get_pfnblock_flags_mask() part:

static inline get_pageblock_migratetype(const struct page *page)
{
	unsigned long pfn = page_to_pfn(page);

	flags = get_pfnblock_flags_mask(page, pfn, MIGRATETYPE_MASK);
#ifdef CONFIG_MEMORY_ISOLATEION
	if (flags & PB_migrate_isolate_bit)
		return MIGRATE_ISOLATE;
#endif
	return flags;
}

> +static __always_inline int get_pfnblock_migratetype(const struct page *page,
> +					unsigned long pfn)
> +{
> +	unsigned long flags;
> +
> +	flags = get_pfnblock_flags_mask(page, pfn,
> +			MIGRATETYPE_MASK);
> +	if (flags & PB_migrate_isolate_bit)
> +		return MIGRATE_ISOLATE;
> +
> +	return flags;
> +}
> +#else
>  static __always_inline int get_pfnblock_migratetype(const struct page *page,
>  					unsigned long pfn)
>  {
>  	return get_pfnblock_flags_mask(page, pfn, MIGRATETYPE_MASK);
>  }

Same with this one.

>  
> +#endif
> +
>  /**
>   * set_pfnblock_flags_mask - Set the requested group of flags for a pageblock_nr_pages block of pages
>   * @page: The page within the block of interest
> @@ -402,8 +430,14 @@ void set_pfnblock_flags_mask(struct page *page, unsigned long flags,
>  	unsigned long bitidx, word_bitidx;
>  	unsigned long word;
>  
> +#ifdef CONFIG_MEMORY_ISOLATION
> +	BUILD_BUG_ON(NR_PAGEBLOCK_BITS != 8);
> +	/* extra one for MIGRATE_ISOLATE */
> +	BUILD_BUG_ON(MIGRATE_TYPES > (1 << PB_migratetype_bits) + 1);
> +#else
>  	BUILD_BUG_ON(NR_PAGEBLOCK_BITS != 4);
>  	BUILD_BUG_ON(MIGRATE_TYPES > (1 << PB_migratetype_bits));
> +#endif
>  
>  	bitmap = get_pageblock_bitmap(page, pfn);
>  	bitidx = pfn_to_bitidx(page, pfn);
> @@ -426,7 +460,12 @@ void set_pageblock_migratetype(struct page *page, int migratetype)
>  		     migratetype < MIGRATE_PCPTYPES))
>  		migratetype = MIGRATE_UNMOVABLE;
>  
> -	set_pfnblock_flags_mask(page, (unsigned long)migratetype,
> +#ifdef CONFIG_MEMORY_ISOLATION
> +	if (migratetype == MIGRATE_ISOLATE)
> +		set_pageblock_isolate(page);

Are there paths actually doing this after the second patch?

There are many instances that want to *read* the migratetype or
MIGRATE_ISOLATE, but only isolation code should be manipulating that
bit through the dedicated set/toggle_pageblock_isolate API.

If there isn't one, it might be good to enforce this with a VM_WARN
instead.

> +	else
> +#endif
> +		set_pfnblock_flags_mask(page, (unsigned long)migratetype,
>  				page_to_pfn(page), MIGRATETYPE_MASK);

If the branch stays, you could add a `return' to the MIGRATE_ISOLATE
leg, drop the else and indent this line normally.

On 8 May 2025, at 1:24, Johannes Weiner wrote:

> On Wed, May 07, 2025 at 05:10:56PM -0400, Zi Yan wrote:
>> During page isolation, the original migratetype is overwritten, since
>> MIGRATE_* are enums and stored in pageblock bitmaps. Change
>> MIGRATE_ISOLATE to be stored a standalone bit, PB_migrate_isolate, like
>> PB_migrate_skip, so that migratetype is not lost during pageblock
>> isolation. pageblock bits needs to be word aligned, so expand
>> the number of pageblock bits from 4 to 8 and make PB_migrate_isolate bit 7.
>>
>> Signed-off-by: Zi Yan <ziy@nvidia.com>
>> ---
>>  include/linux/mmzone.h          | 17 ++++++++++----
>>  include/linux/page-isolation.h  |  2 +-
>>  include/linux/pageblock-flags.h | 33 +++++++++++++++++++++++++-
>>  mm/page_alloc.c                 | 41 ++++++++++++++++++++++++++++++++-
>>  4 files changed, 86 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
>> index b19a98c20de8..9ec022a0b826 100644
>> --- a/include/linux/mmzone.h
>> +++ b/include/linux/mmzone.h
>> @@ -106,14 +106,23 @@ static inline bool migratetype_is_mergeable(int mt)
>>
>>  extern int page_group_by_mobility_disabled;
>>
>> -#define MIGRATETYPE_MASK ((1UL << PB_migratetype_bits) - 1)
>> +#ifdef CONFIG_MEMORY_ISOLATION
>> +#define MIGRATETYPE_MASK ((BIT(PB_migratetype_bits) - 1) | PB_migrate_isolate_bit)
>> +#else
>> +#define MIGRATETYPE_MASK (BIT(PB_migratetype_bits) - 1)
>> +#endif
>>
>> +#ifdef CONFIG_MEMORY_ISOLATION
>> +unsigned long get_pageblock_migratetype(const struct page *page);
>> +#else
>>  #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)
>> +#endif
>> +
>> +#define folio_migratetype(folio)					\
>> +	get_pageblock_migratetype(&folio->page)
>> +
>>  struct free_area {
>>  	struct list_head	free_list[MIGRATE_TYPES];
>>  	unsigned long		nr_free;
>> diff --git a/include/linux/page-isolation.h b/include/linux/page-isolation.h
>> index 898bb788243b..51797dc39cbc 100644
>> --- a/include/linux/page-isolation.h
>> +++ b/include/linux/page-isolation.h
>> @@ -5,7 +5,7 @@
>>  #ifdef CONFIG_MEMORY_ISOLATION
>>  static inline bool is_migrate_isolate_page(struct page *page)
>>  {
>> -	return get_pageblock_migratetype(page) == MIGRATE_ISOLATE;
>> +	return get_pageblock_isolate(page);
>
> The old version still works, right?
>
> It would match is_migrate_isolate() a bit better, but no strong
> feelings either way...

Yes and compiler should generate the same code. OK, I will drop this.

>
>>  static inline bool is_migrate_isolate(int migratetype)
>>  {
>> diff --git a/include/linux/pageblock-flags.h b/include/linux/pageblock-flags.h
>> index 0c4963339f0b..9fadae5892b2 100644
>> --- a/include/linux/pageblock-flags.h
>> +++ b/include/linux/pageblock-flags.h
>> @@ -20,7 +20,10 @@ enum pageblock_bits {
>>  	PB_migrate_end = PB_migrate + PB_migratetype_bits - 1,
>>  			/* 3 bits required for migrate types */
>>  	PB_migrate_skip,/* If set the block is skipped by compaction */
>> -
>> +#ifdef CONFIG_MEMORY_ISOLATION
>> +	PB_migrate_isolate = 7, /* If set the block is isolated */
>> +			/* set it to 7 to make pageblock bit word aligned */
>> +#endif
>>  	/*
>>  	 * Assume the bits will always align on a word. If this assumption
>>  	 * changes then get/set pageblock needs updating.
>> @@ -28,6 +31,10 @@ enum pageblock_bits {
>>  	NR_PAGEBLOCK_BITS
>>  };
>>
>> +#ifdef CONFIG_MEMORY_ISOLATION
>> +#define PB_migrate_isolate_bit BIT(PB_migrate_isolate)
>> +#endif
>> +
>>  #if defined(CONFIG_PAGE_BLOCK_ORDER)
>>  #define PAGE_BLOCK_ORDER CONFIG_PAGE_BLOCK_ORDER
>>  #else
>> @@ -105,4 +112,28 @@ static inline void set_pageblock_skip(struct page *page)
>>  }
>>  #endif /* CONFIG_COMPACTION */
>>
>> +#ifdef CONFIG_MEMORY_ISOLATION
>> +#define get_pageblock_isolate(page) \
>> +	get_pfnblock_flags_mask(page, page_to_pfn(page),	\
>> +			PB_migrate_isolate_bit)
>> +#define clear_pageblock_isolate(page) \
>> +	set_pfnblock_flags_mask(page, 0, page_to_pfn(page),	\
>> +			PB_migrate_isolate_bit)
>> +#define set_pageblock_isolate(page) \
>> +	set_pfnblock_flags_mask(page, PB_migrate_isolate_bit,	\
>> +			page_to_pfn(page),			\
>> +			PB_migrate_isolate_bit)
>
> Would it make sense to move these to page_isolation.c? Then they
> wouldn't have to be macros.

Sure. Although I am using get_pageblock_isolate() in mm/page_alloc.c,
I can change it to use get_pageblock_migratetype() == MIGRATE_ISOLATE.

>
>> +#else
>> +static inline bool get_pageblock_isolate(struct page *page)
>> +{
>> +	return false;
>> +}
>> +static inline void clear_pageblock_isolate(struct page *page)
>> +{
>> +}
>> +static inline void set_pageblock_isolate(struct page *page)
>> +{
>> +}
>> +#endif /* CONFIG_MEMORY_ISOLATION */
>> +
>>  #endif	/* PAGEBLOCK_FLAGS_H */
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index c77592b22256..acf68ef041d8 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -381,12 +381,40 @@ unsigned long get_pfnblock_flags_mask(const struct page *page,
>>  	return (word >> bitidx) & mask;
>>  }
>>
>> +#ifdef CONFIG_MEMORY_ISOLATION
>> +unsigned long get_pageblock_migratetype(const struct page *page)
>> +{
>> +	unsigned long flags;
>> +
>> +	flags = get_pfnblock_flags_mask(page, page_to_pfn(page),
>> +			MIGRATETYPE_MASK);
>> +	if (flags & PB_migrate_isolate_bit)
>> +		return MIGRATE_ISOLATE;
>> +
>> +	return flags;
>> +}
>
> Since MIGRATETYPE_MASK includes the isolate bit if it exists, I think
> you can share the get_pfnblock_flags_mask() part:
>
> static inline get_pageblock_migratetype(const struct page *page)
> {
> 	unsigned long pfn = page_to_pfn(page);
>
> 	flags = get_pfnblock_flags_mask(page, pfn, MIGRATETYPE_MASK);
> #ifdef CONFIG_MEMORY_ISOLATEION
> 	if (flags & PB_migrate_isolate_bit)
> 		return MIGRATE_ISOLATE;
> #endif
> 	return flags;
> }
>
>> +static __always_inline int get_pfnblock_migratetype(const struct page *page,
>> +					unsigned long pfn)
>> +{
>> +	unsigned long flags;
>> +
>> +	flags = get_pfnblock_flags_mask(page, pfn,
>> +			MIGRATETYPE_MASK);
>> +	if (flags & PB_migrate_isolate_bit)
>> +		return MIGRATE_ISOLATE;
>> +
>> +	return flags;
>> +}
>> +#else
>>  static __always_inline int get_pfnblock_migratetype(const struct page *page,
>>  					unsigned long pfn)
>>  {
>>  	return get_pfnblock_flags_mask(page, pfn, MIGRATETYPE_MASK);
>>  }
>
> Same with this one.
>

OK, will change these two.

>>
>> +#endif
>> +
>>  /**
>>   * set_pfnblock_flags_mask - Set the requested group of flags for a pageblock_nr_pages block of pages
>>   * @page: The page within the block of interest
>> @@ -402,8 +430,14 @@ void set_pfnblock_flags_mask(struct page *page, unsigned long flags,
>>  	unsigned long bitidx, word_bitidx;
>>  	unsigned long word;
>>
>> +#ifdef CONFIG_MEMORY_ISOLATION
>> +	BUILD_BUG_ON(NR_PAGEBLOCK_BITS != 8);
>> +	/* extra one for MIGRATE_ISOLATE */
>> +	BUILD_BUG_ON(MIGRATE_TYPES > (1 << PB_migratetype_bits) + 1);
>> +#else
>>  	BUILD_BUG_ON(NR_PAGEBLOCK_BITS != 4);
>>  	BUILD_BUG_ON(MIGRATE_TYPES > (1 << PB_migratetype_bits));
>> +#endif
>>
>>  	bitmap = get_pageblock_bitmap(page, pfn);
>>  	bitidx = pfn_to_bitidx(page, pfn);
>> @@ -426,7 +460,12 @@ void set_pageblock_migratetype(struct page *page, int migratetype)
>>  		     migratetype < MIGRATE_PCPTYPES))
>>  		migratetype = MIGRATE_UNMOVABLE;
>>
>> -	set_pfnblock_flags_mask(page, (unsigned long)migratetype,
>> +#ifdef CONFIG_MEMORY_ISOLATION
>> +	if (migratetype == MIGRATE_ISOLATE)
>> +		set_pageblock_isolate(page);
>
> Are there paths actually doing this after the second patch?
>
> There are many instances that want to *read* the migratetype or
> MIGRATE_ISOLATE, but only isolation code should be manipulating that
> bit through the dedicated set/toggle_pageblock_isolate API.
>
> If there isn't one, it might be good to enforce this with a VM_WARN
> instead.

I checked all set_pageblock_migratetype() callers and do not see
one using it for pageblock isolation. Let me replace the code
with a VM_WARN and add a comment to tell users to use dedicated
pageblock isolation APIs.

Thanks for all the reviews. Let me send the fixup patches.

>
>> +	else
>> +#endif
>> +		set_pfnblock_flags_mask(page, (unsigned long)migratetype,
>>  				page_to_pfn(page), MIGRATETYPE_MASK);
>
> If the branch stays, you could add a `return' to the MIGRATE_ISOLATE
> leg, drop the else and indent this line normally.


--
Best Regards,
Yan, Zi

>>> @@ -426,7 +460,12 @@ void set_pageblock_migratetype(struct page *page, int migratetype)
>>>  		     migratetype < MIGRATE_PCPTYPES))
>>>  		migratetype = MIGRATE_UNMOVABLE;
>>>
>>> -	set_pfnblock_flags_mask(page, (unsigned long)migratetype,
>>> +#ifdef CONFIG_MEMORY_ISOLATION
>>> +	if (migratetype == MIGRATE_ISOLATE)
>>> +		set_pageblock_isolate(page);
>>
>> Are there paths actually doing this after the second patch?
>>
>> There are many instances that want to *read* the migratetype or
>> MIGRATE_ISOLATE, but only isolation code should be manipulating that
>> bit through the dedicated set/toggle_pageblock_isolate API.
>>
>> If there isn't one, it might be good to enforce this with a VM_WARN
>> instead.
>
> I checked all set_pageblock_migratetype() callers and do not see
> one using it for pageblock isolation. Let me replace the code
> with a VM_WARN and add a comment to tell users to use dedicated
> pageblock isolation APIs.
>

Actually, move_freepages_block_isolate() calls __move_freepages_block()
to move free pages to MIGRATE_ISOLATE pageblock and
set_pageblock_migratetype() is used inside __move_freepages_block().
So the branch has to stay. Will use the suggestion below.

>>
>>> +	else
>>> +#endif
>>> +		set_pfnblock_flags_mask(page, (unsigned long)migratetype,
>>>  				page_to_pfn(page), MIGRATETYPE_MASK);
>>
>> If the branch stays, you could add a `return' to the MIGRATE_ISOLATE
>> leg, drop the else and indent this line normally.


Best Regards,
Yan, Zi

On Thu, May 08, 2025 at 03:17:05PM -0400, Zi Yan wrote:
> 
> >>> @@ -426,7 +460,12 @@ void set_pageblock_migratetype(struct page *page, int migratetype)
> >>>  		     migratetype < MIGRATE_PCPTYPES))
> >>>  		migratetype = MIGRATE_UNMOVABLE;
> >>>
> >>> -	set_pfnblock_flags_mask(page, (unsigned long)migratetype,
> >>> +#ifdef CONFIG_MEMORY_ISOLATION
> >>> +	if (migratetype == MIGRATE_ISOLATE)
> >>> +		set_pageblock_isolate(page);
> >>
> >> Are there paths actually doing this after the second patch?
> >>
> >> There are many instances that want to *read* the migratetype or
> >> MIGRATE_ISOLATE, but only isolation code should be manipulating that
> >> bit through the dedicated set/toggle_pageblock_isolate API.
> >>
> >> If there isn't one, it might be good to enforce this with a VM_WARN
> >> instead.
> >
> > I checked all set_pageblock_migratetype() callers and do not see
> > one using it for pageblock isolation. Let me replace the code
> > with a VM_WARN and add a comment to tell users to use dedicated
> > pageblock isolation APIs.
> >
> 
> Actually, move_freepages_block_isolate() calls __move_freepages_block()
> to move free pages to MIGRATE_ISOLATE pageblock and
> set_pageblock_migratetype() is used inside __move_freepages_block().
> So the branch has to stay. Will use the suggestion below.

Ah, good catch. But looking at the callers, it's:

move_freepages_block()
move_freepages_block_isolate()
try_to_claim_block()

The last one would benefit from having the set_pageblock_migratetype()
there explicitly, as this is what this function is supposed to do. It
also should never set the isolation bit.

move_freepages_block_isolate() has two set_pageblock_migratetype()
calls already. And after the series, it should only manipulate the
isolate bit, not change the actual migratetype anymore, right?

Maybe it makes the most sense to move it into the three callers?

And then fortify set_pageblock_migratetype() after all.

On 8 May 2025, at 16:46, Johannes Weiner wrote:

> On Thu, May 08, 2025 at 03:17:05PM -0400, Zi Yan wrote:
>>
>>>>> @@ -426,7 +460,12 @@ void set_pageblock_migratetype(struct page *page, int migratetype)
>>>>>  		     migratetype < MIGRATE_PCPTYPES))
>>>>>  		migratetype = MIGRATE_UNMOVABLE;
>>>>>
>>>>> -	set_pfnblock_flags_mask(page, (unsigned long)migratetype,
>>>>> +#ifdef CONFIG_MEMORY_ISOLATION
>>>>> +	if (migratetype == MIGRATE_ISOLATE)
>>>>> +		set_pageblock_isolate(page);
>>>>
>>>> Are there paths actually doing this after the second patch?
>>>>
>>>> There are many instances that want to *read* the migratetype or
>>>> MIGRATE_ISOLATE, but only isolation code should be manipulating that
>>>> bit through the dedicated set/toggle_pageblock_isolate API.
>>>>
>>>> If there isn't one, it might be good to enforce this with a VM_WARN
>>>> instead.
>>>
>>> I checked all set_pageblock_migratetype() callers and do not see
>>> one using it for pageblock isolation. Let me replace the code
>>> with a VM_WARN and add a comment to tell users to use dedicated
>>> pageblock isolation APIs.
>>>
>>
>> Actually, move_freepages_block_isolate() calls __move_freepages_block()
>> to move free pages to MIGRATE_ISOLATE pageblock and
>> set_pageblock_migratetype() is used inside __move_freepages_block().
>> So the branch has to stay. Will use the suggestion below.
>
> Ah, good catch. But looking at the callers, it's:
>
> move_freepages_block()
> move_freepages_block_isolate()
> try_to_claim_block()
>
> The last one would benefit from having the set_pageblock_migratetype()
> there explicitly, as this is what this function is supposed to do. It
> also should never set the isolation bit.
>
> move_freepages_block_isolate() has two set_pageblock_migratetype()
> calls already. And after the series, it should only manipulate the
> isolate bit, not change the actual migratetype anymore, right?
>
> Maybe it makes the most sense to move it into the three callers?
>
> And then fortify set_pageblock_migratetype() after all.

Sounds good to me. Let me update my fixups.

Hi Andrew,

I will resend new fixups based on Johannes' comment.

--
Best Regards,
Yan, Zi

On 8 May 2025, at 16:53, Zi Yan wrote:

> On 8 May 2025, at 16:46, Johannes Weiner wrote:
>
>> On Thu, May 08, 2025 at 03:17:05PM -0400, Zi Yan wrote:
>>>
>>>>>> @@ -426,7 +460,12 @@ void set_pageblock_migratetype(struct page *page, int migratetype)
>>>>>>  		     migratetype < MIGRATE_PCPTYPES))
>>>>>>  		migratetype = MIGRATE_UNMOVABLE;
>>>>>>
>>>>>> -	set_pfnblock_flags_mask(page, (unsigned long)migratetype,
>>>>>> +#ifdef CONFIG_MEMORY_ISOLATION
>>>>>> +	if (migratetype == MIGRATE_ISOLATE)
>>>>>> +		set_pageblock_isolate(page);
>>>>>
>>>>> Are there paths actually doing this after the second patch?
>>>>>
>>>>> There are many instances that want to *read* the migratetype or
>>>>> MIGRATE_ISOLATE, but only isolation code should be manipulating that
>>>>> bit through the dedicated set/toggle_pageblock_isolate API.
>>>>>
>>>>> If there isn't one, it might be good to enforce this with a VM_WARN
>>>>> instead.
>>>>
>>>> I checked all set_pageblock_migratetype() callers and do not see
>>>> one using it for pageblock isolation. Let me replace the code
>>>> with a VM_WARN and add a comment to tell users to use dedicated
>>>> pageblock isolation APIs.
>>>>
>>>
>>> Actually, move_freepages_block_isolate() calls __move_freepages_block()
>>> to move free pages to MIGRATE_ISOLATE pageblock and
>>> set_pageblock_migratetype() is used inside __move_freepages_block().
>>> So the branch has to stay. Will use the suggestion below.
>>
>> Ah, good catch. But looking at the callers, it's:
>>
>> move_freepages_block()
>> move_freepages_block_isolate()
>> try_to_claim_block()
>>
>> The last one would benefit from having the set_pageblock_migratetype()
>> there explicitly, as this is what this function is supposed to do. It
>> also should never set the isolation bit.
>>
>> move_freepages_block_isolate() has two set_pageblock_migratetype()
>> calls already. And after the series, it should only manipulate the
>> isolate bit, not change the actual migratetype anymore, right?
>>
>> Maybe it makes the most sense to move it into the three callers?
>>
>> And then fortify set_pageblock_migratetype() after all.
>
> Sounds good to me. Let me update my fixups.

Hmm, hit another roadblock. In online_pages() from mm/memory_hotplug.c,
move_pfn_range_to_zone(MIGRATE_ISOLATE) calls memmap_init_range(),
which uses set_pageblock_migratetype(MIGRATE_ISOLATE).

I could use set_pageblock_isolate() in memmap_init_range(), but
that requires move set_pageblock_isolate() out of mm/page_isolation.c.
The change might be too substantial for a fixup.

I also would like to get some opinion from David on this. So I am
holding this fixup and will send it out as a separate patch when
I get more information.


--
Best Regards,
Yan, Zi

On 8 May 2025, at 21:33, Zi Yan wrote:

> On 8 May 2025, at 16:53, Zi Yan wrote:
>
>> On 8 May 2025, at 16:46, Johannes Weiner wrote:
>>
>>> On Thu, May 08, 2025 at 03:17:05PM -0400, Zi Yan wrote:
>>>>
>>>>>>> @@ -426,7 +460,12 @@ void set_pageblock_migratetype(struct page *page, int migratetype)
>>>>>>>  		     migratetype < MIGRATE_PCPTYPES))
>>>>>>>  		migratetype = MIGRATE_UNMOVABLE;
>>>>>>>
>>>>>>> -	set_pfnblock_flags_mask(page, (unsigned long)migratetype,
>>>>>>> +#ifdef CONFIG_MEMORY_ISOLATION
>>>>>>> +	if (migratetype == MIGRATE_ISOLATE)
>>>>>>> +		set_pageblock_isolate(page);
>>>>>>
>>>>>> Are there paths actually doing this after the second patch?
>>>>>>
>>>>>> There are many instances that want to *read* the migratetype or
>>>>>> MIGRATE_ISOLATE, but only isolation code should be manipulating that
>>>>>> bit through the dedicated set/toggle_pageblock_isolate API.
>>>>>>
>>>>>> If there isn't one, it might be good to enforce this with a VM_WARN
>>>>>> instead.
>>>>>
>>>>> I checked all set_pageblock_migratetype() callers and do not see
>>>>> one using it for pageblock isolation. Let me replace the code
>>>>> with a VM_WARN and add a comment to tell users to use dedicated
>>>>> pageblock isolation APIs.
>>>>>
>>>>
>>>> Actually, move_freepages_block_isolate() calls __move_freepages_block()
>>>> to move free pages to MIGRATE_ISOLATE pageblock and
>>>> set_pageblock_migratetype() is used inside __move_freepages_block().
>>>> So the branch has to stay. Will use the suggestion below.
>>>
>>> Ah, good catch. But looking at the callers, it's:
>>>
>>> move_freepages_block()
>>> move_freepages_block_isolate()
>>> try_to_claim_block()
>>>
>>> The last one would benefit from having the set_pageblock_migratetype()
>>> there explicitly, as this is what this function is supposed to do. It
>>> also should never set the isolation bit.
>>>
>>> move_freepages_block_isolate() has two set_pageblock_migratetype()
>>> calls already. And after the series, it should only manipulate the
>>> isolate bit, not change the actual migratetype anymore, right?
>>>
>>> Maybe it makes the most sense to move it into the three callers?
>>>
>>> And then fortify set_pageblock_migratetype() after all.
>>
>> Sounds good to me. Let me update my fixups.
>
> Hmm, hit another roadblock. In online_pages() from mm/memory_hotplug.c,
> move_pfn_range_to_zone(MIGRATE_ISOLATE) calls memmap_init_range(),
> which uses set_pageblock_migratetype(MIGRATE_ISOLATE).
>
> I could use set_pageblock_isolate() in memmap_init_range(), but
> that requires move set_pageblock_isolate() out of mm/page_isolation.c.
> The change might be too substantial for a fixup.
>
> I also would like to get some opinion from David on this. So I am
> holding this fixup and will send it out as a separate patch when
> I get more information.

Hi David,

I have some concern regarding online_pages() calling
move_pfn_range_to_zone(MIGRATE_ISOLATE) code path after my patchset.
During the process, all pageblocks are initialized to MIGRATE_ISOLATE,
then undo_isolate_page_range() is called to unisolate all pageblocks.
That means these pageblocks do not have proper migratetype, since
they are not set to any type other than MIGRATE_ISOLATE.

Maybe the code should be changed to

move_pfn_range_to_zone(zone, pfn, nr_pages, NULL, MIGRATE_MOVABLE);
start_isolate_page_range(pfn, pfn + nr_pages, ISOLATE_MODE_NONE, 0);
...
undo_isolate_page_range(pfn, pfn + nr_pages);

so that these pageblocks have MIGRATE_MOVABLE type.

--
Best Regards,
Yan, Zi