Re: File THP and HWPoison

On Thu, Mar 18, 2021 at 05:08:43PM +0300, Kirill A. Shutemov wrote:

On Tue, Mar 16, 2021 at 02:09:47PM +0000, Matthew Wilcox wrote:

quoted

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?

I did some git archaeology and found this check was introduced in
7f6bf39bbdd1 ("mm/hwpoison: fix panic due to split huge zero page") where
it wasn't intended to catch _file_ pages at all, but the zero page.
I suspect that nobody thought to look at this when introducing THP
for shmem.

quoted

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.

I think that's a poor quality implementation ... it'd cause processes
to die that weren't even touching the page that had hwpoison.  Using
a PF_SECOND bit lets us do the check as cheaply as if we made hwpoison
PF_HEAD.