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

[Ye Liu <ye.liu@linux.dev>]

在 2026/6/29 16:00, 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@linux.dev>
>>>> ---
>>>>  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.
> 
Indeed, let me do it properly.  In v3 I'll split this into two patches:
                                                                       
1. Add MR_NEVER (value 10) to enum migrate_reason in migrate_mode.h,   
add the corresponding "never_migrated" string in the MIGRATE_REASON    
trace macro, and replace the four bare -1 with MR_NEVER in             
page_owner.c.  This removes the local MIGRATE_REASON_NONE define       
entirely.                                                              
2. A follow-up cleanup that converts all 'int reason' parameters in the
callchain to 'enum migrate_reason'.  The affected functions are:       
                                                                       
2.   page_owner: __folio_set_owner_migrate_reason(),                   
          folio_set_owner_migrate_reason()                             
  migrate: migrate_pages(), migrate_pages_sync(),                      
       migrate_pages_batch(), migrate_folios_move(),                   
       migrate_hugetlbs(), unmap_and_move_huge_page()                  
  hugetlb: move_hugetlb_state(), htlb_allow_alloc_fallback()           
  trace: mm_migrate_pages / mm_migrate_pages_start events              
                                                                       
2. The 'short last_migrate_reason' struct field and the                
__update_page_owner_handle() parameter are intentionally left as       
'short' since struct page_owner is per-page metadata and the size      
matters there.                                                         
                                                                       
Thanks for the review.                                                 

>>>
>>>> + */
>>>> +#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:
>>>
>>>
>>>
>>>
>>
> 

-- 
Thanks,
Ye Liu