| Message ID | 20240510062602.901510-4-jane.chu@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Enhance soft hwpoison handling and injection | expand |
On Fri, May 10, 2024 at 12:26:00AM -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> ... > +/* > + * MF_IGNORED - Either the m-f() handler did nothing to recover, or it did "or if it " > + * something, then caught in a race condition which renders the effort sort "it was caught" I would also add to MF_IGNORED that we mark the page hwpoisoned anyway. > @@ -1018,7 +1034,7 @@ 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; > } I was confused because I saw you replaced all MF_MSG_UNKNOWN, so I wondered how we can end up here until I saw this is a catch-all in case we fail to make sense of the page->flags. While you are improving this, I would suggest to add a little comment above the function explaining how we can reach it. > /* > @@ -2055,6 +2071,7 @@ 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); > + return action_result(pfn, MF_MSG_ALREADY_POISONED, MF_FAILED); I do not understand why are you doing this. First of all, why is this considered a failure? We did not fail this time, did we? We went right ahead and kill the process which was re-accessing the hwpoisoned page. Is that considered a failure? Second, you are know supressing -EHWPOISON with whatever action_result() will gives us, which judging from the actual code would be -EBUSY? I do not think that that is right, and we should be returning -EHWPOISON. Or whatever error code kill_accessing_process() gives us. > @@ -2231,6 +2248,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); This is not coherent with what you did in try_memory_failure_hugetlb for MF_MSG_ALREADY_POISONED, I __think__ that in there we should be doing the same as we do here.
On 5/16/2024 2:46 AM, Oscar Salvador wrote: > On Fri, May 10, 2024 at 12:26:00AM -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> > ... >> +/* >> + * MF_IGNORED - Either the m-f() handler did nothing to recover, or it did > "or if it" >> + * something, then caught in a race condition which renders the effort sort > "it was caught" > > I would also add to MF_IGNORED that we mark the page hwpoisoned anyway. Thanks! Will fix, and add comments. > >> @@ -1018,7 +1034,7 @@ 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; >> } > I was confused because I saw you replaced all MF_MSG_UNKNOWN, so I > wondered how we can end up here until I saw this is a catch-all in case > we fail to make sense of the page->flags. > While you are improving this, I would suggest to add a little comment > above the function explaining how we can reach it. Yes, it's a catch-all versus something that suppose to happen, will add comments. > >> /* >> @@ -2055,6 +2071,7 @@ 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); >> + return action_result(pfn, MF_MSG_ALREADY_POISONED, MF_FAILED); > I do not understand why are you doing this. > > First of all, why is this considered a failure? We did not fail this > time, did we? We went right ahead and kill the process which was > re-accessing the hwpoisoned page. Is that considered a failure? Given that the goal for the m-f() handler is to isolate the poisoned page, so in this case, even if the kernel has killed the page, nothing else could be done to prevent the page from being accessed and triggering another MCE, which is, I suppose more sever than triggering a page fault. > > Second, you are know supressing -EHWPOISON with whatever action_result() > will gives us, which judging from the actual code would be -EBUSY? > I do not think that that is right, and we should be returning > -EHWPOISON. Or whatever error code kill_accessing_process() gives us. > > >> @@ -2231,6 +2248,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); > This is not coherent with what you did in try_memory_failure_hugetlb > for MF_MSG_ALREADY_POISONED, I __think__ that in there we should be > doing the same as we do here. > > Good catch, thanks. In both cases, 'res' from kill_accessing_process() should be returned. Thanks! -jane >
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..62133c10fb51 100644 --- a/mm/memory-failure.c +++ b/mm/memory-failure.c @@ -879,6 +879,20 @@ static int kill_accessing_process(struct task_struct *p, unsigned long pfn, return ret > 0 ? -EHWPOISON : -EFAULT; } +/* + * MF_IGNORED - Either the m-f() handler did nothing to recover, or it did + * something, then caught in a race condition which renders the effort sort + * of moot. This is perhaps the most sever. + * MF_FAILED - The m-f() handler might have killed the process, but it can't + * do much to the poisoned page other than marking it poisoned, due to + * conditions such as extra pin, unmap failure, etc. + * MF_DELAYED - The poisoned page has been unmapped but not completely isolated, + * such as page might remain in LRU. But an attempt by userspace process + * to access the page again will hit page fault which will kill the process. + * MF_RECOVERED - The poisoned page has been completely isolated, via unmap, + * taking the page out of the buddy system, or hole punching out of the + * mapping. + */ static const char *action_name[] = { [MF_IGNORED] = "Ignored", [MF_FAILED] = "Failed", @@ -893,6 +907,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 +921,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", }; @@ -1018,7 +1034,7 @@ 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 +2071,7 @@ 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); + return action_result(pfn, MF_MSG_ALREADY_POISONED, MF_FAILED); } return res; } else if (res == -EBUSY) { @@ -2062,7 +2079,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 +2114,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 +2248,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 +2285,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 +2381,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; }
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 | 28 +++++++++++++++++++++++----- 3 files changed, 27 insertions(+), 5 deletions(-)