From: Qi Zheng <zhengqi.arch@bytedance.com>
To: Hugh Dickins <hughd@google.com>,
Andrew Morton <akpm@linux-foundation.org>,
Pasha Tatashin <pasha.tatashin@soleen.com>
Cc: Mike Kravetz <mike.kravetz@oracle.com>,
Mike Rapoport <rppt@kernel.org>,
"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
Matthew Wilcox <willy@infradead.org>,
David Hildenbrand <david@redhat.com>,
Suren Baghdasaryan <surenb@google.com>,
Yang Shi <shy828301@gmail.com>,
Mel Gorman <mgorman@techsingularity.net>,
Peter Xu <peterx@redhat.com>,
Peter Zijlstra <peterz@infradead.org>,
Will Deacon <will@kernel.org>, Yu Zhao <yuzhao@google.com>,
Alistair Popple <apopple@nvidia.com>,
Ralph Campbell <rcampbell@nvidia.com>,
Ira Weiny <ira.weiny@intel.com>,
Steven Price <steven.price@arm.com>,
SeongJae Park <sj@kernel.org>,
Lorenzo Stoakes <lstoakes@gmail.com>,
Huang Ying <ying.huang@intel.com>,
Naoya Horiguchi <naoya.horiguchi@nec.com>,
Christophe Leroy <christophe.leroy@csgroup.eu>,
Zack Rusin <zackr@vmware.com>, Jason Gunthorpe <jgg@ziepe.ca>,
Axel Rasmussen <axelrasmussen@google.com>,
Anshuman Khandual <anshuman.khandual@arm.com>,
Miaohe Lin <linmiaohe@huawei.com>,
Minchan Kim <minchan@kernel.org>,
Christoph Hellwig <hch@infradead.org>, Song Liu <song@kernel.org>,
Thomas Hellstrom <thomas.hellstrom@linux.intel.com>,
Russell King <linux@armlinux.org.uk>,
"David S. Miller" <davem@davemloft.net>,
Michael Ellerman <mpe@ellerman.id.au>,
"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>,
Heiko Carstens <hca@linux.ibm.com>,
Christian Borntraeger <borntraeger@linux.ibm.com>,
Claudio Imbrenda <imbrenda@linux.ibm.com>,
Alexander Gordeev <agordeev@linux.ibm.com>,
Gerald Schaefer <gerald.schaefer@linux.ibm.com>,
Vasily Gorbik <gor@linux.ibm.com>, Jann Horn <jannh@google.com>,
Vishal Moola <vishal.moola@gmail.com>,
Vlastimil Babka <vbabka@suse.cz>, Zi Yan <ziy@nvidia.com>,
linux-arm-kernel@lists.infradead.org, sparclinux@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-s390@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [PATCH v3 10/13] mm/khugepaged: collapse_pte_mapped_thp() with mmap_read_lock()
Date: Thu, 3 Aug 2023 17:17:34 +0800 [thread overview]
Message-ID: <0df84f9f-e9b0-80b1-4c9e-95abc1a73a96@bytedance.com> (raw)
In-Reply-To: <b53be6a4-7715-51f9-aad-f1347dcb7c4@google.com>
Hi,
On 2023/7/12 12:42, Hugh Dickins wrote:
> Bring collapse_and_free_pmd() back into collapse_pte_mapped_thp().
> It does need mmap_read_lock(), but it does not need mmap_write_lock(),
> nor vma_start_write() nor i_mmap lock nor anon_vma lock. All racing
> paths are relying on pte_offset_map_lock() and pmd_lock(), so use those.
>
> Follow the pattern in retract_page_tables(); and using pte_free_defer()
> removes most of the need for tlb_remove_table_sync_one() here; but call
> pmdp_get_lockless_sync() to use it in the PAE case.
>
> First check the VMA, in case page tables are being torn down: from JannH.
> Confirm the preliminary find_pmd_or_thp_or_none() once page lock has been
> acquired and the page looks suitable: from then on its state is stable.
>
> However, collapse_pte_mapped_thp() was doing something others don't:
> freeing a page table still containing "valid" entries. i_mmap lock did
> stop a racing truncate from double-freeing those pages, but we prefer
> collapse_pte_mapped_thp() to clear the entries as usual. Their TLB
> flush can wait until the pmdp_collapse_flush() which follows, but the
> mmu_notifier_invalidate_range_start() has to be done earlier.
>
> Do the "step 1" checking loop without mmu_notifier: it wouldn't be good
> for khugepaged to keep on repeatedly invalidating a range which is then
> found unsuitable e.g. contains COWs. "step 2", which does the clearing,
> must then be more careful (after dropping ptl to do mmu_notifier), with
> abort prepared to correct the accounting like "step 3". But with those
> entries now cleared, "step 4" (after dropping ptl to do pmd_lock) is kept
> safe by the huge page lock, which stops new PTEs from being faulted in.
>
> Signed-off-by: Hugh Dickins <hughd@google.com>
> ---
> mm/khugepaged.c | 172 ++++++++++++++++++++++----------------------------
> 1 file changed, 77 insertions(+), 95 deletions(-)
>
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 3bb05147961b..46986eb4eebb 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -1483,7 +1483,7 @@ static bool khugepaged_add_pte_mapped_thp(struct mm_struct *mm,
> return ret;
> }
>
> -/* hpage must be locked, and mmap_lock must be held in write */
> +/* hpage must be locked, and mmap_lock must be held */
> static int set_huge_pmd(struct vm_area_struct *vma, unsigned long addr,
> pmd_t *pmdp, struct page *hpage)
> {
> @@ -1495,7 +1495,7 @@ static int set_huge_pmd(struct vm_area_struct *vma, unsigned long addr,
> };
>
> VM_BUG_ON(!PageTransHuge(hpage));
> - mmap_assert_write_locked(vma->vm_mm);
> + mmap_assert_locked(vma->vm_mm);
>
> if (do_set_pmd(&vmf, hpage))
> return SCAN_FAIL;
> @@ -1504,48 +1504,6 @@ static int set_huge_pmd(struct vm_area_struct *vma, unsigned long addr,
> return SCAN_SUCCEED;
> }
>
> -/*
> - * A note about locking:
> - * Trying to take the page table spinlocks would be useless here because those
> - * are only used to synchronize:
> - *
> - * - modifying terminal entries (ones that point to a data page, not to another
> - * page table)
> - * - installing *new* non-terminal entries
> - *
> - * Instead, we need roughly the same kind of protection as free_pgtables() or
> - * mm_take_all_locks() (but only for a single VMA):
> - * The mmap lock together with this VMA's rmap locks covers all paths towards
> - * the page table entries we're messing with here, except for hardware page
> - * table walks and lockless_pages_from_mm().
> - */
> -static void collapse_and_free_pmd(struct mm_struct *mm, struct vm_area_struct *vma,
> - unsigned long addr, pmd_t *pmdp)
> -{
> - pmd_t pmd;
> - struct mmu_notifier_range range;
> -
> - mmap_assert_write_locked(mm);
> - if (vma->vm_file)
> - lockdep_assert_held_write(&vma->vm_file->f_mapping->i_mmap_rwsem);
> - /*
> - * All anon_vmas attached to the VMA have the same root and are
> - * therefore locked by the same lock.
> - */
> - if (vma->anon_vma)
> - lockdep_assert_held_write(&vma->anon_vma->root->rwsem);
> -
> - mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, mm, addr,
> - addr + HPAGE_PMD_SIZE);
> - mmu_notifier_invalidate_range_start(&range);
> - pmd = pmdp_collapse_flush(vma, addr, pmdp);
> - tlb_remove_table_sync_one();
> - mmu_notifier_invalidate_range_end(&range);
> - mm_dec_nr_ptes(mm);
> - page_table_check_pte_clear_range(mm, addr, pmd);
> - pte_free(mm, pmd_pgtable(pmd));
> -}
> -
> /**
> * collapse_pte_mapped_thp - Try to collapse a pte-mapped THP for mm at
> * address haddr.
> @@ -1561,26 +1519,29 @@ static void collapse_and_free_pmd(struct mm_struct *mm, struct vm_area_struct *v
> int collapse_pte_mapped_thp(struct mm_struct *mm, unsigned long addr,
> bool install_pmd)
> {
> + struct mmu_notifier_range range;
> + bool notified = false;
> unsigned long haddr = addr & HPAGE_PMD_MASK;
> struct vm_area_struct *vma = vma_lookup(mm, haddr);
> struct page *hpage;
> pte_t *start_pte, *pte;
> - pmd_t *pmd;
> - spinlock_t *ptl;
> - int count = 0, result = SCAN_FAIL;
> + pmd_t *pmd, pgt_pmd;
> + spinlock_t *pml, *ptl;
> + int nr_ptes = 0, result = SCAN_FAIL;
> int i;
>
> - mmap_assert_write_locked(mm);
> + mmap_assert_locked(mm);
> +
> + /* First check VMA found, in case page tables are being torn down */
> + if (!vma || !vma->vm_file ||
> + !range_in_vma(vma, haddr, haddr + HPAGE_PMD_SIZE))
> + return SCAN_VMA_CHECK;
>
> /* Fast check before locking page if already PMD-mapped */
> result = find_pmd_or_thp_or_none(mm, haddr, &pmd);
> if (result == SCAN_PMD_MAPPED)
> return result;
>
> - if (!vma || !vma->vm_file ||
> - !range_in_vma(vma, haddr, haddr + HPAGE_PMD_SIZE))
> - return SCAN_VMA_CHECK;
> -
> /*
> * If we are here, we've succeeded in replacing all the native pages
> * in the page cache with a single hugepage. If a mm were to fault-in
> @@ -1610,6 +1571,7 @@ int collapse_pte_mapped_thp(struct mm_struct *mm, unsigned long addr,
> goto drop_hpage;
> }
>
> + result = find_pmd_or_thp_or_none(mm, haddr, &pmd);
> switch (result) {
> case SCAN_SUCCEED:
> break;
> @@ -1623,27 +1585,10 @@ int collapse_pte_mapped_thp(struct mm_struct *mm, unsigned long addr,
> goto drop_hpage;
> }
>
> - /* Lock the vma before taking i_mmap and page table locks */
> - vma_start_write(vma);
> -
> - /*
> - * We need to lock the mapping so that from here on, only GUP-fast and
> - * hardware page walks can access the parts of the page tables that
> - * we're operating on.
> - * See collapse_and_free_pmd().
> - */
> - i_mmap_lock_write(vma->vm_file->f_mapping);
> -
> - /*
> - * This spinlock should be unnecessary: Nobody else should be accessing
> - * the page tables under spinlock protection here, only
> - * lockless_pages_from_mm() and the hardware page walker can access page
> - * tables while all the high-level locks are held in write mode.
> - */
> result = SCAN_FAIL;
> start_pte = pte_offset_map_lock(mm, pmd, haddr, &ptl);
> - if (!start_pte)
> - goto drop_immap;
> + if (!start_pte) /* mmap_lock + page lock should prevent this */
> + goto drop_hpage;
>
> /* step 1: check all mapped PTEs are to the right huge page */
> for (i = 0, addr = haddr, pte = start_pte;
> @@ -1670,10 +1615,18 @@ int collapse_pte_mapped_thp(struct mm_struct *mm, unsigned long addr,
> */
> if (hpage + i != page)
> goto abort;
> - count++;
> }
>
> - /* step 2: adjust rmap */
> + pte_unmap_unlock(start_pte, ptl);
> + mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, mm,
> + haddr, haddr + HPAGE_PMD_SIZE);
> + mmu_notifier_invalidate_range_start(&range);
> + notified = true;
> + start_pte = pte_offset_map_lock(mm, pmd, haddr, &ptl);
> + if (!start_pte) /* mmap_lock + page lock should prevent this */
> + goto abort;
> +
> + /* step 2: clear page table and adjust rmap */
> for (i = 0, addr = haddr, pte = start_pte;
> i < HPAGE_PMD_NR; i++, addr += PAGE_SIZE, pte++) {
> struct page *page;
> @@ -1681,47 +1634,76 @@ int collapse_pte_mapped_thp(struct mm_struct *mm, unsigned long addr,
>
> if (pte_none(ptent))
> continue;
> - page = vm_normal_page(vma, addr, ptent);
> - if (WARN_ON_ONCE(page && is_zone_device_page(page)))
> + /*
> + * We dropped ptl after the first scan, to do the mmu_notifier:
> + * page lock stops more PTEs of the hpage being faulted in, but
> + * does not stop write faults COWing anon copies from existing
> + * PTEs; and does not stop those being swapped out or migrated.
> + */
> + if (!pte_present(ptent)) {
> + result = SCAN_PTE_NON_PRESENT;
> goto abort;
> + }
> + page = vm_normal_page(vma, addr, ptent);
> + if (hpage + i != page)
> + goto abort;
> +
> + /*
> + * Must clear entry, or a racing truncate may re-remove it.
> + * TLB flush can be left until pmdp_collapse_flush() does it.
> + * PTE dirty? Shmem page is already dirty; file is read-only.
> + */
> + pte_clear(mm, addr, pte);
This is not non-present PTE entry, so we should call ptep_clear() to let
page_table_check track the PTE clearing operation, right? Otherwise it
may lead to false positives?
Thanks,
Qi
> page_remove_rmap(page, vma, false);
> + nr_ptes++;
> }
>
> pte_unmap_unlock(start_pte, ptl);
>
> /* step 3: set proper refcount and mm_counters. */
> - if (count) {
> - page_ref_sub(hpage, count);
> - add_mm_counter(vma->vm_mm, mm_counter_file(hpage), -count);
> + if (nr_ptes) {
> + page_ref_sub(hpage, nr_ptes);
> + add_mm_counter(mm, mm_counter_file(hpage), -nr_ptes);
> }
>
> - /* step 4: remove pte entries */
> - /* we make no change to anon, but protect concurrent anon page lookup */
> - if (vma->anon_vma)
> - anon_vma_lock_write(vma->anon_vma);
> + /* step 4: remove page table */
>
> - collapse_and_free_pmd(mm, vma, haddr, pmd);
> + /* Huge page lock is still held, so page table must remain empty */
> + pml = pmd_lock(mm, pmd);
> + if (ptl != pml)
> + spin_lock_nested(ptl, SINGLE_DEPTH_NESTING);
> + pgt_pmd = pmdp_collapse_flush(vma, haddr, pmd);
> + pmdp_get_lockless_sync();
> + if (ptl != pml)
> + spin_unlock(ptl);
> + spin_unlock(pml);
>
> - if (vma->anon_vma)
> - anon_vma_unlock_write(vma->anon_vma);
> - i_mmap_unlock_write(vma->vm_file->f_mapping);
> + mmu_notifier_invalidate_range_end(&range);
> +
> + mm_dec_nr_ptes(mm);
> + page_table_check_pte_clear_range(mm, haddr, pgt_pmd);
> + pte_free_defer(mm, pmd_pgtable(pgt_pmd));
>
> maybe_install_pmd:
> /* step 5: install pmd entry */
> result = install_pmd
> ? set_huge_pmd(vma, haddr, pmd, hpage)
> : SCAN_SUCCEED;
> -
> + goto drop_hpage;
> +abort:
> + if (nr_ptes) {
> + flush_tlb_mm(mm);
> + page_ref_sub(hpage, nr_ptes);
> + add_mm_counter(mm, mm_counter_file(hpage), -nr_ptes);
> + }
> + if (start_pte)
> + pte_unmap_unlock(start_pte, ptl);
> + if (notified)
> + mmu_notifier_invalidate_range_end(&range);
> drop_hpage:
> unlock_page(hpage);
> put_page(hpage);
> return result;
> -
> -abort:
> - pte_unmap_unlock(start_pte, ptl);
> -drop_immap:
> - i_mmap_unlock_write(vma->vm_file->f_mapping);
> - goto drop_hpage;
> }
>
> static void khugepaged_collapse_pte_mapped_thps(struct khugepaged_mm_slot *mm_slot)
> @@ -2855,9 +2837,9 @@ int madvise_collapse(struct vm_area_struct *vma, struct vm_area_struct **prev,
> case SCAN_PTE_MAPPED_HUGEPAGE:
> BUG_ON(mmap_locked);
> BUG_ON(*prev);
> - mmap_write_lock(mm);
> + mmap_read_lock(mm);
> result = collapse_pte_mapped_thp(mm, addr, true);
> - mmap_write_unlock(mm);
> + mmap_locked = true;
> goto handle_result;
> /* Whitelisted set of results where continuing OK */
> case SCAN_PMD_NULL:
next prev parent reply other threads:[~2023-08-03 9:18 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-12 4:27 [PATCH v3 00/13] mm: free retracted page table by RCU Hugh Dickins
2023-07-12 4:30 ` [PATCH v3 01/13] mm/pgtable: add rcu_read_lock() and rcu_read_unlock()s Hugh Dickins
2023-07-12 4:32 ` [PATCH v3 02/13] mm/pgtable: add PAE safety to __pte_offset_map() Hugh Dickins
2023-07-12 4:33 ` [PATCH v3 03/13] arm: adjust_pte() use pte_offset_map_nolock() Hugh Dickins
2023-07-12 4:34 ` [PATCH v3 04/13] powerpc: assert_pte_locked() " Hugh Dickins
[not found] ` <87msztbiy8.fsf@linux.ibm.com>
2023-07-19 5:04 ` Hugh Dickins
[not found] ` <6762c880-6d2b-233f-6786-7ad5b0472dc7@linux.ibm.com>
[not found] ` <20230721131341.w5abuxcbohofpzwa@patel>
2023-07-23 22:26 ` [PATCH v3 04/13 fix] powerpc: assert_pte_locked() use pte_offset_map_nolock(): fix Hugh Dickins
2023-07-12 4:35 ` [PATCH v3 05/13] powerpc: add pte_free_defer() for pgtables sharing page Hugh Dickins
2023-07-12 4:37 ` [PATCH v3 06/13] sparc: add pte_free_defer() for pte_t *pgtable_t Hugh Dickins
2023-07-12 4:38 ` [PATCH v3 07/13] s390: add pte_free_defer() for pgtables sharing page Hugh Dickins
2023-07-13 4:47 ` Alexander Gordeev
[not found] ` <20230719162506.235856eb@p-imbrenda>
2023-07-23 22:29 ` [PATCH v3 07/13 fix] s390: add pte_free_defer() for pgtables sharing page: fix Hugh Dickins
2023-07-12 4:39 ` [PATCH v3 08/13] mm/pgtable: add pte_free_defer() for pgtable as page Hugh Dickins
2023-07-12 4:41 ` [PATCH v3 09/13] mm/khugepaged: retract_page_tables() without mmap or vma lock Hugh Dickins
2023-07-12 4:42 ` [PATCH v3 10/13] mm/khugepaged: collapse_pte_mapped_thp() with mmap_read_lock() Hugh Dickins
2023-07-23 22:32 ` [PATCH v3 10/13 fix] mm/khugepaged: collapse_pte_mapped_thp() with mmap_read_lock(): fix Hugh Dickins
2023-08-03 9:17 ` Qi Zheng [this message]
2023-08-06 3:55 ` [PATCH v3 10/13] mm/khugepaged: collapse_pte_mapped_thp() with mmap_read_lock() Hugh Dickins
2023-08-07 2:21 ` Qi Zheng
2023-08-06 3:59 ` [PATCH v3 10/13 fix2] mm/khugepaged: collapse_pte_mapped_thp() with mmap_read_lock(): fix2 Hugh Dickins
2023-08-14 20:36 ` [BUG] Re: [PATCH v3 10/13] mm/khugepaged: collapse_pte_mapped_thp() with mmap_read_lock() Jann Horn
2023-08-15 6:34 ` Hugh Dickins
2023-08-15 7:11 ` David Hildenbrand
2023-08-15 15:41 ` Hugh Dickins
2023-08-21 19:48 ` Hugh Dickins
2023-07-12 4:43 ` [PATCH v3 11/13] mm/khugepaged: delete khugepaged_collapse_pte_mapped_thps() Hugh Dickins
2023-07-23 22:35 ` [PATCH v3 11/13 fix] mm/khugepaged: delete khugepaged_collapse_pte_mapped_thps(): fix Hugh Dickins
2023-07-12 4:44 ` [PATCH v3 12/13] mm: delete mmap_write_trylock() and vma_try_start_write() Hugh Dickins
2023-07-12 4:48 ` [PATCH mm " Hugh Dickins
2023-07-12 4:46 ` [PATCH v3 13/13] mm/pgtable: notes on pte_offset_map[_lock]() Hugh Dickins
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0df84f9f-e9b0-80b1-4c9e-95abc1a73a96@bytedance.com \
--to=zhengqi.arch@bytedance.com \
--cc=agordeev@linux.ibm.com \
--cc=akpm@linux-foundation.org \
--cc=aneesh.kumar@linux.ibm.com \
--cc=anshuman.khandual@arm.com \
--cc=apopple@nvidia.com \
--cc=axelrasmussen@google.com \
--cc=borntraeger@linux.ibm.com \
--cc=christophe.leroy@csgroup.eu \
--cc=davem@davemloft.net \
--cc=david@redhat.com \
--cc=gerald.schaefer@linux.ibm.com \
--cc=gor@linux.ibm.com \
--cc=hca@linux.ibm.com \
--cc=hch@infradead.org \
--cc=hughd@google.com \
--cc=imbrenda@linux.ibm.com \
--cc=ira.weiny@intel.com \
--cc=jannh@google.com \
--cc=jgg@ziepe.ca \
--cc=kirill.shutemov@linux.intel.com \
--cc=linmiaohe@huawei.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linux-s390@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=lstoakes@gmail.com \
--cc=mgorman@techsingularity.net \
--cc=mike.kravetz@oracle.com \
--cc=minchan@kernel.org \
--cc=mpe@ellerman.id.au \
--cc=naoya.horiguchi@nec.com \
--cc=pasha.tatashin@soleen.com \
--cc=peterx@redhat.com \
--cc=peterz@infradead.org \
--cc=rcampbell@nvidia.com \
--cc=rppt@kernel.org \
--cc=shy828301@gmail.com \
--cc=sj@kernel.org \
--cc=song@kernel.org \
--cc=sparclinux@vger.kernel.org \
--cc=steven.price@arm.com \
--cc=surenb@google.com \
--cc=thomas.hellstrom@linux.intel.com \
--cc=vbabka@suse.cz \
--cc=vishal.moola@gmail.com \
--cc=will@kernel.org \
--cc=willy@infradead.org \
--cc=ying.huang@intel.com \
--cc=yuzhao@google.com \
--cc=zackr@vmware.com \
--cc=ziy@nvidia.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).