Re: [PATCH V3] powerpc/mm: Fix Multi hit ERAT cause by recent THP update

Balbir Singh [off-list ref] writes:

On Tue, 2016-02-09 at 06:50 +0530, Aneesh Kumar K.V wrote:

quoted

=C2=A0
Also make sure we wait for irq disable section in other cpus to finish
before flipping a huge pte entry with a regular pmd entry. Code paths
like find_linux_pte_or_hugepte depend on irq disable to get
a stable pte_t pointer. A parallel thp split need to make sure we
don't convert a pmd pte to a regular pmd entry without waiting for the
irq disable section to finish.
=20
Acked-by: Kirill A. Shutemov <redacted>
Signed-off-by: Aneesh Kumar K.V <redacted>
---
=C2=A0arch/powerpc/include/asm/book3s/64/pgtable.h |=C2=A0=C2=A04 ++++
=C2=A0arch/powerpc/mm/pgtable_64.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=

=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0| 35

quoted

+++++++++++++++++++++++++++-
=C2=A0include/asm-generic/pgtable.h=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=

=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0|=C2=A0=C2=A08 =
+++++++

quoted

=C2=A0mm/huge_memory.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=

=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0|=C2=A0=C2=A01 +

quoted

=C2=A04 files changed, 47 insertions(+), 1 deletion(-)
=20

diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h

b/arch/powerpc/include/asm/book3s/64/pgtable.h
index 8d1c41d28318..ac07a30a7934 100644

--- a/arch/powerpc/include/asm/book3s/64/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/64/pgtable.h

@@ -281,6 +281,10 @@ extern pgtable_t pgtable_trans_huge_withdraw(struct

mm_struct *mm, pmd_t *pmdp);
=C2=A0extern void pmdp_invalidate(struct vm_area_struct *vma, unsigned l=

ong

quoted

address,
=C2=A0			=C2=A0=C2=A0=C2=A0=C2=A0pmd_t *pmdp);
=C2=A0
+#define __HAVE_ARCH_PMDP_HUGE_SPLIT_PREPARE
+extern void pmdp_huge_split_prepare(struct vm_area_struct *vma,
+				=C2=A0=C2=A0=C2=A0=C2=A0unsigned long address, pmd_t *pmdp);
+
=C2=A0#define pmd_move_must_withdraw pmd_move_must_withdraw
=C2=A0struct spinlock;
=C2=A0static inline int pmd_move_must_withdraw(struct spinlock *new_pmd_=

ptl,

quoted

diff --git a/arch/powerpc/mm/pgtable_64.c b/arch/powerpc/mm/pgtable_64.c
index 3124a20d0fab..c8a00da39969 100644
--- a/arch/powerpc/mm/pgtable_64.c
+++ b/arch/powerpc/mm/pgtable_64.c

@@ -646,6 +646,30 @@ pgtable_t pgtable_trans_huge_withdraw(struct mm_str=

uct

quoted

*mm, pmd_t *pmdp)
=C2=A0	return pgtable;
=C2=A0}
=C2=A0
+void pmdp_huge_split_prepare(struct vm_area_struct *vma,
+			=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0unsigned long address, pmd_t *pmdp)
+{
+	VM_BUG_ON(address & ~HPAGE_PMD_MASK);
+
+#ifdef CONFIG_DEBUG_VM
+	BUG_ON(REGION_ID(address) !=3D USER_REGION_ID);
+#endif
+	/*
+	=C2=A0* We can't mark the pmd none here, because that will cause a race
+	=C2=A0* against exit_mmap. We need to continue mark pmd TRANS HUGE, wh=

ile

quoted

+	=C2=A0* we spilt, but at the same time we wan't rest of the ppc64 code
+	=C2=A0* not to insert hash pte on this, because we will be modifying
+	=C2=A0* the deposited pgtable in the caller of this function. Hence
+	=C2=A0* clear the _PAGE_USER so that we move the fault handling to
+	=C2=A0* higher level function and that will serialize against ptl.
+	=C2=A0* We need to flush existing hash pte entries here even though,
+	=C2=A0* the translation is still valid, because we will withdraw
+	=C2=A0* pgtable_t after this.
+	=C2=A0*/
+	pmd_hugepage_update(vma->vm_mm, address, pmdp, _PAGE_USER, 0);

Can this break any checks for _PAGE_USER? From other paths?

Should not, that is the same condition we use for autonuma.

quoted

+}
+
+
=C2=A0/*
=C2=A0 * set a new huge pmd. We should not be called for updating
=C2=A0 * an existing pmd entry. That should go via pmd_hugepage_update.

@@ -663,10 +687,19 @@ void set_pmd_at(struct mm_struct *mm, unsigned long

addr,
=C2=A0	return set_pte_at(mm, addr, pmdp_ptep(pmdp), pmd_pte(pmd));
=C2=A0}
=C2=A0
+/*
+ * We use this to invalidate a pmdp entry before switching from a
+ * hugepte to regular pmd entry.
+ */
=C2=A0void pmdp_invalidate(struct vm_area_struct *vma, unsigned long add=

ress,

quoted

=C2=A0		=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0pmd_t *pmdp)
=C2=A0{
-	pmd_hugepage_update(vma->vm_mm, address, pmdp, _PAGE_PRESENT, 0);
+	pmd_hugepage_update(vma->vm_mm, address, pmdp, ~0UL, 0);
+	/*
+	=C2=A0* This ensures that generic code that rely on IRQ disabling
+	=C2=A0* to prevent a parallel THP split work as expected.
+	=C2=A0*/
+	kick_all_cpus_sync();

Seems expensive, anyway I think the right should do something like or a w=

rapper

for it

on_each_cpu_mask(mm_cpumask(vma->vm_mm), do_nothing, NULL, 1);

do_nothing is not exported, but that can be fixed :)

Now we can't depend for mm_cpumask, a parallel find_linux_pte_hugepte
can happen outside that. Now i had a variant for kick_all_cpus_sync that
ignored idle cpus. But then that needs more verification.

http://article.gmane.org/gmane.linux.ports.ppc.embedded/81105

-aneesh