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

[Miaohe Lin <linmiaohe@huawei.com>]

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) */

>  /*
>   * 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?

Thanks.
.