On 12/23/2025 12:50 AM, David Hildenbrand (Red Hat) wrote:
On 12/23/25 02:21, 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
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.
mm/memory-failure.c | 56 ++++++++++++++++++++++++++------------------- 1 file changed, 33 insertions(+), 23 deletions(-)
diff --git a/mm/memory-failure.c b/mm/memory-failure.c index fbc5a01260c8..8b47e8a1b12d 100644 --- a/mm/memory-failure.c +++ b/mm/memory-failure.c @@ -1883,12 +1883,18 @@ 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_ALREADY_POISONED 3 /* already poisoned */ +#define MF_HUGETLB_ACC_EXISTING_POISON 4 /* accessed existing poisoned page */
What happened to the idea of using an enum?
I briefly mentioned the reason of using #define instead of enum in the v1 -> v2 comment, more below.
+/*
- Set hugetlb folio as hwpoisoned, update folio private raw hwpoison
list
- to keep track of the poisoned pages.
- */
+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_ALREADY_POISONED : 0; /* * Once the hwpoison hugepage has lost reliable raw error info, @@ -1896,20 +1902,18 @@ 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_ALREADY_POISONED;
head = raw_hwp_list_head(folio); llist_for_each_entry(p, head->first, node) { if (p->page == page) - return -EHWPOISON; + return MF_HUGETLB_ACC_EXISTING_POISON; } 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,32 +1959,30 @@ 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 */ +#define MF_NOT_HUGETLB 2 /* not a hugepage */
If you're already dealing with negative error codes, "MF_NOT_HUGETLB" nicely translated to -EINVAL.
Agreed, thanks, will make the change in next round.
But I wonder if it would be cleaner to just define all values in an enum and return that enum instead of an int from the functions.
enum md_hugetlb_status { MF_HUGETLB_INVALID, /* not a hugetlb folio */ MF_HUGETLB_BUSY, /* busy, retry later */ MF_HUGETLB_FREED, /* hugetlb folio was freed */ MF_HUGETLB_IN_USED, /* ??? no idea what that really means */ MF_HUGETLB_FOLIO_PRE_POISONED, /* folio already poisoned, per- page information unclear */ MF_HUGETLB_PAGE_PRE_POISONED, /* exact page already poisoned */ }
enum is nicer than #define for sure, the only concern I have is that, this enum as a return type from _get_huge_page_for_hwpoison()/ get_huge_page_for_hwpoison() will leave footprint in include/linux/mm.h that declares _get_huge_page_for_hwpoison(), include/linux/hugetlb.h that declares/defines get_huge_page_for_hwpoison(), and mm/hugetlb.c.
Is there a neat way? or, am I nitpicking? :)
thanks, -jane