Re: File THP and HWPoison

["Kirill A. Shutemov" <kirill@shutemov.name>]

On Tue, Mar 16, 2021 at 02:09:47PM +0000, Matthew Wilcox wrote:
> If we get a memory failure in the middle of a file THP, I think we handle
> it poorly.
> 
> int memory_failure(unsigned long pfn, int flags)
> ...
>         if (TestSetPageHWPoison(p)) {
> ...
>         orig_head = hpage = compound_head(p);
> ...
>         if (PageTransHuge(hpage)) {
>                 if (try_to_split_thp_page(p, "Memory Failure") < 0) {
>                         action_result(pfn, MF_MSG_UNSPLIT_THP, MF_IGNORED);
>                         return -EBUSY;
>                 }
> 
> static int try_to_split_thp_page(struct page *page, const char *msg)
> {
>         lock_page(page);
>         if (!PageAnon(page) || unlikely(split_huge_page(page))) {
>                 unsigned long pfn = page_to_pfn(page);
> 
>                 unlock_page(page);
>                 if (!PageAnon(page))
>                         pr_info("%s: %#lx: non anonymous thp\n", msg, pfn);
>                 else
>                         pr_info("%s: %#lx: thp split failed\n", msg, pfn);
>                 put_page(page);
>                 return -EBUSY;
> 
> So (for some reason) we don't even try to split a file THP.  But then,
> if we take a page fault on a file THP:
> 
> static struct page *next_uptodate_page(struct page *page,
> ...
>                 if (PageHWPoison(page))
>                         goto skip;
> (... but we're only testing the head page here, which isn't necessarily
> the one which got the error ...)
> 
>         if (pmd_none(*vmf->pmd) && PageTransHuge(page)) {
>             vm_fault_t ret = do_set_pmd(vmf, page);
> 
> So we now map the PMD-sized page into userspace, even though it has a
> HWPoison in it.
> 
> I think there are two things that we should be doing:
> 
> 1. Attempt to split THPs which are file-backed.  That makes most of this
> problem disappear because there won't be THPs with HWPoison, mostly.

+Naoya. Could you give more context here?

> 2. When the THP fails to split, use a spare page flag to indicate that
> the THP contains a HWPoison bit in one of its subpages.  There are a
> lot of PF_SECOND flags available for this purpose.
> 
> but I know almost nothing about the memory-failure subsystem and I'm
> still learning all the complexities of THPs, so it's entirely possible
> I've overlooked something important.

I wounder if it would be cleaner to switch PG_hwpoison to PF_HEAD: if
split failed we posion whole compound page. Yes, we will waste more
memory, but it makes it much cleaner for user: just check if the page is
poisoned.

-- 
 Kirill A. Shutemov