Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1575248
| From | Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in zap_pmd_range() |
| Date | 2017-02-07 00:30 +0100 |
| Message-ID | <t80To-39D-11@gated-at.bofh.it> (permalink) |
| References | <t7xHH-1ay-5@gated-at.bofh.it> <t7xHI-1ay-31@gated-at.bofh.it> <t7MdH-1Vy-13@gated-at.bofh.it> <t7Rdp-5m6-39@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
On Mon, Feb 06, 2017 at 07:02:41AM -0600, Zi Yan wrote:
> On 6 Feb 2017, at 1:43, Naoya Horiguchi wrote:
>
> > On Sun, Feb 05, 2017 at 11:12:41AM -0500, Zi Yan wrote:
> >> From: Zi Yan <ziy@nvidia.com>
> >>
> >> Originally, zap_pmd_range() checks pmd value without taking pmd lock.
> >> This can cause pmd_protnone entry not being freed.
> >>
> >> Because there are two steps in changing a pmd entry to a pmd_protnone
> >> entry. First, the pmd entry is cleared to a pmd_none entry, then,
> >> the pmd_none entry is changed into a pmd_protnone entry.
> >> The racy check, even with barrier, might only see the pmd_none entry
> >> in zap_pmd_range(), thus, the mapping is neither split nor zapped.
> >>
> >> Later, in free_pmd_range(), pmd_none_or_clear() will see the
> >> pmd_protnone entry and clear it as a pmd_bad entry. Furthermore,
> >> since the pmd_protnone entry is not properly freed, the corresponding
> >> deposited pte page table is not freed either.
> >>
> >> This causes memory leak or kernel crashing, if VM_BUG_ON() is enabled.
> >>
> >> This patch relies on __split_huge_pmd_locked() and
> >> __zap_huge_pmd_locked().
> >>
> >> Signed-off-by: Zi Yan <zi.yan@cs.rutgers.edu>
> >> ---
> >> mm/memory.c | 24 +++++++++++-------------
> >> 1 file changed, 11 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/mm/memory.c b/mm/memory.c
> >> index 3929b015faf7..7cfdd5208ef5 100644
> >> --- a/mm/memory.c
> >> +++ b/mm/memory.c
> >> @@ -1233,33 +1233,31 @@ static inline unsigned long zap_pmd_range(struct mmu_gather *tlb,
> >> struct zap_details *details)
> >> {
> >> pmd_t *pmd;
> >> + spinlock_t *ptl;
> >> unsigned long next;
> >>
> >> pmd = pmd_offset(pud, addr);
> >> + ptl = pmd_lock(vma->vm_mm, pmd);
> >
> > If USE_SPLIT_PMD_PTLOCKS is true, pmd_lock() returns different ptl for
> > each pmd. The following code runs over pmds within [addr, end) with
> > a single ptl (of the first pmd,) so I suspect this locking really works.
> > Maybe pmd_lock() should be called inside while loop?
>
> According to include/linux/mm.h, pmd_lockptr() first gets the page the pmd is in,
> using mask = ~(PTRS_PER_PMD * sizeof(pmd_t) -1) = 0xfffffffffffff000 and virt_to_page().
> Then, ptlock_ptr() gets spinlock_t either from page->ptl (split case) or
> mm->page_table_lock (not split case).
>
> It seems to me that all PMDs in one page table page share a single spinlock. Let me know
> if I misunderstand any code.
Thanks for clarification, it was my misunderstanding.
Naoya
>
> But your suggestion can avoid holding the pmd lock for long without cond_sched(),
> I can move the spinlock inside the loop.
>
> Thanks.
>
> diff --git a/mm/memory.c b/mm/memory.c
> index 5299b261c4b4..ff61d45eaea7 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -1260,31 +1260,34 @@ static inline unsigned long zap_pmd_range(struct mmu_gather *tlb,
> struct zap_details *details)
> {
> pmd_t *pmd;
> - spinlock_t *ptl;
> + spinlock_t *ptl = NULL;
> unsigned long next;
>
> pmd = pmd_offset(pud, addr);
> - ptl = pmd_lock(vma->vm_mm, pmd);
> do {
> + ptl = pmd_lock(vma->vm_mm, pmd);
> next = pmd_addr_end(addr, end);
> if (is_swap_pmd(*pmd) || pmd_trans_huge(*pmd) || pmd_devmap(*pmd)) {
> if (next - addr != HPAGE_PMD_SIZE) {
> VM_BUG_ON_VMA(vma_is_anonymous(vma) &&
> !rwsem_is_locked(&tlb->mm->mmap_sem), vma);
> __split_huge_pmd_locked(vma, pmd, addr, false);
> - } else if (__zap_huge_pmd_locked(tlb, vma, pmd, addr))
> - continue;
> + } else if (__zap_huge_pmd_locked(tlb, vma, pmd, addr)) {
> + spin_unlock(ptl);
> + goto next;
> + }
> /* fall through */
> }
>
> - if (pmd_none_or_clear_bad(pmd))
> - continue;
> + if (pmd_none_or_clear_bad(pmd)) {
> + spin_unlock(ptl);
> + goto next;
> + }
> spin_unlock(ptl);
> next = zap_pte_range(tlb, vma, pmd, addr, next, details);
> +next:
> cond_resched();
> - spin_lock(ptl);
> } while (pmd++, addr = next, addr != end);
> - spin_unlock(ptl);
>
> return addr;
> }
>
>
> >
> > Thanks,
> > Naoya Horiguchi
> >
> >> do {
> >> next = pmd_addr_end(addr, end);
> >> if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd)) {
> >> if (next - addr != HPAGE_PMD_SIZE) {
> >> VM_BUG_ON_VMA(vma_is_anonymous(vma) &&
> >> !rwsem_is_locked(&tlb->mm->mmap_sem), vma);
> >> - __split_huge_pmd(vma, pmd, addr, false, NULL);
> >> - } else if (zap_huge_pmd(tlb, vma, pmd, addr))
> >> - goto next;
> >> + __split_huge_pmd_locked(vma, pmd, addr, false);
> >> + } else if (__zap_huge_pmd_locked(tlb, vma, pmd, addr))
> >> + continue;
> >> /* fall through */
> >> }
> >> - /*
> >> - * Here there can be other concurrent MADV_DONTNEED or
> >> - * trans huge page faults running, and if the pmd is
> >> - * none or trans huge it can change under us. This is
> >> - * because MADV_DONTNEED holds the mmap_sem in read
> >> - * mode.
> >> - */
> >> - if (pmd_none_or_trans_huge_or_clear_bad(pmd))
> >> - goto next;
> >> +
> >> + if (pmd_none_or_clear_bad(pmd))
> >> + continue;
> >> + spin_unlock(ptl);
> >> next = zap_pte_range(tlb, vma, pmd, addr, next, details);
> >> -next:
> >> cond_resched();
> >> + spin_lock(ptl);
> >> } while (pmd++, addr = next, addr != end);
> >> + spin_unlock(ptl);
> >>
> >> return addr;
> >> }
> >> --
> >> 2.11.0
> >>
>
>
> --
> Best Regards
> Yan Zi
Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread
[PATCH v3 00/14] mm: page migration enhancement for thp Zi Yan <zi.yan@sent.com> - 2017-02-05 17:20 +0100
[PATCH v3 11/14] mm: hwpoison: soft offline supports thp migration Zi Yan <zi.yan@sent.com> - 2017-02-05 17:20 +0100
[PATCH v3 13/14] mm: migrate: move_pages() supports thp migration Zi Yan <zi.yan@sent.com> - 2017-02-05 17:20 +0100
Re: [PATCH v3 13/14] mm: migrate: move_pages() supports thp migration Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2017-02-09 10:30 +0100
Re: [PATCH v3 13/14] mm: migrate: move_pages() supports thp migration "Zi Yan" <zi.yan@sent.com> - 2017-02-09 18:40 +0100
[PATCH v3 04/14] mm: x86: move _PAGE_SWP_SOFT_DIRTY from bit 7 to bit 1 Zi Yan <zi.yan@sent.com> - 2017-02-05 17:20 +0100
Re: [PATCH v3 04/14] mm: x86: move _PAGE_SWP_SOFT_DIRTY from bit 7 to bit 1 Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2017-02-09 10:30 +0100
[PATCH v3 03/14] mm: use pmd lock instead of racy checks in zap_pmd_range() Zi Yan <zi.yan@sent.com> - 2017-02-05 17:20 +0100
Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in zap_pmd_range() "Hillf Danton" <hillf.zj@alibaba-inc.com> - 2017-02-06 05:10 +0100
Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in zap_pmd_range() Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2017-02-06 08:50 +0100
Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in zap_pmd_range() "Zi Yan" <zi.yan@sent.com> - 2017-02-06 14:10 +0100
Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in zap_pmd_range() Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2017-02-07 00:30 +0100
Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in zap_pmd_range() "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-02-06 17:10 +0100
Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in zap_pmd_range() "Zi Yan" <zi.yan@sent.com> - 2017-02-06 17:40 +0100
Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in zap_pmd_range() "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-02-06 18:40 +0100
Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in zap_pmd_range() "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> - 2017-02-07 15:00 +0100
Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in zap_pmd_range() "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-02-07 15:30 +0100
Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in zap_pmd_range() "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-02-07 17:40 +0100
Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in zap_pmd_range() "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-02-07 19:00 +0100
Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in zap_pmd_range() "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-02-13 12:00 +0100
Re: [PATCH v3 03/14] mm: use pmd lock instead of racy checks in zap_pmd_range() Andrea Arcangeli <aarcange@redhat.com> - 2017-02-13 15:50 +0100
[PATCH v3 01/14] mm: thp: make __split_huge_pmd_locked visible. Zi Yan <zi.yan@sent.com> - 2017-02-05 17:20 +0100
Re: [PATCH v3 01/14] mm: thp: make __split_huge_pmd_locked visible. Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2017-02-06 07:30 +0100
Re: [PATCH v3 01/14] mm: thp: make __split_huge_pmd_locked visible. "Zi Yan" <zi.yan@sent.com> - 2017-02-06 13:20 +0100
Re: [PATCH v3 01/14] mm: thp: make __split_huge_pmd_locked visible. Matthew Wilcox <willy@infradead.org> - 2017-02-06 16:10 +0100
Re: [PATCH v3 01/14] mm: thp: make __split_huge_pmd_locked visible. "Zi Yan" <zi.yan@sent.com> - 2017-02-06 16:10 +0100
[PATCH v3 09/14] mm: thp: check pmd migration entry in common path Zi Yan <zi.yan@sent.com> - 2017-02-05 17:20 +0100
Re: [PATCH v3 09/14] mm: thp: check pmd migration entry in common path Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2017-02-09 10:30 +0100
[PATCH v3 07/14] mm: thp: introduce CONFIG_ARCH_ENABLE_THP_MIGRATION Zi Yan <zi.yan@sent.com> - 2017-02-05 17:20 +0100
[PATCH v3 05/14] mm: mempolicy: add queue_pages_node_check() Zi Yan <zi.yan@sent.com> - 2017-02-05 17:20 +0100
[PATCH v3 14/14] mm: memory_hotplug: memory hotremove supports thp migration Zi Yan <zi.yan@sent.com> - 2017-02-05 17:20 +0100
[PATCH v3 02/14] mm: thp: create new __zap_huge_pmd_locked function. Zi Yan <zi.yan@sent.com> - 2017-02-05 17:20 +0100
csiph-web