Re: [PATCH v2 2/6] mm/page_owner: use MIGRATE_REASON_NONE instead of -1 for last_migrate_reason

[Vlastimil Babka (SUSE)]

On 6/29/26 04:20, Ye Liu wrote:
> 
> 
> 在 2026/6/27 02:45, Zi Yan 写道:
>> On Thu Jun 25, 2026 at 10:45 PM EDT, Ye Liu wrote:
>>> The last_migrate_reason field uses -1 as a sentinel value to mean "no
>>> migration has happened".  Replace the four bare -1 occurrences with a
>>> local MIGRATE_REASON_NONE define so the intent is explicit and the
>>> magic number is eliminated.
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Ye Liu <ye.liu@xxxxxxxxx>
>>> ---
>>>  mm/page_owner.c | 15 +++++++++++----
>>>  1 file changed, 11 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/mm/page_owner.c b/mm/page_owner.c
>>> index 342549891a8d..ebafa9d7ff07 100644
>>> --- a/mm/page_owner.c
>>> +++ b/mm/page_owner.c
>>> @@ -21,6 +21,13 @@
>>>   */
>>>  #define PAGE_OWNER_STACK_DEPTH (16)
>>>  
>>> +/*
>>> + * Used to indicate that a page has never been migrated, as the valid
>>> + * migrate_reason values are non-negative enum members (MR_* in
>>> + * include/linux/migrate_mode.h).
>> 
>> Why not change last_migrate_reason to enum migrate_reason and functions
>> pass migrate reasons? MR_TYPES probably can be used to indicate no
>> migration has happened if we do not want to add MR_NO_MIGRATION.

Indeed. I'd just add a proper MR_NEVER option with a comment.

>> folio_set_owner_migrate_reason(),
>> __folio_set_owner_migrate_reason(),
>> move_hugetlb_state(), and
>> __update_page_owner_handle() need to update their signatures for this.

They should ideally, but do they "need" as part of this series?
But I guess it's not a big deal.

> Thanks for the review.                                                        
>                                                                               
> Using MR_TYPES as a sentinel is problematic - it's used kernel-wide as an     
> array-size marker (migrate_reason_names[MR_TYPES]). Overloading it to also    
> mean "never migrated" conflates two unrelated semantics. It also has a        

Well it's just for tracepoint and dumping page owner. Nothing prevents us
assigning a value to MR_NEVER that's not -1 (just make MR_NEVER the first or
last option of the enum) and a give it a name e.g. "never_migrated" in
include/trace/events/migrate.h

> subtle out-of-bounds risk: if anyone ever uses the field as an array index    
> without the guard check, MR_TYPES silently points past the end of the array,  
> while -1 faults immediately.                                                  

That wouldn't be a problem either with a proper value.

> Adding MR_NONE = -1 to the enum is the right long-term fix, but that needs    
> proper enum definition changes and cascading signature updates across multiple
> headers — better done as a separate series.                                   
> The current patch is a minimal improvement: it kills the magic number and     
> makes intent explicit without touching any header.                            

Minimal fixes are sometimes necessary for actual bugs, for stable backport
etc. Nothing prevents us doing it properly here.

>> 
>>> + */
>>> +#define MIGRATE_REASON_NONE (-1)
>>> +
>>>  struct page_owner {
>>>  	unsigned short order;
>>>  	short last_migrate_reason;
>>> @@ -339,7 +346,7 @@ noinline void __set_page_owner(struct page *page, unsigned short order,
>>>  	depot_stack_handle_t handle;
>>>  
>>>  	handle = save_stack(gfp_mask);
>>> -	__update_page_owner_handle(page, handle, order, gfp_mask, -1,
>>> +	__update_page_owner_handle(page, handle, order, gfp_mask, MIGRATE_REASON_NONE,
>>>  				   ts_nsec, current->pid, current->tgid,
>>>  				   current->comm);
>>>  	inc_stack_record_count(handle, gfp_mask, 1 << order);
>>> @@ -596,7 +603,7 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn,
>>>  	if (ret >= count)
>>>  		goto err;
>>>  
>>> -	if (page_owner->last_migrate_reason != -1) {
>>> +	if (page_owner->last_migrate_reason != MIGRATE_REASON_NONE) {
>>>  		ret += scnprintf(kbuf + ret, count - ret,
>>>  			"Page has been migrated, last migrate reason: %s\n",
>>>  			migrate_reason_names[page_owner->last_migrate_reason]);
>>> @@ -667,7 +674,7 @@ void __dump_page_owner(const struct page *page)
>>>  		stack_depot_print(handle);
>>>  	}
>>>  
>>> -	if (page_owner->last_migrate_reason != -1)
>>> +	if (page_owner->last_migrate_reason != MIGRATE_REASON_NONE)
>>>  		pr_alert("page has been migrated, last migrate reason: %s\n",
>>>  			migrate_reason_names[page_owner->last_migrate_reason]);
>>>  	page_ext_put(page_ext);
>>> @@ -826,7 +833,7 @@ static void init_pages_in_zone(struct zone *zone)
>>>  
>>>  			/* Found early allocated page */
>>>  			__update_page_owner_handle(page, early_handle, 0, 0,
>>> -						   -1, local_clock(), current->pid,
>>> +						   MIGRATE_REASON_NONE, local_clock(), current->pid,
>>>  						   current->tgid, current->comm);
>>>  			count++;
>>>  ext_put_continue:
>> 
>> 
>> 
>> 
>