On Mon, Mar 27, 2023 at 2:15 PM Jiaqi Yan <jiaqiyan@xxxxxxxxxx> 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@xxxxxxxxxx>
Reviewed-by: Yang Shi <shy828301@xxxxxxxxx>
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.
> + 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
>
