Re: [PATCH v5 1/2] mm/memory-failure: fix missing ->mf_stats count in hugetlb poison

[jane.chu@oracle.com]

On 1/14/2026 11:31 PM, Miaohe Lin wrote:
> On 2026/1/15 5:37, Jane Chu wrote:
>> When a newly poisoned subpage ends up in an already poisoned hugetlb
>> folio, 'num_poisoned_pages' is incremented, but the per node ->mf_stats
>> is not. Fix the inconsistency by designating action_result() to update
>> them both.
>>
>> While at it, define __get_huge_page_for_hwpoison() return values in terms
>> of symbol names for better readibility. Also rename
>> folio_set_hugetlb_hwpoison() to hugetlb_update_hwpoison() since the
>> function does more than the conventional bit setting and the fact
>> three possible return values are expected.
>>
>> Fixes: 18f41fa616ee4 ("mm: memory-failure: bump memory failure stats to pglist_data")
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Jane Chu <jane.chu@oracle.com>
>> ---
>> v5 -> v4:
>>    fix a bug pointed out by William and Chris, add comment.
>> v3 -> v4:
>>    incorporate/adapt David's suggestions.
>> v2 -> v3:
>>    No change.
>> v1 -> v2:
>>    adapted David and Liam's comment, define __get_huge_page_for_hwpoison()
>> return values in terms of symbol names instead of naked integers for better
>> readibility.  #define instead of enum is used since the function has footprint
>> outside MF, just try to limit the MF specifics local.
>>    also renamed folio_set_hugetlb_hwpoison() to hugetlb_update_hwpoison()
>> since the function does more than the conventional bit setting and the
>> fact three possible return values are expected.
>>
>> Signed-off-by: Jane Chu <jane.chu@oracle.com>
> 
> This patch looks good to me. A few nits below.
> 
>> ---
>>   mm/memory-failure.c | 87 ++++++++++++++++++++++++++++-----------------
>>   1 file changed, 54 insertions(+), 33 deletions(-)
>>
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index fbc5a01260c8..2563718c34c6 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -1883,12 +1883,24 @@ static unsigned long __folio_free_raw_hwp(struct folio *folio, bool move_flag)
>>   	return count;
>>   }
>>   
>> -static int folio_set_hugetlb_hwpoison(struct folio *folio, struct page *page)
>> +#define	MF_HUGETLB_FOLIO_PRE_POISONED	3  /* folio already poisoned */
>> +#define	MF_HUGETLB_PAGE_PRE_POISONED	4  /* exact page already poisoned */
>> +/*
>> + * Set hugetlb folio as hwpoisoned, update folio private raw hwpoison list
>> + * to keep track of the poisoned pages.
>> + * Return:
>> + *	0: folio was not already poisoned;
>> + *	MF_HUGETLB_FOLIO_PRE_POISONED: folio was already poisoned: either
>> + *		multiple pages being poisoned, or per page information unclear,
>> + *	MF_HUGETLB_PAGE_PRE_POISONED: folio was already poisoned, an exact
>> + *		poisoned page is being consumed again.
>> + */
>> +static int hugetlb_update_hwpoison(struct folio *folio, struct page *page)
>>   {
>>   	struct llist_head *head;
>>   	struct raw_hwp_page *raw_hwp;
>>   	struct raw_hwp_page *p;
>> -	int ret = folio_test_set_hwpoison(folio) ? -EHWPOISON : 0;
>> +	int ret = folio_test_set_hwpoison(folio) ? MF_HUGETLB_FOLIO_PRE_POISONED : 0;
>>   
>>   	/*
>>   	 * Once the hwpoison hugepage has lost reliable raw error info,
>> @@ -1896,20 +1908,17 @@ static int folio_set_hugetlb_hwpoison(struct folio *folio, struct page *page)
>>   	 * so skip to add additional raw error info.
>>   	 */
>>   	if (folio_test_hugetlb_raw_hwp_unreliable(folio))
>> -		return -EHWPOISON;
>> +		return MF_HUGETLB_FOLIO_PRE_POISONED;
>>   	head = raw_hwp_list_head(folio);
>>   	llist_for_each_entry(p, head->first, node) {
>>   		if (p->page == page)
>> -			return -EHWPOISON;
>> +			return MF_HUGETLB_PAGE_PRE_POISONED;
>>   	}
>>   
>>   	raw_hwp = kmalloc(sizeof(struct raw_hwp_page), GFP_ATOMIC);
>>   	if (raw_hwp) {
>>   		raw_hwp->page = page;
>>   		llist_add(&raw_hwp->node, head);
>> -		/* the first error event will be counted in action_result(). */
>> -		if (ret)
>> -			num_poisoned_pages_inc(page_to_pfn(page));
>>   	} else {
>>   		/*
>>   		 * Failed to save raw error info.  We no longer trace all
>> @@ -1955,44 +1964,43 @@ void folio_clear_hugetlb_hwpoison(struct folio *folio)
>>   	folio_free_raw_hwp(folio, true);
>>   }
>>   
>> +#define	MF_HUGETLB_FREED		0	/* freed hugepage */
>> +#define	MF_HUGETLB_IN_USED		1	/* in-use hugepage */
> 
> It might be better to define all of them together. e.g.
> 
> #define MF_HUGETLB_FREED		0 	/* freed hugepage */
> #define MF_HUGETLB_IN_USED		1	/* in-use hugepage */
> #define MF_HUGETLB_NON_HUGEPAGE		2	/* not a hugepage */
> #define MF_HUGETLB_FOLIO_PRE_POISONED	3  	/* folio already poisoned */
> #define MF_HUGETLB_PAGE_PRE_POISONED	4  	/* exact page already poisoned */
> #define MF_HUGETLB_RETRY		5	/* the hugepage is busy (try to retry) */
> 

Will do, thanks!

>>   /*
>>    * Called from hugetlb code with hugetlb_lock held.
>> - *
>> - * Return values:
>> - *   0             - free hugepage
>> - *   1             - in-use hugepage
>> - *   2             - not a hugepage
>> - *   -EBUSY        - the hugepage is busy (try to retry)
>> - *   -EHWPOISON    - the hugepage is already hwpoisoned
>>    */
>>   int __get_huge_page_for_hwpoison(unsigned long pfn, int flags,
>>   				 bool *migratable_cleared)
>>   {
>>   	struct page *page = pfn_to_page(pfn);
>>   	struct folio *folio = page_folio(page);
>> -	int ret = 2;	/* fallback to normal page handling */
>> +	int ret = -EINVAL;
>>   	bool count_increased = false;
>> +	int rc;
>>   
>>   	if (!folio_test_hugetlb(folio))
>>   		goto out;
>>   
>>   	if (flags & MF_COUNT_INCREASED) {
>> -		ret = 1;
>> +		ret = MF_HUGETLB_IN_USED;
>>   		count_increased = true;
>>   	} else if (folio_test_hugetlb_freed(folio)) {
>> -		ret = 0;
>> +		ret = MF_HUGETLB_FREED;
>>   	} else if (folio_test_hugetlb_migratable(folio)) {
>> -		ret = folio_try_get(folio);
>> -		if (ret)
>> +		if (folio_try_get(folio)) {
>> +			ret = MF_HUGETLB_IN_USED;
>>   			count_increased = true;
>> +		} else
>> +			ret = MF_HUGETLB_FREED;
>>   	} else {
>>   		ret = -EBUSY;
>>   		if (!(flags & MF_NO_RETRY))
>>   			goto out;
>>   	}
>>   
>> -	if (folio_set_hugetlb_hwpoison(folio, page)) {
>> -		ret = -EHWPOISON;
>> +	rc = hugetlb_update_hwpoison(folio, page);
>> +	if (rc >= MF_HUGETLB_FOLIO_PRE_POISONED) {
>> +		ret = rc;
>>   		goto out;
>>   	}
>>   
>> @@ -2017,10 +2025,15 @@ int __get_huge_page_for_hwpoison(unsigned long pfn, int flags,
>>    * with basic operations like hugepage allocation/free/demotion.
>>    * 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
>> + *	-EBUSY		- not recovered
>> + *	-EOPNOTSUPP	- hwpoison_filter'ed
>> + *	-EHWPOISON	- folio or exact page already poisoned
>>    */
>>   static int try_memory_failure_hugetlb(unsigned long pfn, int flags, int *hugetlb)
>>   {
>> -	int res;
>> +	int res, rv;
>>   	struct page *p = pfn_to_page(pfn);
>>   	struct folio *folio;
>>   	unsigned long page_flags;
>> @@ -2029,22 +2042,30 @@ static int try_memory_failure_hugetlb(unsigned long pfn, int flags, int *hugetlb
>>   	*hugetlb = 1;
>>   retry:
>>   	res = get_huge_page_for_hwpoison(pfn, flags, &migratable_cleared);
>> -	if (res == 2) { /* fallback to normal page handling */
>> +	switch (res) {
>> +	case -EINVAL:	/* fallback to normal page handling */
>>   		*hugetlb = 0;
>>   		return 0;
>> -	} else if (res == -EHWPOISON) {
>> -		if (flags & MF_ACTION_REQUIRED) {
>> -			folio = page_folio(p);
>> -			res = kill_accessing_process(current, folio_pfn(folio), flags);
>> -		}
>> -		action_result(pfn, MF_MSG_ALREADY_POISONED, MF_FAILED);
>> -		return res;
>> -	} else if (res == -EBUSY) {
>> +	case -EBUSY:
>>   		if (!(flags & MF_NO_RETRY)) {
>>   			flags |= MF_NO_RETRY;
>>   			goto retry;
>>   		}
>>   		return action_result(pfn, MF_MSG_GET_HWPOISON, MF_IGNORED);
>> +	case MF_HUGETLB_FOLIO_PRE_POISONED:
>> +	case MF_HUGETLB_PAGE_PRE_POISONED:
>> +		rv = -EHWPOISON;
>> +		if (flags & MF_ACTION_REQUIRED) {
>> +			folio = page_folio(p);
>> +			rv = kill_accessing_process(current, folio_pfn(folio), flags);
>> +		}
>> +		if (res == MF_HUGETLB_PAGE_PRE_POISONED)
>> +			action_result(pfn, MF_MSG_ALREADY_POISONED, MF_FAILED);
>> +		else
>> +			action_result(pfn, MF_MSG_HUGE, MF_FAILED);
>> +		return rv;
>> +	default:
> 
> Should we add a warn here?

Yes, added a WARN_ON.

thanks!
-jane

> 
> Thanks.
> .