Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 7 Oct 2021 12:06:34 -0400
From:   Peter Xu <peterx@redhat.com>
To:     Yang Shi <shy828301@gmail.com>
Cc:     HORIGUCHI =?utf-8?B?TkFPWUEo5aCA5Y+jIOebtOS5nyk=?= 
        <naoya.horiguchi@nec.com>, Hugh Dickins <hughd@google.com>,
        "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
        Matthew Wilcox <willy@infradead.org>,
        Oscar Salvador <osalvador@suse.de>,
        Andrew Morton <akpm@linux-foundation.org>,
        Linux MM <linux-mm@kvack.org>,
        Linux FS-devel Mailing List <linux-fsdevel@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [v3 PATCH 2/5] mm: filemap: check if THP has hwpoisoned subpage
 for PMD page fault
Message-ID: <YV8bChbXop3FuwPC@t490s>
References: <20210930215311.240774-1-shy828301@gmail.com>
 <20210930215311.240774-3-shy828301@gmail.com>
 <YV4Dz3y4NXhtqd6V@t490s>
 <CAHbLzkp8oO9qvDN66_ALOqNrUDrzHH7RZc3G5GQ1pxz8qXJjqw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <CAHbLzkp8oO9qvDN66_ALOqNrUDrzHH7RZc3G5GQ1pxz8qXJjqw@mail.gmail.com>
Precedence: bulk

On Wed, Oct 06, 2021 at 04:57:38PM -0700, Yang Shi wrote:
> > For example, I see that both unpoison_memory() and soft_offline_page() will
> > call it too, does it mean that we'll also set the bits e.g. even when we want
> > to inject an unpoison event too?
> 
> unpoison_memory() should be not a problem since it will just bail out
> once THP is met as the comment says:
> 
> /*
> * unpoison_memory() can encounter thp only when the thp is being
> * worked by memory_failure() and the page lock is not held yet.
> * In such case, we yield to memory_failure() and make unpoison fail.
> */

But I still think setting the subpage-hwpoison bit hides too deep there, it'll
be great we can keep get_hwpoison_page() as simple as a safe version of getting
the refcount of the page we want.  Or we'd still better touch up the comment
above get_hwpoison_page() to show that side effect.

> 
> 
> And I think we should set the flag for soft offline too, right? The

I'm not familiar with either memory failure or soft offline, so far it looks
right to me.  However..

> soft offline does set the hwpoison flag for the corrupted sub page and
> doesn't split file THP,

.. I believe this will become not true after your patch 5, right?

> so it should be captured by page fault as well. And yes for poison injection.

One more thing: besides thp split and page free, do we need to conditionally
drop the HasHwpoisoned bit when received an unpoison event?

If my understanding is correct, we may need to scan all the subpages there, to
make sure HasHwpoisoned bit reflects the latest status for the thp in question.

> 
> But your comment reminds me that get_hwpoison_page() is just called
> when !MF_COUNT_INCREASED, so it means MADV_HWPOISON still could
> escape. This needs to be covered too.

Right, maybe that's also a clue that we shouldn't set the new page flag within
get_hwpoison_page(), since get_hwpoison_page() is actually well coupled with
MF_COUNT_INCREASED and all of them are only about refcounting of the pages.

-- 
Peter Xu