[PATCH 0/2] mm/compaction: some fix for the range passed to pageblock_pfn_to_page()

[PATCH 0/2] mm/compaction: some fix for the range passed to pageblock_pfn_to_page()

[Wei Yang]

During the code reading of isolate_migratepages_range(), first spot the range
passed to pageblock_pfn_to_page() is different from that to
isolate_migratepages_block().

This implies there is a chance that pageblock_pfn_to_page() thinks the range
is in the same zone, but isolate_migratepages_block() will isolate range in
two different zones. This is not what we expect.

Then I found pageblock_pfn_to_page() has an optimization if zone->contiguous,
this means even the range is across two different zones, it will think the
range is within the same zone.

So introduce two patches to fix it:

Patch 1: check the range belongs to the zone first
Patch 2: pass the correct range to pageblock_pfn_to_page()

Wei Yang (2):
  mm/compaction: check the range to pageblock_pfn_to_page() is within
    the zone first
  mm/compaction: fix the range to pageblock_pfn_to_page()

 mm/compaction.c | 37 ++++++++++++++-----------------------
 mm/internal.h   |  3 +++
 2 files changed, 17 insertions(+), 23 deletions(-)

-- 
2.34.1

[PATCH 1/2] mm/compaction: check the range to pageblock_pfn_to_page() is within the zone first

[Wei Yang]

Function pageblock_pfn_to_page() is introduced by commit 7d49d8868336
("mm, compaction: reduce zone checking frequency in the migration scanner"),
where there is no requirement on start_pfn/end_pfn except they are in
the same pageblock.

So at that time, pageblock_pfn_to_page() would be passed with pfn
without compared with zone boundary.

But after commit 7cf91a98e607 ("mm/compaction: speed up
pageblock_pfn_to_page() when zone is contiguous"), pageblock_pfn_to_page()
would think the range is valid and in the same zone if zone->contiguous,
even the range doesn't belong to this zone.

For example, in fast_isolate_freepages(), min_pfn is assigned to
pageblock_start_pfn() and passed to pageblock_pfn_to_page() without
checking with zone_start_pfn. And mostly, the end_pfn is not checked
with zone_end_pfn() before using.

To make this function robust, check the range is within the zone first.

Fixes: 7cf91a98e607 ("mm/compaction: speed up pageblock_pfn_to_page() when zone is contiguous")
Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
---
 mm/internal.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/mm/internal.h b/mm/internal.h
index 38607b2821d9..8e1a3819c9f1 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -724,6 +724,9 @@ extern struct page *__pageblock_pfn_to_page(unsigned long start_pfn,
 static inline struct page *pageblock_pfn_to_page(unsigned long start_pfn,
 				unsigned long end_pfn, struct zone *zone)
 {
+	if (start_pfn < zone->zone_start_pfn || end_pfn > zone_end_pfn(zone))
+		return NULL;
+
 	if (zone->contiguous)
 		return pfn_to_page(start_pfn);
 
-- 
2.34.1

[PATCH 2/2] mm/compaction: fix the range to pageblock_pfn_to_page()

[Wei Yang]

Current code may skip some part of the range to
pageblock_pfn_to_page() to check whether the range is in the same zone.

Function pageblock_pfn_to_page() is first introduced by commit 7d49d8868336
("mm, compaction: reduce zone checking frequency in the migration scanner"),
in which it checks and isolates on the same range [pfn, block_end_pfn].

While after commit e1409c325fdc ("mm/compaction: pass only pageblock
aligned range to pageblock_pfn_to_page"), we operate on two different
ranges for check and isolation.

  * [block_start_pfn, block_end_pfn] to check it is in the same zone
  * [pfn, block_end_pfn] to do isolation

It miss some range to check when start_pfn and zone->zone_start_pfn is
in the same pageblock but (start_pfn < zone->zone_start_pfn). The range
before zone_start_pfn is missed.

         start_pfn     zone_start_pfn
    +----+-------------+-------------------+
    block_start_pfn                        block_end_pfn

This leads to the range check is passed, but it will isolate a range in
two different zones.

Let's use the same [block_start_pfn, block_end_pfn] for each iteration
to represent the range to check and isolate.

Fixes: e1409c325fdc ("mm/compaction: pass only pageblock aligned range to pageblock_pfn_to_page")
Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
---
 mm/compaction.c | 37 ++++++++++++++-----------------------
 1 file changed, 14 insertions(+), 23 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 1e8f8eca318c..8760d10bd0b3 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -1320,27 +1320,22 @@ int
 isolate_migratepages_range(struct compact_control *cc, unsigned long start_pfn,
 							unsigned long end_pfn)
 {
-	unsigned long pfn, block_start_pfn, block_end_pfn;
+	unsigned long block_start_pfn, block_end_pfn;
 	int ret = 0;
 
 	/* Scan block by block. First and last block may be incomplete */
-	pfn = start_pfn;
-	block_start_pfn = pageblock_start_pfn(pfn);
-	if (block_start_pfn < cc->zone->zone_start_pfn)
-		block_start_pfn = cc->zone->zone_start_pfn;
-	block_end_pfn = pageblock_end_pfn(pfn);
+	block_start_pfn = start_pfn;
+	block_end_pfn = pageblock_end_pfn(start_pfn);
 
-	for (; pfn < end_pfn; pfn = block_end_pfn,
-				block_start_pfn = block_end_pfn,
+	for (; block_start_pfn < end_pfn; block_start_pfn = block_end_pfn,
 				block_end_pfn += pageblock_nr_pages) {
 
 		block_end_pfn = min(block_end_pfn, end_pfn);
 
-		if (!pageblock_pfn_to_page(block_start_pfn,
-					block_end_pfn, cc->zone))
+		if (!pageblock_pfn_to_page(block_start_pfn, block_end_pfn, cc->zone))
 			continue;
 
-		ret = isolate_migratepages_block(cc, pfn, block_end_pfn,
+		ret = isolate_migratepages_block(cc, block_start_pfn, block_end_pfn,
 						 ISOLATE_UNEVICTABLE);
 
 		if (ret)
@@ -2046,7 +2041,6 @@ static isolate_migrate_t isolate_migratepages(struct compact_control *cc)
 {
 	unsigned long block_start_pfn;
 	unsigned long block_end_pfn;
-	unsigned long low_pfn;
 	struct page *page;
 	const isolate_mode_t isolate_mode =
 		(sysctl_compact_unevictable_allowed ? ISOLATE_UNEVICTABLE : 0) |
@@ -2058,20 +2052,17 @@ static isolate_migrate_t isolate_migratepages(struct compact_control *cc)
 	 * initialized by compact_zone(). The first failure will use
 	 * the lowest PFN as the starting point for linear scanning.
 	 */
-	low_pfn = fast_find_migrateblock(cc);
-	block_start_pfn = pageblock_start_pfn(low_pfn);
-	if (block_start_pfn < cc->zone->zone_start_pfn)
-		block_start_pfn = cc->zone->zone_start_pfn;
+	block_start_pfn = fast_find_migrateblock(cc);
 
 	/*
 	 * fast_find_migrateblock() has already ensured the pageblock is not
 	 * set with a skipped flag, so to avoid the isolation_suitable check
 	 * below again, check whether the fast search was successful.
 	 */
-	fast_find_block = low_pfn != cc->migrate_pfn && !cc->fast_search_fail;
+	fast_find_block = block_start_pfn != cc->migrate_pfn && !cc->fast_search_fail;
 
 	/* Only scan within a pageblock boundary */
-	block_end_pfn = pageblock_end_pfn(low_pfn);
+	block_end_pfn = pageblock_end_pfn(block_start_pfn);
 
 	/*
 	 * Iterate over whole pageblocks until we find the first suitable.
@@ -2079,7 +2070,7 @@ static isolate_migrate_t isolate_migratepages(struct compact_control *cc)
 	 */
 	for (; block_end_pfn <= cc->free_pfn;
 			fast_find_block = false,
-			cc->migrate_pfn = low_pfn = block_end_pfn,
+			cc->migrate_pfn = block_end_pfn,
 			block_start_pfn = block_end_pfn,
 			block_end_pfn += pageblock_nr_pages) {
 
@@ -2088,7 +2079,7 @@ static isolate_migrate_t isolate_migratepages(struct compact_control *cc)
 		 * many pageblocks unsuitable, so periodically check if we
 		 * need to schedule.
 		 */
-		if (!(low_pfn % (COMPACT_CLUSTER_MAX * pageblock_nr_pages)))
+		if (!(block_start_pfn % (COMPACT_CLUSTER_MAX * pageblock_nr_pages)))
 			cond_resched();
 
 		page = pageblock_pfn_to_page(block_start_pfn,
@@ -2109,8 +2100,8 @@ static isolate_migrate_t isolate_migratepages(struct compact_control *cc)
 		 * before making it "skip" so other compaction instances do
 		 * not scan the same block.
 		 */
-		if ((pageblock_aligned(low_pfn) ||
-		     low_pfn == cc->zone->zone_start_pfn) &&
+		if ((pageblock_aligned(block_start_pfn) ||
+		     block_start_pfn == cc->zone->zone_start_pfn) &&
 		    !fast_find_block && !isolation_suitable(cc, page))
 			continue;
 
@@ -2128,7 +2119,7 @@ static isolate_migrate_t isolate_migratepages(struct compact_control *cc)
 		}
 
 		/* Perform the isolation */
-		if (isolate_migratepages_block(cc, low_pfn, block_end_pfn,
+		if (isolate_migratepages_block(cc, block_start_pfn, block_end_pfn,
 						isolate_mode))
 			return ISOLATE_ABORT;
 
-- 
2.34.1

Re: [PATCH 0/2] mm/compaction: some fix for the range passed to pageblock_pfn_to_page()

[Andrew Morton]

On Thu,  2 Oct 2025 03:31:38 +0000 Wei Yang <richard.weiyang@gmail.com> wrote:

> During the code reading of isolate_migratepages_range(), first spot the range
> passed to pageblock_pfn_to_page() is different from that to
> isolate_migratepages_block().
> 
> This implies there is a chance that pageblock_pfn_to_page() thinks the range
> is in the same zone, but isolate_migratepages_block() will isolate range in
> two different zones. This is not what we expect.
> 
> Then I found pageblock_pfn_to_page() has an optimization if zone->contiguous,
> this means even the range is across two different zones, it will think the
> range is within the same zone.
> 
> So introduce two patches to fix it:

What do you think might be the worst-case userspace-visible effects
of the bug?

Thanks.

Re: [PATCH 0/2] mm/compaction: some fix for the range passed to pageblock_pfn_to_page()

[Wei Yang]

On Tue, Oct 07, 2025 at 06:29:22PM -0700, Andrew Morton wrote:
>On Thu,  2 Oct 2025 03:31:38 +0000 Wei Yang <richard.weiyang@gmail.com> wrote:
>
>> During the code reading of isolate_migratepages_range(), first spot the range
>> passed to pageblock_pfn_to_page() is different from that to
>> isolate_migratepages_block().
>> 
>> This implies there is a chance that pageblock_pfn_to_page() thinks the range
>> is in the same zone, but isolate_migratepages_block() will isolate range in
>> two different zones. This is not what we expect.
>> 
>> Then I found pageblock_pfn_to_page() has an optimization if zone->contiguous,
>> this means even the range is across two different zones, it will think the
>> range is within the same zone.
>> 
>> So introduce two patches to fix it:
>
>What do you think might be the worst-case userspace-visible effects
>of the bug?

I don't see userspace-visible effect yet.

Since I lack the knowledge of the consequence of isolating cross zone range,
currently I can't tell the effect accurately.

The worst case in my mind is we put some page on a different zone's freelist.
But I don't totally understand the behavior now.

>
>Thanks.

-- 
Wei Yang
Help you, Help me

Re: [PATCH 0/2] mm/compaction: some fix for the range passed to pageblock_pfn_to_page()

[Michal Hocko]

On Wed 08-10-25 02:32:18, Wei Yang wrote:
> On Tue, Oct 07, 2025 at 06:29:22PM -0700, Andrew Morton wrote:
> >On Thu,  2 Oct 2025 03:31:38 +0000 Wei Yang <richard.weiyang@gmail.com> wrote:
> >
> >> During the code reading of isolate_migratepages_range(), first spot the range
> >> passed to pageblock_pfn_to_page() is different from that to
> >> isolate_migratepages_block().
> >> 
> >> This implies there is a chance that pageblock_pfn_to_page() thinks the range
> >> is in the same zone, but isolate_migratepages_block() will isolate range in
> >> two different zones. This is not what we expect.
> >> 
> >> Then I found pageblock_pfn_to_page() has an optimization if zone->contiguous,
> >> this means even the range is across two different zones, it will think the
> >> range is within the same zone.
> >> 
> >> So introduce two patches to fix it:
> >
> >What do you think might be the worst-case userspace-visible effects
> >of the bug?
> 
> I don't see userspace-visible effect yet.
> 
> Since I lack the knowledge of the consequence of isolating cross zone range,
> currently I can't tell the effect accurately.
> 
> The worst case in my mind is we put some page on a different zone's freelist.
> But I don't totally understand the behavior now.

Is there any actual problem to fix here then?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 0/2] mm/compaction: some fix for the range passed to pageblock_pfn_to_page()

[Wei Yang]

On Wed, Oct 08, 2025 at 09:52:51AM +0200, Michal Hocko wrote:
>On Wed 08-10-25 02:32:18, Wei Yang wrote:
>> On Tue, Oct 07, 2025 at 06:29:22PM -0700, Andrew Morton wrote:
>> >On Thu,  2 Oct 2025 03:31:38 +0000 Wei Yang <richard.weiyang@gmail.com> wrote:
>> >
>> >> During the code reading of isolate_migratepages_range(), first spot the range
>> >> passed to pageblock_pfn_to_page() is different from that to
>> >> isolate_migratepages_block().
>> >> 
>> >> This implies there is a chance that pageblock_pfn_to_page() thinks the range
>> >> is in the same zone, but isolate_migratepages_block() will isolate range in
>> >> two different zones. This is not what we expect.
>> >> 
>> >> Then I found pageblock_pfn_to_page() has an optimization if zone->contiguous,
>> >> this means even the range is across two different zones, it will think the
>> >> range is within the same zone.
>> >> 
>> >> So introduce two patches to fix it:
>> >
>> >What do you think might be the worst-case userspace-visible effects
>> >of the bug?
>> 
>> I don't see userspace-visible effect yet.
>> 
>> Since I lack the knowledge of the consequence of isolating cross zone range,
>> currently I can't tell the effect accurately.
>> 
>> The worst case in my mind is we put some page on a different zone's freelist.
>> But I don't totally understand the behavior now.
>
>Is there any actual problem to fix here then?

I don't see an actual problem in practice yet.

>-- 
>Michal Hocko
>SUSE Labs

-- 
Wei Yang
Help you, Help me

Re: [PATCH 0/2] mm/compaction: some fix for the range passed to pageblock_pfn_to_page()

[David Hildenbrand]

On 08.10.25 11:13, Wei Yang wrote:
> On Wed, Oct 08, 2025 at 09:52:51AM +0200, Michal Hocko wrote:
>> On Wed 08-10-25 02:32:18, Wei Yang wrote:
>>> On Tue, Oct 07, 2025 at 06:29:22PM -0700, Andrew Morton wrote:
>>>> On Thu,  2 Oct 2025 03:31:38 +0000 Wei Yang <richard.weiyang@gmail.com> wrote:
>>>>
>>>>> During the code reading of isolate_migratepages_range(), first spot the range
>>>>> passed to pageblock_pfn_to_page() is different from that to
>>>>> isolate_migratepages_block().
>>>>>
>>>>> This implies there is a chance that pageblock_pfn_to_page() thinks the range
>>>>> is in the same zone, but isolate_migratepages_block() will isolate range in
>>>>> two different zones. This is not what we expect.
>>>>>
>>>>> Then I found pageblock_pfn_to_page() has an optimization if zone->contiguous,
>>>>> this means even the range is across two different zones, it will think the
>>>>> range is within the same zone.
>>>>>
>>>>> So introduce two patches to fix it:
>>>>
>>>> What do you think might be the worst-case userspace-visible effects
>>>> of the bug?
>>>
>>> I don't see userspace-visible effect yet.
>>>
>>> Since I lack the knowledge of the consequence of isolating cross zone range,
>>> currently I can't tell the effect accurately.
>>>
>>> The worst case in my mind is we put some page on a different zone's freelist.
>>> But I don't totally understand the behavior now.
>>
>> Is there any actual problem to fix here then?
> 
> I don't see an actual problem in practice yet.

We should avoid talking about "fix" then. Let me take a look at the patches.

-- 
Cheers

David / dhildenb

Re: [PATCH 1/2] mm/compaction: check the range to pageblock_pfn_to_page() is within the zone first

[David Hildenbrand]

On 02.10.25 05:31, Wei Yang wrote:
> Function pageblock_pfn_to_page() is introduced by commit 7d49d8868336
> ("mm, compaction: reduce zone checking frequency in the migration scanner"),
> where there is no requirement on start_pfn/end_pfn except they are in
> the same pageblock.
> 
> So at that time, pageblock_pfn_to_page() would be passed with pfn
> without compared with zone boundary.
> 
> But after commit 7cf91a98e607 ("mm/compaction: speed up
> pageblock_pfn_to_page() when zone is contiguous"), pageblock_pfn_to_page()
> would think the range is valid and in the same zone if zone->contiguous,
> even the range doesn't belong to this zone.
> 
> For example, in fast_isolate_freepages(), min_pfn is assigned to
> pageblock_start_pfn() and passed to pageblock_pfn_to_page() without
> checking with zone_start_pfn. And mostly, the end_pfn is not checked
> with zone_end_pfn() before using.
> 
> To make this function robust, check the range is within the zone first.
> 
> Fixes: 7cf91a98e607 ("mm/compaction: speed up pageblock_pfn_to_page() when zone is contiguous")
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> ---
>   mm/internal.h | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/mm/internal.h b/mm/internal.h
> index 38607b2821d9..8e1a3819c9f1 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -724,6 +724,9 @@ extern struct page *__pageblock_pfn_to_page(unsigned long start_pfn,
>   static inline struct page *pageblock_pfn_to_page(unsigned long start_pfn,
>   				unsigned long end_pfn, struct zone *zone)
>   {
> +	if (start_pfn < zone->zone_start_pfn || end_pfn > zone_end_pfn(zone))
> +		return NULL;
> +
>   	if (zone->contiguous)
>   		return pfn_to_page(start_pfn);
>   

fast_isolate_around() adjusts the range to the zone boundaries before 
calling fast_isolate_around().

isolate_migratepages_range(), isolate_freepages_range(), 
isolate_freepages() and isolate_migratepages similarly all seem to take 
the zone range into account as well.

Only fast_isolate_freepages is a bit more tricky. It definitely takes 
the end into account. The start comes from cc->free_pfn but IIRC it's 
always above the zone_start_pfn.

So I wonder -- if we want to add such a check --  whether it should 
instead be a VM_WARN_ON_ONCE()?

-- 
Cheers

David / dhildenb

Re: [PATCH 2/2] mm/compaction: fix the range to pageblock_pfn_to_page()

[David Hildenbrand]

On 02.10.25 05:31, Wei Yang wrote:
> Current code may skip some part of the range to
> pageblock_pfn_to_page() to check whether the range is in the same zone.
> 
> Function pageblock_pfn_to_page() is first introduced by commit 7d49d8868336
> ("mm, compaction: reduce zone checking frequency in the migration scanner"),
> in which it checks and isolates on the same range [pfn, block_end_pfn].
> 
> While after commit e1409c325fdc ("mm/compaction: pass only pageblock
> aligned range to pageblock_pfn_to_page"), we operate on two different
> ranges for check and isolation.
> 
>    * [block_start_pfn, block_end_pfn] to check it is in the same zone
>    * [pfn, block_end_pfn] to do isolation
> 
> It miss some range to check when start_pfn and zone->zone_start_pfn is
> in the same pageblock but (start_pfn < zone->zone_start_pfn). The range
> before zone_start_pfn is missed.
> 
>           start_pfn     zone_start_pfn
>      +----+-------------+-------------------+
>      block_start_pfn                        block_end_pfn
> 
> This leads to the range check is passed, but it will isolate a range in
> two different zones.

Can that actually happen? I recall that a zone always spans full pageblocks.

-- 
Cheers

David / dhildenb

Re: [PATCH 2/2] mm/compaction: fix the range to pageblock_pfn_to_page()

[Michal Hocko]

On Wed 08-10-25 12:17:29, David Hildenbrand wrote:
> On 02.10.25 05:31, Wei Yang wrote:
> > Current code may skip some part of the range to
> > pageblock_pfn_to_page() to check whether the range is in the same zone.
> > 
> > Function pageblock_pfn_to_page() is first introduced by commit 7d49d8868336
> > ("mm, compaction: reduce zone checking frequency in the migration scanner"),
> > in which it checks and isolates on the same range [pfn, block_end_pfn].
> > 
> > While after commit e1409c325fdc ("mm/compaction: pass only pageblock
> > aligned range to pageblock_pfn_to_page"), we operate on two different
> > ranges for check and isolation.
> > 
> >    * [block_start_pfn, block_end_pfn] to check it is in the same zone
> >    * [pfn, block_end_pfn] to do isolation
> > 
> > It miss some range to check when start_pfn and zone->zone_start_pfn is
> > in the same pageblock but (start_pfn < zone->zone_start_pfn). The range
> > before zone_start_pfn is missed.
> > 
> >           start_pfn     zone_start_pfn
> >      +----+-------------+-------------------+
> >      block_start_pfn                        block_end_pfn
> > 
> > This leads to the range check is passed, but it will isolate a range in
> > two different zones.
> 
> Can that actually happen? I recall that a zone always spans full pageblocks.

My recollection as well and we should rather focus on enforcing that to
be true than complicating already quite a complicated code.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/2] mm/compaction: check the range to pageblock_pfn_to_page() is within the zone first

[Wei Yang]

On Wed, Oct 08, 2025 at 12:14:25PM +0200, David Hildenbrand wrote:
>On 02.10.25 05:31, Wei Yang wrote:
>> Function pageblock_pfn_to_page() is introduced by commit 7d49d8868336
>> ("mm, compaction: reduce zone checking frequency in the migration scanner"),
>> where there is no requirement on start_pfn/end_pfn except they are in
>> the same pageblock.
>> 
>> So at that time, pageblock_pfn_to_page() would be passed with pfn
>> without compared with zone boundary.
>> 
>> But after commit 7cf91a98e607 ("mm/compaction: speed up
>> pageblock_pfn_to_page() when zone is contiguous"), pageblock_pfn_to_page()
>> would think the range is valid and in the same zone if zone->contiguous,
>> even the range doesn't belong to this zone.
>> 
>> For example, in fast_isolate_freepages(), min_pfn is assigned to
>> pageblock_start_pfn() and passed to pageblock_pfn_to_page() without
>> checking with zone_start_pfn. And mostly, the end_pfn is not checked
>> with zone_end_pfn() before using.
>> 
>> To make this function robust, check the range is within the zone first.
>> 
>> Fixes: 7cf91a98e607 ("mm/compaction: speed up pageblock_pfn_to_page() when zone is contiguous")
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> Cc: Vlastimil Babka <vbabka@suse.cz>
>> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
>> ---
>>   mm/internal.h | 3 +++
>>   1 file changed, 3 insertions(+)
>> 
>> diff --git a/mm/internal.h b/mm/internal.h
>> index 38607b2821d9..8e1a3819c9f1 100644
>> --- a/mm/internal.h
>> +++ b/mm/internal.h
>> @@ -724,6 +724,9 @@ extern struct page *__pageblock_pfn_to_page(unsigned long start_pfn,
>>   static inline struct page *pageblock_pfn_to_page(unsigned long start_pfn,
>>   				unsigned long end_pfn, struct zone *zone)
>>   {
>> +	if (start_pfn < zone->zone_start_pfn || end_pfn > zone_end_pfn(zone))
>> +		return NULL;
>> +
>>   	if (zone->contiguous)
>>   		return pfn_to_page(start_pfn);
>
>fast_isolate_around() adjusts the range to the zone boundaries before calling
>fast_isolate_around().
>

Correct.

>isolate_migratepages_range(), isolate_freepages_range(), isolate_freepages()
>and isolate_migratepages similarly all seem to take the zone range into
>account as well.

Maybe I missed something.

isolate_freepages() takes care of the end_pfn, but I don't see it adjust
start_pfn. Or you mean low_pfn is already adjusted?

isolate_freepages_range() / isolate_migratepages_range() just adjust start_pfn.

isolate_migratepages() seems not adjust start_pfn nor end_pfn?

Maybe I misunderstand you, would you mind giving more detail?

>
>Only fast_isolate_freepages is a bit more tricky. It definitely takes the end
>into account. The start comes from cc->free_pfn but IIRC it's always above
>the zone_start_pfn.
>

You mean we rely on the caller to make sure the range is in within zone?

>So I wonder -- if we want to add such a check --  whether it should instead
>be a VM_WARN_ON_ONCE()?
>
>-- 
>Cheers
>
>David / dhildenb

-- 
Wei Yang
Help you, Help me

Re: [PATCH 2/2] mm/compaction: fix the range to pageblock_pfn_to_page()

[Wei Yang]

On Wed, Oct 08, 2025 at 12:17:29PM +0200, David Hildenbrand wrote:
>On 02.10.25 05:31, Wei Yang wrote:
>> Current code may skip some part of the range to
>> pageblock_pfn_to_page() to check whether the range is in the same zone.
>> 
>> Function pageblock_pfn_to_page() is first introduced by commit 7d49d8868336
>> ("mm, compaction: reduce zone checking frequency in the migration scanner"),
>> in which it checks and isolates on the same range [pfn, block_end_pfn].
>> 
>> While after commit e1409c325fdc ("mm/compaction: pass only pageblock
>> aligned range to pageblock_pfn_to_page"), we operate on two different
>> ranges for check and isolation.
>> 
>>    * [block_start_pfn, block_end_pfn] to check it is in the same zone
>>    * [pfn, block_end_pfn] to do isolation
>> 
>> It miss some range to check when start_pfn and zone->zone_start_pfn is
>> in the same pageblock but (start_pfn < zone->zone_start_pfn). The range
>> before zone_start_pfn is missed.
>> 
>>           start_pfn     zone_start_pfn
>>      +----+-------------+-------------------+
>>      block_start_pfn                        block_end_pfn
>> 
>> This leads to the range check is passed, but it will isolate a range in
>> two different zones.
>
>Can that actually happen? I recall that a zone always spans full pageblocks.
>

I have asked myself the same question. Here is what I found from history.

1. From the very beginning, there is this commit.

   commit dc9086004b3d5db75997a645b3fe08d9138b7ad0
   Author: Mel Gorman <mgorman@suse.de>
   Date:   Wed Feb 8 17:13:38 2012 -0800
   
       mm: compaction: check for overlapping nodes during isolation for migration

This one add a check on (page_zone(page) != zone) in isolate_migratepages(),
for overlapping nodes.

And the range to be checked is within one pageblock.

This sounds there is possibility of two zones in one pageblock.

2. Then there was code refactor.

   commit 2fe86e0004076128f05d5a774b5c9c03d9dc3de2
   Author: Michal Nazarewicz <mina86@mina86.com>
   Date:   Mon Jan 30 13:16:26 2012 +0100
   
       mm: compaction: introduce isolate_migratepages_range()

This introduce a wrapper to isolate_migratepages()

    commit edc2ca61249679298c1f343cd9c549964b8df4b4
    Author: Vlastimil Babka <vbabka@suse.cz>
    Date:   Thu Oct 9 15:27:09 2014 -0700
    
        mm, compaction: move pageblock checks up from isolate_migratepages_range()

This move up the check, but still keep the form (page_zone(page) != zone).

3. Introduce pageblock_pfn_to_page()

    commit 7d49d8868336bbf4f68714d8282ca5fd65e387ed
    Author: Vlastimil Babka <vbabka@suse.cz>
    Date:   Thu Oct 9 15:27:11 2014 -0700
    
        mm, compaction: reduce zone checking frequency in the migration scanner

Instead of do (page_zone(page) != page) check in loop, it does the check
before do real job, e.g. isolation.

PS: it looks a little behavioral change.

4. Current state

    commit e1409c325fdc1fef7b3d8025c51892355f065d15
    Author: Joonsoo Kim <iamjoonsoo.kim@lge.com>
    Date:   Tue Mar 15 14:57:48 2016 -0700
    
        mm/compaction: pass only pageblock aligned range to pageblock_pfn_to_page

    commit 7cf91a98e607c2f935dbcc177d70011e95b8faff
    Author: Joonsoo Kim <iamjoonsoo.kim@lge.com>
    Date:   Tue Mar 15 14:57:51 2016 -0700
    
        mm/compaction: speed up pageblock_pfn_to_page() when zone is contiguous

This did some optimization, which seems break something from commit 7d49d8868336.


All in all, if my understanding is correct. All the change comes down from
commit dc9086004b3d5. This means we do have possibility to have two different
zones in on pageblock?

Or this case vanished now?

-- 
Wei Yang
Help you, Help me

Re: [PATCH 1/2] mm/compaction: check the range to pageblock_pfn_to_page() is within the zone first

[David Hildenbrand]

On 09.10.25 04:08, Wei Yang wrote:
> On Wed, Oct 08, 2025 at 12:14:25PM +0200, David Hildenbrand wrote:
>> On 02.10.25 05:31, Wei Yang wrote:
>>> Function pageblock_pfn_to_page() is introduced by commit 7d49d8868336
>>> ("mm, compaction: reduce zone checking frequency in the migration scanner"),
>>> where there is no requirement on start_pfn/end_pfn except they are in
>>> the same pageblock.
>>>
>>> So at that time, pageblock_pfn_to_page() would be passed with pfn
>>> without compared with zone boundary.
>>>
>>> But after commit 7cf91a98e607 ("mm/compaction: speed up
>>> pageblock_pfn_to_page() when zone is contiguous"), pageblock_pfn_to_page()
>>> would think the range is valid and in the same zone if zone->contiguous,
>>> even the range doesn't belong to this zone.
>>>
>>> For example, in fast_isolate_freepages(), min_pfn is assigned to
>>> pageblock_start_pfn() and passed to pageblock_pfn_to_page() without
>>> checking with zone_start_pfn. And mostly, the end_pfn is not checked
>>> with zone_end_pfn() before using.
>>>
>>> To make this function robust, check the range is within the zone first.
>>>
>>> Fixes: 7cf91a98e607 ("mm/compaction: speed up pageblock_pfn_to_page() when zone is contiguous")
>>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>>> Cc: Vlastimil Babka <vbabka@suse.cz>
>>> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
>>> ---
>>>    mm/internal.h | 3 +++
>>>    1 file changed, 3 insertions(+)
>>>
>>> diff --git a/mm/internal.h b/mm/internal.h
>>> index 38607b2821d9..8e1a3819c9f1 100644
>>> --- a/mm/internal.h
>>> +++ b/mm/internal.h
>>> @@ -724,6 +724,9 @@ extern struct page *__pageblock_pfn_to_page(unsigned long start_pfn,
>>>    static inline struct page *pageblock_pfn_to_page(unsigned long start_pfn,
>>>    				unsigned long end_pfn, struct zone *zone)
>>>    {
>>> +	if (start_pfn < zone->zone_start_pfn || end_pfn > zone_end_pfn(zone))
>>> +		return NULL;
>>> +
>>>    	if (zone->contiguous)
>>>    		return pfn_to_page(start_pfn);
>>
>> fast_isolate_around() adjusts the range to the zone boundaries before calling
>> fast_isolate_around().
>>
> 
> Correct.
> 
>> isolate_migratepages_range(), isolate_freepages_range(), isolate_freepages()
>> and isolate_migratepages similarly all seem to take the zone range into
>> account as well.
> 
> Maybe I missed something.
> 
> isolate_freepages() takes care of the end_pfn, but I don't see it adjust
> start_pfn. Or you mean low_pfn is already adjusted?
> 
> isolate_freepages_range() / isolate_migratepages_range() just adjust start_pfn.
> 
> isolate_migratepages() seems not adjust start_pfn nor end_pfn?
> 
> Maybe I misunderstand you, would you mind giving more detail?
> 
>>
>> Only fast_isolate_freepages is a bit more tricky. It definitely takes the end
>> into account. The start comes from cc->free_pfn but IIRC it's always above
>> the zone_start_pfn.
>>

I don't recall all the details, but at least regarding ZONE_MOVABLE we 
make sure that the start is aligned to MAX_ORDER_NR_PAGES and that the 
size is aligned to MAX_ORDER_NR_PAGES, see 
find_zone_movable_pfns_for_nodes().

Note that MAX_ORDER_NR_PAGES >= PAGEBLOCK_NR_PAGES.

I think the same is true for any other zone as well. (DMA / DMA32 etc 
should always be at suitable boundaries already IIRC).

Note that memory hotplug always works in section granularity, and a 
section is always <= MAX_ORDER_NR_PAGES, so it's also guaranteed there.

-- 
Cheers

David / dhildenb

Re: [PATCH 0/2] mm/compaction: some fix for the range passed to pageblock_pfn_to_page()

[Andrew Morton]

On Thu,  2 Oct 2025 03:31:38 +0000 Wei Yang <richard.weiyang@gmail.com> wrote:

> During the code reading of isolate_migratepages_range(), first spot the range
> passed to pageblock_pfn_to_page() is different from that to
> isolate_migratepages_block().
> 
> This implies there is a chance that pageblock_pfn_to_page() thinks the range
> is in the same zone, but isolate_migratepages_block() will isolate range in
> two different zones. This is not what we expect.
> 
> Then I found pageblock_pfn_to_page() has an optimization if zone->contiguous,
> this means even the range is across two different zones, it will think the
> range is within the same zone.
> 
> So introduce two patches to fix it:

Review was inconclusive and we aren't sure if there's actually anything
to fix here, so I'll drop this series for now, thanks.