Re: [RESEND PATCH] mm/memory_failure: use bool for hugetlb indicator in try_memory_failure_hugetlb

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

在 2026/5/13 11:36, Oscar Salvador 写道:
> On Wed, May 13, 2026 at 10:48:52AM +0800, Ye Liu wrote:
>> From: Ye Liu <liuye@kylinos.cn>
>>
>> The hugetlb indicator in try_memory_failure_hugetlb is a Boolean
>> flag, but was declared and assigned as int/0/1. Convert to `bool`
>> and `true`/`false` for clarity and type safety.
>>
>> - try_memory_failure_hugetlb(unsigned long pfn, int flags, bool *hugetlb)
>> - testcase path in memory_failure(): bool hugetlb = false
>> - clear semantic conversion in MF_HUGETLB_NON_HUGEPAGE
>> - preserve behavior (no functional change)
>>
>> Signed-off-by: Ye Liu <liuye@kylinos.cn>
>> Acked-by: Miaohe Lin <linmiaohe@huawei.com>
>> ---
>>  mm/memory-failure.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index 866c4428ac7e..f164fc5959f0 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -2032,7 +2032,7 @@ int __get_huge_page_for_hwpoison(unsigned long pfn, int flags,
>>   *	-EHWPOISON	- folio or exact page already poisoned
>>   *	-EFAULT		- kill_accessing_process finds current->mm null
>>   */
>> -static int try_memory_failure_hugetlb(unsigned long pfn, int flags, int *hugetlb)
>> +static int try_memory_failure_hugetlb(unsigned long pfn, int flags, bool *hugetlb)
>>  {
>>  	int res, rv;
>>  	struct page *p = pfn_to_page(pfn);
>> @@ -2040,12 +2040,12 @@ static int try_memory_failure_hugetlb(unsigned long pfn, int flags, int *hugetlb
>>  	unsigned long page_flags;
>>  	bool migratable_cleared = false;
>>  
>> -	*hugetlb = 1;
>> +	*hugetlb = true;
>>  retry:
>>  	res = get_huge_page_for_hwpoison(pfn, flags, &migratable_cleared);
>>  	switch (res) {
>>  	case MF_HUGETLB_NON_HUGEPAGE:	/* fallback to normal page handling */
>> -		*hugetlb = 0;
>> +		*hugetlb = false;
> 
> Do we really need this boolean though?
> Why do not simply return ENOENT when we find a non-hugetlb page, and then the caller
> knows that if we get ENOENT, it can proceed with normal page handling?
> 
> I might be missing something, but is not the following more cleaer?
> 
>  diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>  index ee42d4361309..44f388df5731 100644
>  --- a/mm/memory-failure.c
>  +++ b/mm/memory-failure.c
>  @@ -2026,13 +2026,14 @@ int __get_huge_page_for_hwpoison(unsigned long pfn, int flags,
>    * So some of prechecks for hwpoison (pinning, and testing/setting
>    * PageHWPoison) should be done in single hugetlb_lock range.
>    * Returns:
>  - *     0               - not hugetlb, or recovered
>  + *     0               - recovered
>  + *     -ENOENT         - no hugetlb page
>    *     -EBUSY          - not recovered
>    *     -EOPNOTSUPP     - hwpoison_filter'ed
>    *     -EHWPOISON      - folio or exact page already poisoned
>    *     -EFAULT         - kill_accessing_process finds current->mm null
>    */
>  -static int try_memory_failure_hugetlb(unsigned long pfn, int flags, int *hugetlb)
>  +static int try_memory_failure_hugetlb(unsigned long pfn, int flags)
>   {
>          int res, rv;
>          struct page *p = pfn_to_page(pfn);
>  @@ -2040,13 +2041,11 @@ static int try_memory_failure_hugetlb(unsigned long pfn, int flags, int *hugetlb
>          unsigned long page_flags;
>          bool migratable_cleared = false;
>  
>  -       *hugetlb = 1;
>   retry:
>          res = get_huge_page_for_hwpoison(pfn, flags, &migratable_cleared);
>          switch (res) {
>          case MF_HUGETLB_NON_HUGEPAGE:   /* fallback to normal page handling */
>  -               *hugetlb = 0;
>  -               return 0;
>  +               return -ENOENT;
>          case MF_HUGETLB_RETRY:
>                  if (!(flags & MF_NO_RETRY)) {
>                          flags |= MF_NO_RETRY;
>  @@ -2107,7 +2106,7 @@ static int try_memory_failure_hugetlb(unsigned long pfn, int flags, int *hugetlb
>   }
>  
>   #else
>  -static inline int try_memory_failure_hugetlb(unsigned long pfn, int flags, int *hugetlb)
>  +static inline int try_memory_failure_hugetlb(unsigned long pfn, int flags)
>   {
>          return 0;
>   }
>  @@ -2386,8 +2385,11 @@ int memory_failure(unsigned long pfn, int flags)
>          }
>  
>   try_again:
>  -       res = try_memory_failure_hugetlb(pfn, flags, &hugetlb);
>  -       if (hugetlb)
>  +       res = try_memory_failure_hugetlb(pfn, flags);
>  +       /*
>  +        * -ENOENT means the page we found is not hugetlb, so proceed with normal page handling
>  +        */
>  +       if (res != -ENOENT)
>                  goto unlock_mutex;
>  
>          if (TestSetPageHWPoison(p)) {
> 
> 


Hi Oscar,

Good point.  Using -ENOENT to distinguish "not a hugetlb page" from
"hugetlb handled" is indeed cleaner than carrying an extra output
parameter.

One thing to note: the #else stub when CONFIG_HUGETLB_PAGE is not set
currently does:

    return 0;

which with your change would mean "hugetlb handled, skip normal path"
instead of the intended "not hugetlb, proceed with normal handling".
It should be changed to:

    return -ENOENT;

Otherwise the non-hugetlb config would silently skip all normal page
failure processing.

-- 
Thanks,
Ye Liu