From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 97A05C433F5 for ; Fri, 25 Mar 2022 02:20:52 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id E616B8D0001; Thu, 24 Mar 2022 22:20:51 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id E11756B0073; Thu, 24 Mar 2022 22:20:51 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C8B5E8D0001; Thu, 24 Mar 2022 22:20:51 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0056.hostedemail.com [216.40.44.56]) by kanga.kvack.org (Postfix) with ESMTP id B2F2C6B0071 for ; Thu, 24 Mar 2022 22:20:51 -0400 (EDT) Received: from smtpin22.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 66AF71828AE40 for ; Fri, 25 Mar 2022 02:20:51 +0000 (UTC) X-FDA: 79281305502.22.2736DEB Received: from mail-pf1-f170.google.com (mail-pf1-f170.google.com [209.85.210.170]) by imf23.hostedemail.com (Postfix) with ESMTP id CF319140005 for ; Fri, 25 Mar 2022 02:20:50 +0000 (UTC) Received: by mail-pf1-f170.google.com with SMTP id z16so5378772pfh.3 for ; Thu, 24 Mar 2022 19:20:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=kWMrkANkCIqD+F5WT/UDyvNpAYue6SwtCjsECm7HqxU=; b=TSTlQePFvY44dbCwW50dLMsbSZqNAzrVkxjk3dhcHgdDl0wFbGsDvaej5XjDrrirB9 Ujl9ssUO6QRSMZpnWJH2skZTdMJTLpLa5gGi3E6pvx/b1RULuvlwuZhvIH9jhzJVv24X OSzSiPO89+8j1CgKR8BtGFfrtCPp/3Mgr5e//GNOmAn+TOataZ1WcaEYH81vbu3LFS/t gefUJ8yiwPtNpA/rsPYR4FUmivIG0Rts+jZ2fhvIQv1ulZSA9Mb0zOo8I+KDRSuuiQwl /AAhQgBZeVydkMLOItBeeZcYxQveCxDGjqBjuwHWO+xey3+etjySPhPQgLPdOmfkL/xR EQNw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=kWMrkANkCIqD+F5WT/UDyvNpAYue6SwtCjsECm7HqxU=; b=qrxADaclKwVRIVqVa53Z3smxorvD/JFk9lSKs/bGpJQZCZKQeYW+kjnGCv/yKJ9KTO Hd6z/ZekT2i+O56G8+INKJ0y1htlr3P4HyHAcgZa+UvB4NwsXlS3ni3ormc+RXqtZpDo rI8w2I7lbyBukg9jcyrpB8Ax0iOpqg6dMG/4yFXCcRv0sAwS7VXXSZdzIM1B5PG1q2Rq xI8DuPYLrPshxna4jC606kRkw6cU1MSUfCmS0W3T1+7b63F+Y88yr18kJ7ukqyNJdCMY DQB6/4nRwCg4J64oAY/P3VC/sMJ3ZqksN6qImd5AKPYZi/RdcobHPo1QP9/jtCOmi0lD jJAw== X-Gm-Message-State: AOAM532aXdgwOgZoYzWG71hfJrfgnC92qy8D+BtYkVmJj8fubx3zcCzV fNnzbHanYucxh88vZnxnt/HzgjMePkxFXJFIxBs= X-Google-Smtp-Source: ABdhPJzb2HqiEAW4x3079cxCOZjwrewRSe7x8i6IsObyUq6/8ddwnEnSBId/hVqzTz9Fx67u9zjDe4QGD4KQJgtC8HU= X-Received: by 2002:a65:6951:0:b0:381:f10:ccaa with SMTP id w17-20020a656951000000b003810f10ccaamr6048501pgq.587.1648174849468; Thu, 24 Mar 2022 19:20:49 -0700 (PDT) MIME-Version: 1.0 References: <20220323232929.3035443-1-jiaqiyan@google.com> <20220323232929.3035443-2-jiaqiyan@google.com> In-Reply-To: <20220323232929.3035443-2-jiaqiyan@google.com> From: Yang Shi Date: Thu, 24 Mar 2022 19:20:37 -0700 Message-ID: Subject: Re: [RFC v1 1/2] mm: khugepaged: recover from poisoned anonymous memory To: Jiaqi Yan Cc: Tony Luck , =?UTF-8?B?SE9SSUdVQ0hJIE5BT1lBKOWggOWPoyDnm7TkuZ8p?= , "Kirill A. Shutemov" , Miaohe Lin , Jue Wang , Linux MM Content-Type: text/plain; charset="UTF-8" X-Stat-Signature: r5y7oxjo1swto3ot4x5aeyqt9yekbi3f Authentication-Results: imf23.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=TSTlQePF; spf=pass (imf23.hostedemail.com: domain of shy828301@gmail.com designates 209.85.210.170 as permitted sender) smtp.mailfrom=shy828301@gmail.com; dmarc=pass (policy=none) header.from=gmail.com X-Rspam-User: X-Rspamd-Server: rspam08 X-Rspamd-Queue-Id: CF319140005 X-HE-Tag: 1648174850-177288 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Wed, Mar 23, 2022 at 4:29 PM 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. Roll back the page table and page states > in the 2nd loop when copy failed: > 1) re-establish the PTEs-to-PMD connection. > 2) release pages back to their LRU list. > > Signed-off-by: Jiaqi Yan > --- > include/linux/highmem.h | 19 ++++++ > mm/khugepaged.c | 136 ++++++++++++++++++++++++++++++---------- > 2 files changed, 122 insertions(+), 33 deletions(-) > > diff --git a/include/linux/highmem.h b/include/linux/highmem.h > index 39bb9b47fa9c..15d0aa4d349c 100644 > --- a/include/linux/highmem.h > +++ b/include/linux/highmem.h > @@ -281,6 +281,25 @@ static inline void copy_user_highpage(struct page *to, struct page *from, > kunmap_local(vfrom); > } > > +/* > + * Machine check exception handled version of copy_user_highpage. > + * Return true if copying page content failed; otherwise false. > + */ > +static inline bool copy_user_highpage_mc(struct page *to, struct page *from, > + unsigned long vaddr, struct vm_area_struct *vma) Patch #2 defined copy_highpage_mc() which has the same implementation. This function has two unused parameters: vaddr and vma. It seems we just need to keep one. > +{ > + 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; > +} > + > #endif > > #ifndef __HAVE_ARCH_COPY_HIGHPAGE > diff --git a/mm/khugepaged.c b/mm/khugepaged.c > index 131492fd1148..84ed177f56ff 100644 > --- a/mm/khugepaged.c > +++ b/mm/khugepaged.c > @@ -739,44 +739,97 @@ 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 copy succeeds; > + * otherwise restore the original pmd page table. > + * > + * @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 PTEs' PMD > + * @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; > > - if (pte_none(pteval) || is_zero_pfn(pte_pfn(pteval))) { > + /* > + * 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); > - 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); > + else { > + src_page = pte_page(pteval); > + if (copy_user_highpage_mc(page, src_page, address, vma)) { > + copy_succeeded = false; > + break; > + } > + } > + } > + > + if (!copy_succeeded) { > + /* > + * Copying failed, re-establish the regular PMD that > + * points to regular page table. Since PTEs are still > + * isolated and locked, acquiring anon_vma_lock is unnecessary. > + */ > + pmd_ptl = pmd_lock(vma->vm_mm, pmd); > + pmd_populate(vma->vm_mm, pmd, pmd_pgtable(rollback)); > + spin_unlock(pmd_ptl); I think we could just jump to list_for_each_entry_safe, right? But it seems that list_for_each_entry_safe is not enough, please see the later comments. > + } > + > + 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))) { > + if (copy_succeeded) { With the above jump, you don't need to check if copy is succeeded or not since this is the succeeded path only. > + add_mm_counter(vma->vm_mm, MM_ANONPAGES, 1); > + if (is_zero_pfn(pte_pfn(pteval))) { > + /* > + * ptl mostly unnecessary. > + */ > + spin_lock(pte_ptl); > + pte_clear(vma->vm_mm, _address, _pte); > + spin_unlock(pte_ptl); > + } > } > } else { > src_page = pte_page(pteval); > - copy_user_highpage(page, src_page, address, vma); > if (!PageCompound(src_page)) > release_pte_page(src_page); > - /* > - * 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, false); > - spin_unlock(ptl); > - free_page_and_swap_cache(src_page); > + > + if (copy_succeeded) { > + /* > + * ptl mostly unnecessary, but preempt has to > + * be disabled to update the per-cpu stats > + * inside page_remove_rmap(). > + */ > + spin_lock(pte_ptl); > + pte_clear(vma->vm_mm, _address, _pte); > + page_remove_rmap(src_page, false); > + spin_unlock(pte_ptl); > + free_page_and_swap_cache(src_page); > + } > } > } > > @@ -784,6 +837,8 @@ static void __collapse_huge_page_copy(pte_t *pte, struct page *page, > list_del(&src_page->lru); > release_pte_page(src_page); > } If copy failed you need to unpin the isolated pages and put them back to LRU IIUC. > + > + return copy_succeeded; > } > > static void khugepaged_alloc_sleep(void) > @@ -1066,6 +1121,7 @@ static void collapse_huge_page(struct mm_struct *mm, > struct vm_area_struct *vma; > struct mmu_notifier_range range; > gfp_t gfp; > + bool copy_succeeded = false; > > VM_BUG_ON(address & ~HPAGE_PMD_MASK); > > @@ -1177,9 +1233,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); > + copy_succeeded = __collapse_huge_page_copy(pte, new_page, pmd, _pmd, > + vma, address, pte_ptl, &compound_pagelist); > pte_unmap(pte); > + if (!copy_succeeded) { Shall the fail handling be moved before pte_unmap()? > + result = SCAN_FAIL; I think a new result is preferred, for example, SCAN_COPY_MC, it would be helpful for debug. > + goto out_up_write; > + } > /* > * spin_lock() below is not the equivalent of smp_wmb(), but > * the smp_wmb() inside __SetPageUptodate() can be reused to > @@ -1364,9 +1424,14 @@ static int khugepaged_scan_pmd(struct mm_struct *mm, > pte_unmap_unlock(pte, ptl); > if (ret) { > node = khugepaged_find_target_node(); > - /* collapse_huge_page will return with the mmap_lock released */ > - collapse_huge_page(mm, address, hpage, node, > - referenced, unmapped); > + /* > + * collapse_huge_page will return with the mmap_r+w_lock released. > + * It is uncertain if *hpage is NULL or not when collapse_huge_page > + * returns, so keep ret=1 to jump to breakouterloop_mmap_lock > + * in khugepaged_scan_mm_slot, then *hpage will be freed > + * if collapse failed. It may be not true for !NUMA case, the huge page may be reused, so you need to make sure the huge page is cleared before return, I had a patch that remove the special case for !NUMA (https://lore.kernel.org/linux-mm/20210817202146.3218-1-shy828301@gmail.com/), it could avoid clearing huge page, I should resurrect it. > + */ > + collapse_huge_page(mm, address, hpage, node, referenced, unmapped); > } > out: > trace_mm_khugepaged_scan_pmd(mm, page, writable, referenced, > @@ -2168,6 +2233,11 @@ 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) released if both scan and collapse succeeded; > + * 2) still held if either scan or collapse failed. > + */ > ret = khugepaged_scan_pmd(mm, vma, > khugepaged_scan.address, > hpage); > -- > 2.35.1.894.gb6a874cedc-goog >