Re: File THP and HWPoison

On Thu, Mar 18, 2021 at 02:57:16PM +0000, Matthew Wilcox wrote:

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

quoted

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?

Recently, I tried to address the problem on
https://lore.kernel.org/linux-mm/20210209062128.453814-1-nao.horiguchi@gmail.com/ (local)
but the patch was found incorrect because the related page table entries disappeared
after split_huge_page() succeeded.  I thought I'm going to study more, but
didn't make it this week because I looked at other review requests.

A pmd mapping for anonymous thp is replaced with 512 pte mappings by
split_huge_page(), so I'm wondering why we don't do the same for shmem thp.

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.

Yes, 7f6bf39bbdd1 was worked before thp page cache, so we did not consider
it at that time.

Thanks,
Naoya Horiguchi

quoted

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.