In MIGRATE_ISOLATE case, zone freepage state shouldn't be modified as
caller will take care of it. Add missing is_migrate_isolate() here to
avoid possible unbalanced freepage state.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/page_alloc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index a35ef385d906..94baf33da865 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -873,7 +873,8 @@ static inline bool set_page_guard(struct zone *zone, struct page *page,
 	INIT_LIST_HEAD(&page->buddy_list);
 	set_page_private(page, order);
 	/* Guard pages are not available for any usage */
-	__mod_zone_freepage_state(zone, -(1 << order), migratetype);
+	if (!is_migrate_isolate(migratetype))
+		__mod_zone_freepage_state(zone, -(1 << order), migratetype);
 
 	return true;
 }
-- 
2.23.0

On 09.09.22 11:24, Miaohe Lin wrote:
> In MIGRATE_ISOLATE case, zone freepage state shouldn't be modified as
> caller will take care of it. Add missing is_migrate_isolate() here to
> avoid possible unbalanced freepage state.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>   mm/page_alloc.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index a35ef385d906..94baf33da865 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -873,7 +873,8 @@ static inline bool set_page_guard(struct zone *zone, struct page *page,
>   	INIT_LIST_HEAD(&page->buddy_list);
>   	set_page_private(page, order);
>   	/* Guard pages are not available for any usage */
> -	__mod_zone_freepage_state(zone, -(1 << order), migratetype);
> +	if (!is_migrate_isolate(migratetype))
> +		__mod_zone_freepage_state(zone, -(1 << order), migratetype);
>   
>   	return true;
>   }

Do we have a fixes: tag for this one?

Can it even happen that the pageblock is isolated when we end up in this 
function? IIUC, we'd have an allocation in an isolated pageblock, which 
would be wrong already?

-- 
Thanks,

David / dhildenb

On 2022/9/9 19:31, David Hildenbrand wrote:
> On 09.09.22 11:24, Miaohe Lin wrote:
>> In MIGRATE_ISOLATE case, zone freepage state shouldn't be modified as
>> caller will take care of it. Add missing is_migrate_isolate() here to
>> avoid possible unbalanced freepage state.
>>
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>> ---
>>   mm/page_alloc.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index a35ef385d906..94baf33da865 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -873,7 +873,8 @@ static inline bool set_page_guard(struct zone *zone, struct page *page,
>>       INIT_LIST_HEAD(&page->buddy_list);
>>       set_page_private(page, order);
>>       /* Guard pages are not available for any usage */
>> -    __mod_zone_freepage_state(zone, -(1 << order), migratetype);
>> +    if (!is_migrate_isolate(migratetype))
>> +        __mod_zone_freepage_state(zone, -(1 << order), migratetype);
>>         return true;
>>   }
> 
> Do we have a fixes: tag for this one?
> 
> Can it even happen that the pageblock is isolated when we end up in this function? IIUC, we'd have an allocation in an isolated pageblock, which would be wrong already?

For "normal" page allocation case, migratetype can't be MIGRATE_ISOLATE. So it's fine. But when called from take_page_off_buddy(), the issue
could be triggered as it breaks the assumption in the set_page_guard (that migratetype can't be MIGRATE_ISOLATE). So the fixes tag might be:

	Fixes: 06be6ff3d2ec ("mm,hwpoison: rework soft offline for free pages")

Or am I miss something?

Many thanks for your review and comment in this series, David. :)

Thanks,
Miaohe Lin

On Fri, Sep 09, 2022 at 05:24:43PM +0800, Miaohe Lin wrote:
> In MIGRATE_ISOLATE case, zone freepage state shouldn't be modified as
> caller will take care of it. Add missing is_migrate_isolate() here to
> avoid possible unbalanced freepage state.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Ok, I gave it some thought and I guess it's safe to assume that this would happen if
someone isolates the block, and then we face an MCE failure/soft-offline on a page
within that block.

take_page_off_buddy
 break_down_buddy_pages
  set_page_guard

will trigger __mod_zone_freepage_state(), which already had been triggered back
when the block was isolated.

I think the changelog could grow fatter to better explain the issue.

Reviewed-by: Oscar Salvador <osalvador@suse.de>

> ---
>  mm/page_alloc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index a35ef385d906..94baf33da865 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -873,7 +873,8 @@ static inline bool set_page_guard(struct zone *zone, struct page *page,
>  	INIT_LIST_HEAD(&page->buddy_list);
>  	set_page_private(page, order);
>  	/* Guard pages are not available for any usage */
> -	__mod_zone_freepage_state(zone, -(1 << order), migratetype);
> +	if (!is_migrate_isolate(migratetype))
> +		__mod_zone_freepage_state(zone, -(1 << order), migratetype);
>  
>  	return true;
>  }
> -- 
> 2.23.0
> 

-- 
Oscar Salvador
SUSE Labs

On 2022/9/15 15:26, Oscar Salvador wrote:
> On Fri, Sep 09, 2022 at 05:24:43PM +0800, Miaohe Lin wrote:
>> In MIGRATE_ISOLATE case, zone freepage state shouldn't be modified as
>> caller will take care of it. Add missing is_migrate_isolate() here to
>> avoid possible unbalanced freepage state.
>>
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> 
> Ok, I gave it some thought and I guess it's safe to assume that this would happen if
> someone isolates the block, and then we face an MCE failure/soft-offline on a page
> within that block.
> 
> take_page_off_buddy
>  break_down_buddy_pages
>   set_page_guard
> 
> will trigger __mod_zone_freepage_state(), which already had been triggered back
> when the block was isolated.
> 
> I think the changelog could grow fatter to better explain the issue.

That will be really helpful. Will do it in v2.

> 
> Reviewed-by: Oscar Salvador <osalvador@suse.de>

Many thanks for your suggestion and review.

Thanks,
Miaohe Lin

> 
>> ---
>>  mm/page_alloc.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index a35ef385d906..94baf33da865 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -873,7 +873,8 @@ static inline bool set_page_guard(struct zone *zone, struct page *page,
>>  	INIT_LIST_HEAD(&page->buddy_list);
>>  	set_page_private(page, order);
>>  	/* Guard pages are not available for any usage */
>> -	__mod_zone_freepage_state(zone, -(1 << order), migratetype);
>> +	if (!is_migrate_isolate(migratetype))
>> +		__mod_zone_freepage_state(zone, -(1 << order), migratetype);
>>  
>>  	return true;
>>  }
>> -- 
>> 2.23.0
>>
>