Re: [PATCH v11 1/3] mm/khugepaged: recover from poisoned anonymous memory

[Jiaqi Yan <jiaqiyan@google.com>]

On Tue, Mar 28, 2023 at 8:59 AM Yang Shi <shy828301@gmail.com> wrote:
>
> On Mon, Mar 27, 2023 at 2:15 PM Jiaqi Yan <jiaqiyan@google.com> wrote:
> >
> > Make __collapse_huge_page_copy return whether copying anonymous pages
> > succeeded, and make collapse_huge_page handle the return status.
> >
> > Break existing PTE scan loop into two for-loops. The first loop copies
> > source pages into target huge page, and can fail gracefully when running
> > into memory errors in source pages. If copying all pages succeeds, the
> > second loop releases and clears up these normal pages. Otherwise, the
> > second loop rolls back the page table and page states by:
> > - re-establishing the original PTEs-to-PMD connection.
> > - releasing source pages back to their LRU list.
> >
> > Tested manually:
> > 0. Enable khugepaged on system under test.
> > 1. Start a two-thread application. Each thread allocates a chunk of
> >    non-huge anonymous memory buffer.
> > 2. Pick 4 random buffer locations (2 in each thread) and inject
> >    uncorrectable memory errors at corresponding physical addresses.
> > 3. Signal both threads to make their memory buffer collapsible, i.e.
> >    calling madvise(MADV_HUGEPAGE).
> > 4. Wait and check kernel log: khugepaged is able to recover from poisoned
> >    pages and skips collapsing them.
> > 5. Signal both threads to inspect their buffer contents and make sure no
> >    data corruption.
> >
> > Signed-off-by: Jiaqi Yan <jiaqiyan@google.com>
>
> Reviewed-by: Yang Shi <shy828301@gmail.com>
>
> Just a nit below:
>
> > ---
> >  include/trace/events/huge_memory.h |   3 +-
> >  mm/khugepaged.c                    | 114 +++++++++++++++++++++++++----
> >  2 files changed, 103 insertions(+), 14 deletions(-)
> >
> > diff --git a/include/trace/events/huge_memory.h b/include/trace/events/huge_memory.h
> > index 3e6fb05852f9a..46cce509957ba 100644
> > --- a/include/trace/events/huge_memory.h
> > +++ b/include/trace/events/huge_memory.h
> > @@ -36,7 +36,8 @@
> >         EM( SCAN_ALLOC_HUGE_PAGE_FAIL,  "alloc_huge_page_failed")       \
> >         EM( SCAN_CGROUP_CHARGE_FAIL,    "ccgroup_charge_failed")        \
> >         EM( SCAN_TRUNCATED,             "truncated")                    \
> > -       EMe(SCAN_PAGE_HAS_PRIVATE,      "page_has_private")             \
> > +       EM( SCAN_PAGE_HAS_PRIVATE,      "page_has_private")             \
> > +       EMe(SCAN_COPY_MC,               "copy_poisoned_page")           \
> >
> >  #undef EM
> >  #undef EMe
> > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> > index bee7fd7db380a..bef68286345c8 100644
> > --- a/mm/khugepaged.c
> > +++ b/mm/khugepaged.c
> > @@ -55,6 +55,7 @@ enum scan_result {
> >         SCAN_CGROUP_CHARGE_FAIL,
> >         SCAN_TRUNCATED,
> >         SCAN_PAGE_HAS_PRIVATE,
> > +       SCAN_COPY_MC,
> >  };
> >
> >  #define CREATE_TRACE_POINTS
> > @@ -681,20 +682,22 @@ static int __collapse_huge_page_isolate(struct vm_area_struct *vma,
> >         return result;
> >  }
> >
> > -static void __collapse_huge_page_copy(pte_t *pte, struct page *page,
> > -                                     struct vm_area_struct *vma,
> > -                                     unsigned long address,
> > -                                     spinlock_t *ptl,
> > -                                     struct list_head *compound_pagelist)
> > +static void __collapse_huge_page_copy_succeeded(pte_t *pte,
> > +                                               pmd_t *pmd,
> > +                                               struct vm_area_struct *vma,
> > +                                               unsigned long address,
> > +                                               spinlock_t *ptl,
> > +                                               struct list_head *compound_pagelist)
> >  {
> > -       struct page *src_page, *tmp;
> > +       struct page *src_page;
> > +       struct page *tmp;
> >         pte_t *_pte;
> > -       for (_pte = pte; _pte < pte + HPAGE_PMD_NR;
> > -                               _pte++, page++, address += PAGE_SIZE) {
> > -               pte_t pteval = *_pte;
> > +       pte_t pteval;
> >
> > +       for (_pte = pte; _pte < pte + HPAGE_PMD_NR;
> > +            _pte++, address += PAGE_SIZE) {
> > +               pteval = *_pte;
> >                 if (pte_none(pteval) || is_zero_pfn(pte_pfn(pteval))) {
> > -                       clear_user_highpage(page, address);
> >                         add_mm_counter(vma->vm_mm, MM_ANONPAGES, 1);
> >                         if (is_zero_pfn(pte_pfn(pteval))) {
> >                                 /*
> > @@ -706,7 +709,6 @@ static void __collapse_huge_page_copy(pte_t *pte, struct page *page,
> >                         }
> >                 } else {
> >                         src_page = pte_page(pteval);
> > -                       copy_user_highpage(page, src_page, address, vma);
> >                         if (!PageCompound(src_page))
> >                                 release_pte_page(src_page);
> >                         /*
> > @@ -733,6 +735,88 @@ static void __collapse_huge_page_copy(pte_t *pte, struct page *page,
> >         }
> >  }
> >
> > +static void __collapse_huge_page_copy_failed(pte_t *pte,
> > +                                            pmd_t *pmd,
> > +                                            pmd_t orig_pmd,
> > +                                            struct vm_area_struct *vma,
> > +                                            unsigned long address,
>
> It looks like "address" is not used at all. It could be removed.

Thanks for catching this!

pmd in __collapse_huge_page_copy_succeeded can also be dropped.

>
> > +                                            struct list_head *compound_pagelist)
> > +{
> > +       spinlock_t *pmd_ptl;
> > +
> > +       /*
> > +        * Re-establish the PMD to point to the original page table
> > +        * entry. Restoring PMD needs to be done prior to releasing
> > +        * pages. Since pages are still isolated and locked here,
> > +        * acquiring anon_vma_lock_write is unnecessary.
> > +        */
> > +       pmd_ptl = pmd_lock(vma->vm_mm, pmd);
> > +       pmd_populate(vma->vm_mm, pmd, pmd_pgtable(orig_pmd));
> > +       spin_unlock(pmd_ptl);
> > +       /*
> > +        * Release both raw and compound pages isolated
> > +        * in __collapse_huge_page_isolate.
> > +        */
> > +       release_pte_pages(pte, pte + HPAGE_PMD_NR, compound_pagelist);
> > +}
> > +
> > +/*
> > + * __collapse_huge_page_copy - attempts to copy memory contents from raw
> > + * pages to a hugepage. Cleans up the raw pages if copying succeeds;
> > + * otherwise restores the original page table and releases isolated raw pages.
> > + * Returns SCAN_SUCCEED if copying succeeds, otherwise returns SCAN_COPY_MC.
> > + *
> > + * @pte: starting of the PTEs to copy from
> > + * @page: the new hugepage to copy contents to
> > + * @pmd: pointer to the new hugepage's PMD
> > + * @orig_pmd: the original raw pages' PMD
> > + * @vma: the original raw pages' virtual memory area
> > + * @address: starting address to copy
> > + * @pte_ptl: lock on raw pages' PTEs
> > + * @compound_pagelist: list that stores compound pages
> > + */
> > +static int __collapse_huge_page_copy(pte_t *pte,
> > +                                    struct page *page,
> > +                                    pmd_t *pmd,
> > +                                    pmd_t orig_pmd,
> > +                                    struct vm_area_struct *vma,
> > +                                    unsigned long address,
> > +                                    spinlock_t *pte_ptl,
> > +                                    struct list_head *compound_pagelist)
> > +{
> > +       struct page *src_page;
> > +       pte_t *_pte;
> > +       pte_t pteval;
> > +       unsigned long _address;
> > +       int result = SCAN_SUCCEED;
> > +
> > +       /*
> > +        * Copying pages' contents is subject to memory poison at any iteration.
> > +        */
> > +       for (_pte = pte, _address = address; _pte < pte + HPAGE_PMD_NR;
> > +            _pte++, page++, _address += PAGE_SIZE) {
> > +               pteval = *_pte;
> > +               if (pte_none(pteval) || is_zero_pfn(pte_pfn(pteval))) {
> > +                       clear_user_highpage(page, _address);
> > +                       continue;
> > +               }
> > +               src_page = pte_page(pteval);
> > +               if (copy_mc_user_highpage(page, src_page, _address, vma) > 0) {
> > +                       result = SCAN_COPY_MC;
> > +                       break;
> > +               }
> > +       }
> > +
> > +       if (likely(result == SCAN_SUCCEED))
> > +               __collapse_huge_page_copy_succeeded(pte, pmd, vma, address,
> > +                                                   pte_ptl, compound_pagelist);
> > +       else
> > +               __collapse_huge_page_copy_failed(pte, pmd, orig_pmd, vma,
> > +                                                address, compound_pagelist);
> > +
> > +       return result;
> > +}
> > +
> >  static void khugepaged_alloc_sleep(void)
> >  {
> >         DEFINE_WAIT(wait);
> > @@ -1106,9 +1190,13 @@ static int collapse_huge_page(struct mm_struct *mm, unsigned long address,
> >          */
> >         anon_vma_unlock_write(vma->anon_vma);
> >
> > -       __collapse_huge_page_copy(pte, hpage, vma, address, pte_ptl,
> > -                                 &compound_pagelist);
> > +       result = __collapse_huge_page_copy(pte, hpage, pmd, _pmd,
> > +                                          vma, address, pte_ptl,
> > +                                          &compound_pagelist);
> >         pte_unmap(pte);
> > +       if (unlikely(result != SCAN_SUCCEED))
> > +               goto out_up_write;
> > +
> >         /*
> >          * spin_lock() below is not the equivalent of smp_wmb(), but
> >          * the smp_wmb() inside __SetPageUptodate() can be reused to
> > --
> > 2.40.0.348.gf938b09366-goog
> >