On Tue, Nov 12, 2019 at 05:52:58PM +0530, Aneesh Kumar K.V wrote:
> Naoya Horiguchi <n-horiguchi@xxxxxxxxxxxxx> writes:
>
> > On Fri, Oct 18, 2019 at 01:48:32PM +0200, Michal Hocko wrote:
> >> On Thu 17-10-19 16:21:08, Oscar Salvador wrote:
> >> > From: Naoya Horiguchi <n-horiguchi@xxxxxxxxxxxxx>
> >> >
> >> > Drop the PageHuge check since memory_failure forks into memory_failure_hugetlb()
> >> > for hugetlb pages.
> >> >
> >> > Signed-off-by: Oscar Salvador <osalvador@xxxxxxx>
> >> > Signed-off-by: Naoya Horiguchi <n-horiguchi@xxxxxxxxxxxxx>
> >>
> >> s-o-b chain is reversed.
> >>
> >> The code is a bit confusing. Doesn't this check aim for THP?
> >
> > No, PageHuge() is false for thp, so this if branch is just dead code.
>
> memory_failure()
> {
>
> if (PageTransHuge(hpage)) {
> lock_page(p);
> if (!PageAnon(p) || unlikely(split_huge_page(p))) {
> unlock_page(p);
> if (!PageAnon(p))
> pr_err("Memory failure: %#lx: non anonymous thp\n",
> pfn);
> else
> pr_err("Memory failure: %#lx: thp split failed\n",
> pfn);
> if (TestClearPageHWPoison(p))
> num_poisoned_pages_dec();
> put_hwpoison_page(p);
> return -EBUSY;
> }
> unlock_page(p);
> VM_BUG_ON_PAGE(!page_count(p), p);
> hpage = compound_head(p);
> }
>
> }
>
> Do we need that hpage = compund_head(p) conversion there? We should just
> be able to say hpage = p, or even better after this change use p
> directly instead of hpage in the code following?
Thanks for the comment, the target page never be in compound_page
(without races leading to MF_MSG_DIFFERENT_COMPOUND path), so hpage
shouldn't be used afterward. We also have obsolete comment, so
I feel like the following changes:
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 392ac277b17d..c9df0f183d6c 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1319,7 +1319,6 @@ int memory_failure(unsigned long pfn, int flags)
}
unlock_page(p);
VM_BUG_ON_PAGE(!page_count(p), p);
- hpage = compound_head(p);
}
/*
@@ -1391,11 +1390,8 @@ int memory_failure(unsigned long pfn, int flags)
/*
* Now take care of user space mappings.
* Abort on fail: __delete_from_page_cache() assumes unmapped page.
- *
- * When the raw error page is thp tail page, hpage points to the raw
- * page after thp split.
*/
- if (!hwpoison_user_mappings(p, pfn, flags, &hpage)) {
+ if (!hwpoison_user_mappings(p, pfn, flags, &p)) {
action_result(pfn, MF_MSG_UNMAP_FAILED, MF_IGNORED);
res = -EBUSY;
goto out;
Thanks,
Naoya Horiguchi
