Re: [RFC] Missing compound_head() in memory-failure

[Matthew Wilcox <willy@infradead.org>]

On Tue, Feb 01, 2022 at 05:40:57PM +0000, Joao Martins wrote:
> On 2/1/22 16:01, Joao Martins wrote:
> > On 2/1/22 15:46, Matthew Wilcox wrote:
> >> On Mon, Jan 31, 2022 at 08:54:39PM +0000, Joao Martins wrote:
> >>> On 1/31/22 20:29, Matthew Wilcox wrote:
> >>>> Unless I am mistaken, you have to pass the compound head of the page
> >>>> which has the error to collect_procs().  Am I mistaken?
> >>>>
> >>> -rc2 already has a fix for it:
> >>>
> >>> https://lore.kernel.org/linux-mm/20220129021420.PgBIZm-q9%25akpm@linux-foundation.org/
> >>>
> >>> Earlier in that function there's a:
> >>>
> >>> 	page = compound_head(page);
> >>>
> >>> So the @page passed to collect_procs() already is a head page.
> >>
> >> It's wrong though ;-(  You set the HWPoison bit on the page after
> >> calling compound_head(), so you set the bit on the head page instead
> >> of the precise page that had the poison.
> >>
> > Considering that on device-dax we would unmap the whole 2M page regardless
> > of the poisoned subpage isn't that actually representative still?
> > 
> 
> To say this another way. We do set the HWPoison on the head page,
> and not the subpage as you say, but we end up propagating the resultant
> MCE action on the superset of pages in the whole PMD or PUD.
> 
> What I was trying to say in perhaps a convoluted way is that device-dax
> case isn't different than HugeTLB that only wants to poison head. If there's
> a head page, there's likely a PMD or PUD populated (depending what the device
> was onlined with) and thus that's what gets unmapped. There's no idea of
> subpages being treated any differently, at least as far as device-dax is
> concerned -- unless I miss auditing some other code path.

Using HugeTLB as a model is not a good idea.  THP is the model to
aim for; one can choose to map pages askew (ie not aligned with a
PMD), and I don't think you'll find all the mappings with the current
code (eg if someone has mapped a single page of the hugepage).

> fsdax IIUC seems to rely more on the subpage bit being flagged but no
> functional change here for fsdax as there's only base pages there (no heads).
> 
> >> I'm fixing this up as part of the folio patches, but you may wish to
> >> fix it earlier than that.
> (+Dan in case I misrepresented or missed something)
> 
> Should we deem it a problem, I'll fix for the next -rc.
> Just in case, here's the diff stashed:
> 
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 2e2f740c63dc..661c23df8115 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1577,7 +1577,7 @@ static int memory_failure_hugetlb(unsigned long pfn, int flags)
>  static int memory_failure_dev_pagemap(unsigned long pfn, int flags,
>                 struct dev_pagemap *pgmap)
>  {
> -       struct page *page = pfn_to_page(pfn);
> +       struct page *page = pfn_to_page(pfn), *subpage = page;
>         unsigned long size = 0;
>         struct to_kill *tk;
>         LIST_HEAD(tokill);
> @@ -1631,7 +1631,7 @@ static int memory_failure_dev_pagemap(unsigned long pfn, int flags,
>          * Use this flag as an indication that the dax page has been
>          * remapped UC to prevent speculative consumption of poison.
>          */
> -       SetPageHWPoison(page);
> +       SetPageHWPoison(subpage);
> 
>         /*
>          * Unlike System-RAM there is no possibility to swap in a