On Mon, Oct 10, 2022 at 09:01:41AM -0700, Jiaqi Yan wrote:
> Make __collapse_huge_page_copy return whether
> collapsing/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 does the following to
> roll back the page table and page states:
> 1) re-establish the original PTEs-to-PMD connection.
> 2) release source pages back to their LRU list.
>
> Signed-off-by: Jiaqi Yan <jiaqiyan@xxxxxxxxxx>
> ---
> include/linux/highmem.h | 19 +++++
> include/trace/events/huge_memory.h | 3 +-
> mm/khugepaged.c | 130 ++++++++++++++++++++++-------
> 3 files changed, 121 insertions(+), 31 deletions(-)
>
> diff --git a/include/linux/highmem.h b/include/linux/highmem.h
> index 25679035ca283..91a65bdabcb33 100644
> --- a/include/linux/highmem.h
> +++ b/include/linux/highmem.h
> @@ -332,6 +332,25 @@ static inline void copy_highpage(struct page *to, struct page *from)
>
> #endif
>
> +/*
> + * Machine check exception handled version of copy_highpage.
> + * Return true if copying page content failed; otherwise false.
> + * Note handling #MC requires arch opt-in.
> + */
> +static inline bool copy_highpage_mc(struct page *to, struct page *from)
> +{
> + char *vfrom, *vto;
> + unsigned long ret;
> +
> + vfrom = kmap_local_page(from);
> + vto = kmap_local_page(to);
> + ret = copy_mc_to_kernel(vto, vfrom, PAGE_SIZE);
> + kunmap_local(vto);
> + kunmap_local(vfrom);
> +
> + return ret > 0;
> +}
> +
> static inline void memcpy_page(struct page *dst_page, size_t dst_off,
> struct page *src_page, size_t src_off,
> size_t len)
> diff --git a/include/trace/events/huge_memory.h b/include/trace/events/huge_memory.h
> index d651f3437367d..756e991366639 100644
> --- a/include/trace/events/huge_memory.h
> +++ b/include/trace/events/huge_memory.h
> @@ -33,7 +33,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 70b7ac66411c0..552e7cb4c8b42 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -51,6 +51,7 @@ enum scan_result {
> SCAN_CGROUP_CHARGE_FAIL,
> SCAN_TRUNCATED,
> SCAN_PAGE_HAS_PRIVATE,
> + SCAN_COPY_MC,
> };
>
> #define CREATE_TRACE_POINTS
> @@ -673,44 +674,99 @@ static int __collapse_huge_page_isolate(struct vm_area_struct *vma,
> return 0;
> }
>
> -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)
> +/*
> + * __collapse_huge_page_copy - attempts to copy memory contents from normal
> + * pages to a hugepage. Cleanup the normal pages if copying succeeds;
> + * otherwise restore the original page table and release isolated normal pages.
> + * Returns true if copying succeeds, otherwise returns false.
> + *
> + * @pte: starting of the PTEs to copy from
> + * @page: the new hugepage to copy contents to
> + * @pmd: pointer to the new hugepage's PMD
> + * @rollback: the original normal pages' PMD
> + * @vma: the original normal pages' virtual memory area
> + * @address: starting address to copy
> + * @pte_ptl: lock on normal pages' PTEs
> + * @compound_pagelist: list that stores compound pages
> + */
> +static bool __collapse_huge_page_copy(pte_t *pte,
> + struct page *page,
> + pmd_t *pmd,
> + pmd_t rollback,
> + struct vm_area_struct *vma,
> + unsigned long address,
> + spinlock_t *pte_ptl,
> + struct list_head *compound_pagelist)
> {
> struct page *src_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;
> + unsigned long _address;
> + spinlock_t *pmd_ptl;
> + bool copy_succeeded = true;
>
> + /*
> + * 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);
> + else {
> + src_page = pte_page(pteval);
> + if (copy_highpage_mc(page, src_page)) {
> + copy_succeeded = false;
> + break;
> + }
> + }
> + }
> +
> + if (!copy_succeeded) {
> + /*
> + * Copying failed, re-establish the regular PMD that points to
> + * the regular page table. 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(rollback));
> + spin_unlock(pmd_ptl);
> + }
Initially I expected return here, but you handle copy_succeeded below. Hm.
> +
> + for (_pte = pte, _address = address; _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))) {
> - /*
> - * ptl mostly unnecessary.
> - */
> - spin_lock(ptl);
> - ptep_clear(vma->vm_mm, address, _pte);
> - spin_unlock(ptl);
> + if (copy_succeeded) {
> + add_mm_counter(vma->vm_mm, MM_ANONPAGES, 1);
> + if (is_zero_pfn(pte_pfn(pteval))) {
> + /*
> + * pte_ptl mostly unnecessary.
> + */
> + spin_lock(pte_ptl);
> + pte_clear(vma->vm_mm, _address, _pte);
> + spin_unlock(pte_ptl);
> + }
> }
So this branch is NOP if !copy_succeeded, right?
> } else {
> src_page = pte_page(pteval);
> - copy_user_highpage(page, src_page, address, vma);
> if (!PageCompound(src_page))
> release_pte_page(src_page);
And this branch only calls release_pte_page() (Which I believe would screw
up statistic).
Looks very broken. Or just hard to follow. Or both.
Please consider rework code more to streamline handling !copy_succeeded
situation.
> - /*
> - * ptl mostly unnecessary, but preempt has to
> - * be disabled to update the per-cpu stats
> - * inside page_remove_rmap().
> - */
> - spin_lock(ptl);
> - ptep_clear(vma->vm_mm, address, _pte);
> - page_remove_rmap(src_page, vma, false);
> - spin_unlock(ptl);
> - free_page_and_swap_cache(src_page);
> + if (copy_succeeded) {
> + /*
> + * pte_ptl mostly unnecessary, but preempt
> + * has to be disabled to update the per-cpu
> + * stats inside page_remove_rmap().
> + */
> + spin_lock(pte_ptl);
> + ptep_clear(vma->vm_mm, _address, _pte);
> + page_remove_rmap(src_page, vma, false);
> + spin_unlock(pte_ptl);
> + free_page_and_swap_cache(src_page);
> + }
> }
> }
>
> @@ -723,6 +779,8 @@ static void __collapse_huge_page_copy(pte_t *pte, struct page *page,
> free_swap_cache(src_page);
> putback_lru_page(src_page);
> }
> +
> + return copy_succeeded;
> }
>
> static void khugepaged_alloc_sleep(void)
> @@ -1009,6 +1067,7 @@ static void collapse_huge_page(struct mm_struct *mm,
> struct vm_area_struct *vma;
> struct mmu_notifier_range range;
> gfp_t gfp;
> + bool copied = false;
>
> VM_BUG_ON(address & ~HPAGE_PMD_MASK);
>
> @@ -1121,9 +1180,13 @@ static void collapse_huge_page(struct mm_struct *mm,
> */
> anon_vma_unlock_write(vma->anon_vma);
>
> - __collapse_huge_page_copy(pte, new_page, vma, address, pte_ptl,
> - &compound_pagelist);
> + copied = __collapse_huge_page_copy(pte, new_page, pmd, _pmd,
> + vma, address, pte_ptl, &compound_pagelist);
> pte_unmap(pte);
> + if (!copied) {
> + result = SCAN_COPY_MC;
> + goto out_up_write;
> + }
> /*
> * spin_lock() below is not the equivalent of smp_wmb(), but
> * the smp_wmb() inside __SetPageUptodate() can be reused to
> @@ -2129,6 +2192,13 @@ static unsigned int khugepaged_scan_mm_slot(unsigned int pages,
> khugepaged_scan_file(mm, file, pgoff, hpage);
> fput(file);
> } else {
> + /*
> + * mmap_read_lock is
> + * 1) still held if scan failed;
> + * 2) released if scan succeeded.
> + * It is not affected by collapsing or copying
> + * operations.
> + */
> ret = khugepaged_scan_pmd(mm, vma,
> khugepaged_scan.address,
> hpage);
> --
> 2.38.0.rc1.362.ged0d419d3c-goog
>
>
--
Kiryl Shutsemau / Kirill A. Shutemov
