| Message ID | 20240521235429.2368017-4-jane.chu@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Enhance soft hwpoison handling and injection | expand |
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?
On 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 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? > >
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
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; }
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(-)