Added two explicit MF_MSG messages describing failure in get_hwpoison_page.
Attemped to document the definition of various action names, and made a few
adjustment to the action_result() calls.
Signed-off-by: Jane Chu <jane.chu@oracle.com>
---
include/linux/mm.h | 2 ++
include/ras/ras_event.h | 2 ++
mm/memory-failure.c | 38 +++++++++++++++++++++++++++++++++-----
3 files changed, 37 insertions(+), 5 deletions(-)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 9849dfda44d4..b4598c6a393a 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -4111,6 +4111,7 @@ enum mf_action_page_type {
MF_MSG_DIFFERENT_COMPOUND,
MF_MSG_HUGE,
MF_MSG_FREE_HUGE,
+ MF_MSG_GET_HWPOISON,
MF_MSG_UNMAP_FAILED,
MF_MSG_DIRTY_SWAPCACHE,
MF_MSG_CLEAN_SWAPCACHE,
@@ -4124,6 +4125,7 @@ enum mf_action_page_type {
MF_MSG_BUDDY,
MF_MSG_DAX,
MF_MSG_UNSPLIT_THP,
+ MF_MSG_ALREADY_POISONED,
MF_MSG_UNKNOWN,
};
diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
index c011ea236e9b..b3f6832a94fe 100644
--- a/include/ras/ras_event.h
+++ b/include/ras/ras_event.h
@@ -360,6 +360,7 @@ TRACE_EVENT(aer_event,
EM ( MF_MSG_DIFFERENT_COMPOUND, "different compound page after locking" ) \
EM ( MF_MSG_HUGE, "huge page" ) \
EM ( MF_MSG_FREE_HUGE, "free huge page" ) \
+ EM ( MF_MSG_GET_HWPOISON, "get hwpoison page" ) \
EM ( MF_MSG_UNMAP_FAILED, "unmapping failed page" ) \
EM ( MF_MSG_DIRTY_SWAPCACHE, "dirty swapcache page" ) \
EM ( MF_MSG_CLEAN_SWAPCACHE, "clean swapcache page" ) \
@@ -373,6 +374,7 @@ TRACE_EVENT(aer_event,
EM ( MF_MSG_BUDDY, "free buddy page" ) \
EM ( MF_MSG_DAX, "dax page" ) \
EM ( MF_MSG_UNSPLIT_THP, "unsplit thp" ) \
+ EM ( MF_MSG_ALREADY_POISONED, "already poisoned" ) \
EMe ( MF_MSG_UNKNOWN, "unknown page" )
/*
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 739311e121af..1e22d73c9329 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -879,6 +879,28 @@ static int kill_accessing_process(struct task_struct *p, unsigned long pfn,
return ret > 0 ? -EHWPOISON : -EFAULT;
}
+/*
+ * MF_IGNORED - The m-f() handler marks the page as PG_hwpoisoned'ed.
+ * But it could not do more to isolate the page from being accessed again,
+ * nor does it kill the process. This is extremely rare and one of the
+ * potential causes is that the page state has been changed due to
+ * underlying race condition. This is the most severe outcomes.
+ *
+ * MF_FAILED - The m-f() handler marks the page as PG_hwpoisoned'ed. It
+ * should have killed the process, but it can't isolate the page, due to
+ * conditions such as extra pin, unmap failure, etc. Accessing the page
+ * again will trigger another MCE and the process will be killed by the
+ * m-f() handler immediately.
+ *
+ * MF_DELAYED - The m-f() handler marks the page as PG_hwpoisoned'ed. The
+ * page is unmapped, but perhaps remains in LRU or file mapping. An attempt
+ * to access the page again will trigger page fault and the PF handler
+ * will kill the process.
+ *
+ * MF_RECOVERED - The m-f() handler marks the page as PG_hwpoisoned'ed.
+ * The page has been completely isolated, that is, unmapped, taken out of
+ * the buddy system, or hole-punnched out of the file mapping.
+ */
static const char *action_name[] = {
[MF_IGNORED] = "Ignored",
[MF_FAILED] = "Failed",
@@ -893,6 +915,7 @@ static const char * const action_page_types[] = {
[MF_MSG_DIFFERENT_COMPOUND] = "different compound page after locking",
[MF_MSG_HUGE] = "huge page",
[MF_MSG_FREE_HUGE] = "free huge page",
+ [MF_MSG_GET_HWPOISON] = "get hwpoison page",
[MF_MSG_UNMAP_FAILED] = "unmapping failed page",
[MF_MSG_DIRTY_SWAPCACHE] = "dirty swapcache page",
[MF_MSG_CLEAN_SWAPCACHE] = "clean swapcache page",
@@ -906,6 +929,7 @@ static const char * const action_page_types[] = {
[MF_MSG_BUDDY] = "free buddy page",
[MF_MSG_DAX] = "dax page",
[MF_MSG_UNSPLIT_THP] = "unsplit thp",
+ [MF_MSG_ALREADY_POISONED] = "already poisoned",
[MF_MSG_UNKNOWN] = "unknown page",
};
@@ -1013,12 +1037,13 @@ static int me_kernel(struct page_state *ps, struct page *p)
/*
* Page in unknown state. Do nothing.
+ * This is a catch-all in case we fail to make sense of the page state.
*/
static int me_unknown(struct page_state *ps, struct page *p)
{
pr_err("%#lx: Unknown page state\n", page_to_pfn(p));
unlock_page(p);
- return MF_FAILED;
+ return MF_IGNORED;
}
/*
@@ -2055,6 +2080,8 @@ static int try_memory_failure_hugetlb(unsigned long pfn, int flags, int *hugetlb
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;
}
return res;
} else if (res == -EBUSY) {
@@ -2062,7 +2089,7 @@ static int try_memory_failure_hugetlb(unsigned long pfn, int flags, int *hugetlb
flags |= MF_NO_RETRY;
goto retry;
}
- return action_result(pfn, MF_MSG_UNKNOWN, MF_IGNORED);
+ return action_result(pfn, MF_MSG_GET_HWPOISON, MF_IGNORED);
}
folio = page_folio(p);
@@ -2097,7 +2124,7 @@ static int try_memory_failure_hugetlb(unsigned long pfn, int flags, int *hugetlb
if (!hwpoison_user_mappings(folio, p, pfn, flags)) {
folio_unlock(folio);
- return action_result(pfn, MF_MSG_UNMAP_FAILED, MF_IGNORED);
+ return action_result(pfn, MF_MSG_UNMAP_FAILED, MF_FAILED);
}
return identify_page_state(pfn, p, page_flags);
@@ -2231,6 +2258,7 @@ int memory_failure(unsigned long pfn, int flags)
res = kill_accessing_process(current, pfn, flags);
if (flags & MF_COUNT_INCREASED)
put_page(p);
+ action_result(pfn, MF_MSG_ALREADY_POISONED, MF_FAILED);
goto unlock_mutex;
}
@@ -2267,7 +2295,7 @@ int memory_failure(unsigned long pfn, int flags)
}
goto unlock_mutex;
} else if (res < 0) {
- res = action_result(pfn, MF_MSG_UNKNOWN, MF_IGNORED);
+ res = action_result(pfn, MF_MSG_GET_HWPOISON, MF_IGNORED);
goto unlock_mutex;
}
}
@@ -2363,7 +2391,7 @@ int memory_failure(unsigned long pfn, int flags)
* Abort on fail: __filemap_remove_folio() assumes unmapped page.
*/
if (!hwpoison_user_mappings(folio, p, pfn, flags)) {
- res = action_result(pfn, MF_MSG_UNMAP_FAILED, MF_IGNORED);
+ res = action_result(pfn, MF_MSG_UNMAP_FAILED, MF_FAILED);
goto unlock_page;
}
--
2.39.3On 2024/5/22 7:54, Jane Chu wrote:
> Added two explicit MF_MSG messages describing failure in get_hwpoison_page.
> Attemped to document the definition of various action names, and made a few
> adjustment to the action_result() calls.
>
> Signed-off-by: Jane Chu <jane.chu@oracle.com>
Thanks for your patch. This really improves the code.
> ---
> include/linux/mm.h | 2 ++
> include/ras/ras_event.h | 2 ++
> mm/memory-failure.c | 38 +++++++++++++++++++++++++++++++++-----
> 3 files changed, 37 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 9849dfda44d4..b4598c6a393a 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -4111,6 +4111,7 @@ enum mf_action_page_type {
> MF_MSG_DIFFERENT_COMPOUND,
> MF_MSG_HUGE,
> MF_MSG_FREE_HUGE,
> + MF_MSG_GET_HWPOISON,
> MF_MSG_UNMAP_FAILED,
> MF_MSG_DIRTY_SWAPCACHE,
> MF_MSG_CLEAN_SWAPCACHE,
> @@ -4124,6 +4125,7 @@ enum mf_action_page_type {
> MF_MSG_BUDDY,
> MF_MSG_DAX,
> MF_MSG_UNSPLIT_THP,
> + MF_MSG_ALREADY_POISONED,
> MF_MSG_UNKNOWN,
> };
>
> diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
> index c011ea236e9b..b3f6832a94fe 100644
> --- a/include/ras/ras_event.h
> +++ b/include/ras/ras_event.h
> @@ -360,6 +360,7 @@ TRACE_EVENT(aer_event,
> EM ( MF_MSG_DIFFERENT_COMPOUND, "different compound page after locking" ) \
> EM ( MF_MSG_HUGE, "huge page" ) \
> EM ( MF_MSG_FREE_HUGE, "free huge page" ) \
> + EM ( MF_MSG_GET_HWPOISON, "get hwpoison page" ) \
> EM ( MF_MSG_UNMAP_FAILED, "unmapping failed page" ) \
> EM ( MF_MSG_DIRTY_SWAPCACHE, "dirty swapcache page" ) \
> EM ( MF_MSG_CLEAN_SWAPCACHE, "clean swapcache page" ) \
> @@ -373,6 +374,7 @@ TRACE_EVENT(aer_event,
> EM ( MF_MSG_BUDDY, "free buddy page" ) \
> EM ( MF_MSG_DAX, "dax page" ) \
> EM ( MF_MSG_UNSPLIT_THP, "unsplit thp" ) \
> + EM ( MF_MSG_ALREADY_POISONED, "already poisoned" ) \
> EMe ( MF_MSG_UNKNOWN, "unknown page" )
>
> /*
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 739311e121af..1e22d73c9329 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -879,6 +879,28 @@ static int kill_accessing_process(struct task_struct *p, unsigned long pfn,
> return ret > 0 ? -EHWPOISON : -EFAULT;
> }
>
> +/*
> + * MF_IGNORED - The m-f() handler marks the page as PG_hwpoisoned'ed.
> + * But it could not do more to isolate the page from being accessed again,
> + * nor does it kill the process. This is extremely rare and one of the
> + * potential causes is that the page state has been changed due to
> + * underlying race condition. This is the most severe outcomes.
> + *
> + * MF_FAILED - The m-f() handler marks the page as PG_hwpoisoned'ed. It
> + * should have killed the process, but it can't isolate the page, due to
> + * conditions such as extra pin, unmap failure, etc. Accessing the page
> + * again will trigger another MCE and the process will be killed by the
> + * m-f() handler immediately.
> + *
> + * MF_DELAYED - The m-f() handler marks the page as PG_hwpoisoned'ed. The
> + * page is unmapped, but perhaps remains in LRU or file mapping. An attempt
Would the page remain in LRU or file mapping? IIUC, MF_DELAYED is returned from two functions:
1. me_swapcache_dirty. Page lives in swap cache and removed from LRU.
2. kvm_gmem_error_folio. Page range is unmapped. It seems page won't be in the LRU or page cache.
Or am I miss something?
> + * to access the page again will trigger page fault and the PF handler
> + * will kill the process.
> + *
> + * MF_RECOVERED - The m-f() handler marks the page as PG_hwpoisoned'ed.
> + * The page has been completely isolated, that is, unmapped, taken out of
> + * the buddy system, or hole-punnched out of the file mapping.
> + */
> static const char *action_name[] = {
> [MF_IGNORED] = "Ignored",
> [MF_FAILED] = "Failed",
> @@ -893,6 +915,7 @@ static const char * const action_page_types[] = {
> [MF_MSG_DIFFERENT_COMPOUND] = "different compound page after locking",
> [MF_MSG_HUGE] = "huge page",
> [MF_MSG_FREE_HUGE] = "free huge page",
> + [MF_MSG_GET_HWPOISON] = "get hwpoison page",
> [MF_MSG_UNMAP_FAILED] = "unmapping failed page",
> [MF_MSG_DIRTY_SWAPCACHE] = "dirty swapcache page",
> [MF_MSG_CLEAN_SWAPCACHE] = "clean swapcache page",
> @@ -906,6 +929,7 @@ static const char * const action_page_types[] = {
> [MF_MSG_BUDDY] = "free buddy page",
> [MF_MSG_DAX] = "dax page",
> [MF_MSG_UNSPLIT_THP] = "unsplit thp",
> + [MF_MSG_ALREADY_POISONED] = "already poisoned",
> [MF_MSG_UNKNOWN] = "unknown page",
> };
>
> @@ -1013,12 +1037,13 @@ static int me_kernel(struct page_state *ps, struct page *p)
>
> /*
> * Page in unknown state. Do nothing.
> + * This is a catch-all in case we fail to make sense of the page state.
> */
> static int me_unknown(struct page_state *ps, struct page *p)
> {
> pr_err("%#lx: Unknown page state\n", page_to_pfn(p));
> unlock_page(p);
> - return MF_FAILED;
> + return MF_IGNORED;
> }
>
> /*
> @@ -2055,6 +2080,8 @@ static int try_memory_failure_hugetlb(unsigned long pfn, int flags, int *hugetlb
> 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;
We might reuse the below "return res;"?
> }
> return res;
Besides from the above possible nits, this patch looks good to me.
Acked-by: Miaohe Lin <linmiaohe@huawei.com>
Thanks.
.
On 5/22/2024 7:31 PM, Miaohe Lin wrote:
> [..]
>> +/*
>> + * MF_IGNORED - The m-f() handler marks the page as PG_hwpoisoned'ed.
>> + * But it could not do more to isolate the page from being accessed again,
>> + * nor does it kill the process. This is extremely rare and one of the
>> + * potential causes is that the page state has been changed due to
>> + * underlying race condition. This is the most severe outcomes.
>> + *
>> + * MF_FAILED - The m-f() handler marks the page as PG_hwpoisoned'ed. It
>> + * should have killed the process, but it can't isolate the page, due to
>> + * conditions such as extra pin, unmap failure, etc. Accessing the page
>> + * again will trigger another MCE and the process will be killed by the
>> + * m-f() handler immediately.
>> + *
>> + * MF_DELAYED - The m-f() handler marks the page as PG_hwpoisoned'ed. The
>> + * page is unmapped, but perhaps remains in LRU or file mapping. An attempt
> Would the page remain in LRU or file mapping? IIUC, MF_DELAYED is returned from two functions:
> 1. me_swapcache_dirty. Page lives in swap cache and removed from LRU.
> 2. kvm_gmem_error_folio. Page range is unmapped. It seems page won't be in the LRU or page cache.
> Or am I miss something?
Agreed, I'll fix the comment.
>> + * to access the page again will trigger page fault and the PF handler
>> + * will kill the process.
>> + *
>> + * MF_RECOVERED - The m-f() handler marks the page as PG_hwpoisoned'ed.
>> + * The page has been completely isolated, that is, unmapped, taken out of
>> + * the buddy system, or hole-punnched out of the file mapping.
>> + */
>> static const char *action_name[] = {
>> [MF_IGNORED] = "Ignored",
>> [MF_FAILED] = "Failed",
>> @@ -893,6 +915,7 @@ static const char * const action_page_types[] = {
>> [MF_MSG_DIFFERENT_COMPOUND] = "different compound page after locking",
>> [MF_MSG_HUGE] = "huge page",
>> [MF_MSG_FREE_HUGE] = "free huge page",
>> + [MF_MSG_GET_HWPOISON] = "get hwpoison page",
>> [MF_MSG_UNMAP_FAILED] = "unmapping failed page",
>> [MF_MSG_DIRTY_SWAPCACHE] = "dirty swapcache page",
>> [MF_MSG_CLEAN_SWAPCACHE] = "clean swapcache page",
>> @@ -906,6 +929,7 @@ static const char * const action_page_types[] = {
>> [MF_MSG_BUDDY] = "free buddy page",
>> [MF_MSG_DAX] = "dax page",
>> [MF_MSG_UNSPLIT_THP] = "unsplit thp",
>> + [MF_MSG_ALREADY_POISONED] = "already poisoned",
>> [MF_MSG_UNKNOWN] = "unknown page",
>> };
>>
>> @@ -1013,12 +1037,13 @@ static int me_kernel(struct page_state *ps, struct page *p)
>>
>> /*
>> * Page in unknown state. Do nothing.
>> + * This is a catch-all in case we fail to make sense of the page state.
>> */
>> static int me_unknown(struct page_state *ps, struct page *p)
>> {
>> pr_err("%#lx: Unknown page state\n", page_to_pfn(p));
>> unlock_page(p);
>> - return MF_FAILED;
>> + return MF_IGNORED;
>> }
>>
>> /*
>> @@ -2055,6 +2080,8 @@ static int try_memory_failure_hugetlb(unsigned long pfn, int flags, int *hugetlb
>> 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;
> We might reuse the below "return res;"?
Yes, will fix.
>> }
>> return res;
> Besides from the above possible nits, this patch looks good to me.
> Acked-by: Miaohe Lin <linmiaohe@huawei.com>
> Thanks.
> .
Thanks!
-jane
On Tue, May 21, 2024 at 05:54:27PM -0600, Jane Chu wrote: > Added two explicit MF_MSG messages describing failure in get_hwpoison_page. > Attemped to document the definition of various action names, and made a few > adjustment to the action_result() calls. > > Signed-off-by: Jane Chu <jane.chu@oracle.com> This looks much better, thanks: Reviewed-by: Oscar Salvador <osalvador@suse.de> By the way, I was checking the block in memory_failure() that handles refcount=0 pages, concretely the piece of code that handles buddy pages. In there, if we fail to take the page off the buddy lists, we return MF_FAILED, but I really think we should be returning MF_IGNORED. Thoughts? -- Oscar Salvador SUSE Labs
On 5/22/2024 1:37 PM, Oscar Salvador wrote: > On Tue, May 21, 2024 at 05:54:27PM -0600, Jane Chu wrote: >> Added two explicit MF_MSG messages describing failure in get_hwpoison_page. >> Attemped to document the definition of various action names, and made a few >> adjustment to the action_result() calls. >> >> Signed-off-by: Jane Chu <jane.chu@oracle.com> > This looks much better, thanks: > > Reviewed-by: Oscar Salvador <osalvador@suse.de> > > By the way, I was checking the block in memory_failure() that handles > refcount=0 pages, concretely the piece of code that handles buddy pages. > > In there, if we fail to take the page off the buddy lists, we return > MF_FAILED, but I really think we should be returning MF_IGNORED. I guess you mean this code - if (has_extra_refcount(ps, p, false)) ret = MF_FAILED; ? It appears in below code paths- hwpoison_user_mappings identify_page_state me_huge_page || me_swapcache_dirty || me_swapcache_clean for LRU pages. And for non-LRU if (!folio_test_lru(folio) && !folio_test_writeback(folio)) goto identify_page_state; My hunch is that the most common calling path would be: hwpoison_user_mappings has unmapped the page, then identify_page_state is called, but for some reason failed to take the page off the LRU. The m-f() handler has isolated the page to avoid further MCE, so I think in general return MF_FAILED is okay. That said, the line is not always clear, for example in the non-LRU case, where the m-f() handler may have done only a little, I guess I just need to let the case rest. thanks, -jane > > Thoughts? > >
© 2016 - 2026 Red Hat, Inc.