| Message ID | 20211210110208.879740-1-naoya.horiguchi@linux.dev (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] mm, hwpoison: fix condition in free hugetlb page path | expand |
On 12/10/21 03:02, Naoya Horiguchi wrote: > From: Naoya Horiguchi <naoya.horiguchi@nec.com> > > When a memory error hits a tail page of a free hugepage, > __page_handle_poison() is expected to be called to isolate the error in > 4kB unit, but it's not called due to the outdated if-condition in > memory_failure_hugetlb(). This loses the chance to isolate the error in > the finer unit, so it's not optimal. Drop the condition. > > This "(p != head && TestSetPageHWPoison(head)" condition is based on the > old semantics of PageHWPoison on hugepage (where PG_hwpoison flag was > set on the subpage), so it's not necessray any more. By getting to set > PG_hwpoison on head page for hugepages, concurrent error events on > different subpages in a single hugepage can be prevented by > TestSetPageHWPoison(head) at the beginning of memory_failure_hugetlb(). > So dropping the condition should not reopen the race window originally > mentioned in commit b985194c8c0a ("hwpoison, hugetlb: > lock_page/unlock_page does not match for handling a free hugepage") > > Reported-by: Fei Luo <luofei@unicloud.com> > Signed-off-by: Naoya Horiguchi <naoya.horiguchi@nec.com> > Cc: <stable@vger.kernel.org> # v5.14+ > --- > I set v5.14+ for stable trees because the base code was greatly changed > by commit 0ed950d1f281 ("mm,hwpoison: make get_hwpoison_page() call > get_any_page()"), and this patch is not cleanly applicable, although the > original issue was introduced more previously. > --- > mm/memory-failure.c | 21 +++++++-------------- > 1 file changed, 7 insertions(+), 14 deletions(-) Thank you Naoya! In my spare time, I have been testing memory error code for hugetlb pages. I noticed this issue and had created similar patch. Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
On Fri, Dec 10, 2021 at 08:02:08PM +0900, Naoya Horiguchi wrote: > From: Naoya Horiguchi <naoya.horiguchi@nec.com> > > When a memory error hits a tail page of a free hugepage, > __page_handle_poison() is expected to be called to isolate the error in > 4kB unit, but it's not called due to the outdated if-condition in > memory_failure_hugetlb(). This loses the chance to isolate the error in > the finer unit, so it's not optimal. Drop the condition. > > This "(p != head && TestSetPageHWPoison(head)" condition is based on the > old semantics of PageHWPoison on hugepage (where PG_hwpoison flag was > set on the subpage), so it's not necessray any more. By getting to set > PG_hwpoison on head page for hugepages, concurrent error events on > different subpages in a single hugepage can be prevented by > TestSetPageHWPoison(head) at the beginning of memory_failure_hugetlb(). > So dropping the condition should not reopen the race window originally > mentioned in commit b985194c8c0a ("hwpoison, hugetlb: > lock_page/unlock_page does not match for handling a free hugepage") > > Reported-by: Fei Luo <luofei@unicloud.com> > Signed-off-by: Naoya Horiguchi <naoya.horiguchi@nec.com> > Cc: <stable@vger.kernel.org> # v5.14+ > --- > I set v5.14+ for stable trees because the base code was greatly changed > by commit 0ed950d1f281 ("mm,hwpoison: make get_hwpoison_page() call > get_any_page()"), and this patch is not cleanly applicable, although the > original issue was introduced more previously. > --- > mm/memory-failure.c | 21 +++++++-------------- > 1 file changed, 7 insertions(+), 14 deletions(-) > > diff --git a/mm/memory-failure.c b/mm/memory-failure.c > index 8f0ee5b08696..68d9a35f8908 100644 > --- a/mm/memory-failure.c > +++ b/mm/memory-failure.c > @@ -1521,24 +1521,17 @@ static int memory_failure_hugetlb(unsigned long pfn, int flags) > if (!(flags & MF_COUNT_INCREASED)) { > res = get_hwpoison_page(p, flags); > if (!res) { > - /* > - * Check "filter hit" and "race with other subpage." > - */ > lock_page(head); > - if (PageHWPoison(head)) { > - if ((hwpoison_filter(p) && TestClearPageHWPoison(p)) > - || (p != head && TestSetPageHWPoison(head))) { > + if (hwpoison_filter(p)) { > + if (TestClearPageHWPoison(head)) > num_poisoned_pages_dec(); > - unlock_page(head); > - return 0; > - } > + unlock_page(head); > + return 0; > } > unlock_page(head); > - res = MF_FAILED; > - if (__page_handle_poison(p)) { > - page_ref_inc(p); > - res = MF_RECOVERED; > - } > + res = MF_RECOVERED; > + if (!page_handle_poison(p, true, false)) > + res = MF_FAILED; Sorry, I just found that this change broke "HardwareCorrupted" counter because page_handle_poison() calls num_poisoned_pages_inc() so the counter was incremented twice in total by a single error event. page_handle_poison() is supposed to be called only in soft-offline context, and __page_handle_poison() should be called in hard-offline context. Andrew, could you fold the following diff on mm-hwpoison-fix-condition-in-free-hugetlb-page-path.patch in your tree? (Or if desirable, I'll send a updated full patch.) diff --git a/mm/memory-failure.c b/mm/memory-failure.c index 68d9a35f8908..ee51d6410f9a 100644 --- a/mm/memory-failure.c +++ b/mm/memory-failure.c @@ -1529,9 +1529,11 @@ static int memory_failure_hugetlb(unsigned long pfn, int flags) return 0; } unlock_page(head); - res = MF_RECOVERED; - if (!page_handle_poison(p, true, false)) - res = MF_FAILED; + res = MF_FAILED; + if (__page_handle_poison(p)) { + page_ref_inc(p); + res = MF_RECOVERED; + } action_result(pfn, MF_MSG_FREE_HUGE, res); return res == MF_RECOVERED ? 0 : -EBUSY; } else if (res < 0) { Thanks, Naoya Horiguchi
diff --git a/mm/memory-failure.c b/mm/memory-failure.c index 8f0ee5b08696..68d9a35f8908 100644 --- a/mm/memory-failure.c +++ b/mm/memory-failure.c @@ -1521,24 +1521,17 @@ static int memory_failure_hugetlb(unsigned long pfn, int flags) if (!(flags & MF_COUNT_INCREASED)) { res = get_hwpoison_page(p, flags); if (!res) { - /* - * Check "filter hit" and "race with other subpage." - */ lock_page(head); - if (PageHWPoison(head)) { - if ((hwpoison_filter(p) && TestClearPageHWPoison(p)) - || (p != head && TestSetPageHWPoison(head))) { + if (hwpoison_filter(p)) { + if (TestClearPageHWPoison(head)) num_poisoned_pages_dec(); - unlock_page(head); - return 0; - } + unlock_page(head); + return 0; } unlock_page(head); - res = MF_FAILED; - if (__page_handle_poison(p)) { - page_ref_inc(p); - res = MF_RECOVERED; - } + res = MF_RECOVERED; + if (!page_handle_poison(p, true, false)) + res = MF_FAILED; action_result(pfn, MF_MSG_FREE_HUGE, res); return res == MF_RECOVERED ? 0 : -EBUSY; } else if (res < 0) {