[PATCH v7 0/9] arm64: Enable contiguous pte hugepage support

[PATCH v7 0/9] arm64: Enable contiguous pte hugepage support

[Punit Agrawal]

Hi,

This series re-enables contiguous hugepage support for arm64.

Changes in v7 -

* Patch 4 - "Add break-before-make logic for contiguous entries"
  - get_clear_flush() - unconditionally clear the ptes (even if they
    are not present). This is to bring huge_ptep_get_and_clear()
    in line with generic implementation.
  - get_clear_flush() - Only flush the TLBs if the cleared entry was
    valid.
  - huge_ptep_set_wrprotect() - simplified based on Catalin's
    feedback.

* Patch 6 - "Override huge_pte_clear() to support"
  - Address compile warnings due to non-existence of certain levels of
    the page table when using 16 and 64k pages.

All the dependent series ([2], [3]) for enabling contiguous hugepage
support have been merged in the previous cycle. Additionally, a patch
to clarify the semantics of huge_pte_offset() in generic code[6] is
currently in the -mm tree.

Previous postings can be found at [0], [1], [4], [5], [7].

The patches are based on v4.13-rc6. If there are no further comments
please consider merging for the next release cycle.

Thanks,
Punit

[0] https://www.spinics.net/lists/arm-kernel/msg570422.html
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2017-March/497027.html
[2] https://www.spinics.net/lists/arm-kernel/msg581657.html
[3] https://www.spinics.net/lists/arm-kernel/msg583342.html
[4] https://www.spinics.net/lists/arm-kernel/msg583367.html
[5] https://www.spinics.net/lists/arm-kernel/msg582758.html
[6] https://lkml.org/lkml/2017/8/18/678
[7] https://www.spinics.net/lists/arm-kernel/msg597777.html
[8] http://www.spinics.net/lists/linux-mm/msg133176.html

Punit Agrawal (4):
  arm64: hugetlb: Handle swap entries in huge_pte_offset() for
    contiguous hugepages
  arm64: hugetlb: Override huge_pte_clear() to support contiguous
    hugepages
  arm64: hugetlb: Override set_huge_swap_pte_at() to support contiguous
    hugepages
  arm64: Re-enable support for contiguous hugepages

Steve Capper (5):
  arm64: hugetlb: set_huge_pte_at Add WARN_ON on !pte_present
  arm64: hugetlb: Introduce pte_pgprot helper
  arm64: hugetlb: Spring clean huge pte accessors
  arm64: hugetlb: Add break-before-make logic for contiguous entries
  arm64: hugetlb: Cleanup setup_hugepagesz

 arch/arm64/include/asm/hugetlb.h |   9 +-
 arch/arm64/mm/hugetlbpage.c      | 316 ++++++++++++++++++++++++++++-----------
 2 files changed, 240 insertions(+), 85 deletions(-)

-- 
2.13.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v7 1/9] arm64: hugetlb: set_huge_pte_at Add WARN_ON on !pte_present

[Punit Agrawal]

From: Steve Capper <steve.capper@arm.com>

This patch adds a WARN_ON to set_huge_pte_at as the accessor assumes
that entries to be written down are all present. (There are separate
accessors to clear huge ptes).

We will need to handle the !pte_present case where memory offlining
is used on hugetlb pages. swap and migration entries will be supplied
to set_huge_pte_at in this case.

Cc: David Woods <dwoods@mellanox.com>
Signed-off-by: Steve Capper <steve.capper@arm.com>
Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
---
 arch/arm64/mm/hugetlbpage.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
index 656e0ece2289..7b61e4833432 100644
--- a/arch/arm64/mm/hugetlbpage.c
+++ b/arch/arm64/mm/hugetlbpage.c
@@ -67,6 +67,12 @@ void set_huge_pte_at(struct mm_struct *mm, unsigned long addr,
 	unsigned long pfn;
 	pgprot_t hugeprot;
 
+	/*
+	 * Code needs to be expanded to handle huge swap and migration
+	 * entries. Needed for HUGETLB and MEMORY_FAILURE.
+	 */
+	WARN_ON(!pte_present(pte));
+
 	if (!pte_cont(pte)) {
 		set_pte_at(mm, addr, ptep, pte);
 		return;
-- 
2.13.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v7 2/9] arm64: hugetlb: Introduce pte_pgprot helper

[Punit Agrawal]

From: Steve Capper <steve.capper@arm.com>

Rather than xor pte bits in various places, use this helper function.

Cc: David Woods <dwoods@mellanox.com>
Signed-off-by: Steve Capper <steve.capper@arm.com>
Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
---
 arch/arm64/mm/hugetlbpage.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
index 7b61e4833432..cb84ca33bc6b 100644
--- a/arch/arm64/mm/hugetlbpage.c
+++ b/arch/arm64/mm/hugetlbpage.c
@@ -41,6 +41,16 @@ int pud_huge(pud_t pud)
 #endif
 }
 
+/*
+ * Select all bits except the pfn
+ */
+static inline pgprot_t pte_pgprot(pte_t pte)
+{
+	unsigned long pfn = pte_pfn(pte);
+
+	return __pgprot(pte_val(pfn_pte(pfn, __pgprot(0))) ^ pte_val(pte));
+}
+
 static int find_num_contig(struct mm_struct *mm, unsigned long addr,
 			   pte_t *ptep, size_t *pgsize)
 {
@@ -80,7 +90,7 @@ void set_huge_pte_at(struct mm_struct *mm, unsigned long addr,
 
 	ncontig = find_num_contig(mm, addr, ptep, &pgsize);
 	pfn = pte_pfn(pte);
-	hugeprot = __pgprot(pte_val(pfn_pte(pfn, __pgprot(0))) ^ pte_val(pte));
+	hugeprot = pte_pgprot(pte);
 	for (i = 0; i < ncontig; i++) {
 		pr_debug("%s: set pte %p to 0x%llx\n", __func__, ptep,
 			 pte_val(pfn_pte(pfn, hugeprot)));
@@ -223,9 +233,7 @@ int huge_ptep_set_access_flags(struct vm_area_struct *vma,
 		size_t pgsize = 0;
 		unsigned long pfn = pte_pfn(pte);
 		/* Select all bits except the pfn */
-		pgprot_t hugeprot =
-			__pgprot(pte_val(pfn_pte(pfn, __pgprot(0))) ^
-				 pte_val(pte));
+		pgprot_t hugeprot = pte_pgprot(pte);
 
 		pfn = pte_pfn(pte);
 		ncontig = find_num_contig(vma->vm_mm, addr, ptep,
-- 
2.13.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v7 3/9] arm64: hugetlb: Spring clean huge pte accessors

[Punit Agrawal]

From: Steve Capper <steve.capper@arm.com>

This patch aims to re-structure the huge pte accessors without affecting
their functionality. Control flow is changed to reduce indentation and
expanded use is made of post for loop variable modification.

It is then much easier to add break-before-make semantics in a subsequent
patch.

Cc: David Woods <dwoods@mellanox.com>
Signed-off-by: Steve Capper <steve.capper@arm.com>
Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
---
 arch/arm64/mm/hugetlbpage.c | 119 ++++++++++++++++++++------------------------
 1 file changed, 54 insertions(+), 65 deletions(-)

diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
index cb84ca33bc6b..08deed7c71f0 100644
--- a/arch/arm64/mm/hugetlbpage.c
+++ b/arch/arm64/mm/hugetlbpage.c
@@ -74,7 +74,7 @@ void set_huge_pte_at(struct mm_struct *mm, unsigned long addr,
 	size_t pgsize;
 	int i;
 	int ncontig;
-	unsigned long pfn;
+	unsigned long pfn, dpfn;
 	pgprot_t hugeprot;
 
 	/*
@@ -90,14 +90,13 @@ void set_huge_pte_at(struct mm_struct *mm, unsigned long addr,
 
 	ncontig = find_num_contig(mm, addr, ptep, &pgsize);
 	pfn = pte_pfn(pte);
+	dpfn = pgsize >> PAGE_SHIFT;
 	hugeprot = pte_pgprot(pte);
-	for (i = 0; i < ncontig; i++) {
+
+	for (i = 0; i < ncontig; i++, ptep++, addr += pgsize, pfn += dpfn) {
 		pr_debug("%s: set pte %p to 0x%llx\n", __func__, ptep,
 			 pte_val(pfn_pte(pfn, hugeprot)));
 		set_pte_at(mm, addr, ptep, pfn_pte(pfn, hugeprot));
-		ptep++;
-		pfn += pgsize >> PAGE_SHIFT;
-		addr += pgsize;
 	}
 }
 
@@ -195,91 +194,81 @@ pte_t arch_make_huge_pte(pte_t entry, struct vm_area_struct *vma,
 pte_t huge_ptep_get_and_clear(struct mm_struct *mm,
 			      unsigned long addr, pte_t *ptep)
 {
-	pte_t pte;
-
-	if (pte_cont(*ptep)) {
-		int ncontig, i;
-		size_t pgsize;
-		bool is_dirty = false;
-
-		ncontig = find_num_contig(mm, addr, ptep, &pgsize);
-		/* save the 1st pte to return */
-		pte = ptep_get_and_clear(mm, addr, ptep);
-		for (i = 1, addr += pgsize; i < ncontig; ++i, addr += pgsize) {
-			/*
-			 * If HW_AFDBM is enabled, then the HW could
-			 * turn on the dirty bit for any of the page
-			 * in the set, so check them all.
-			 */
-			++ptep;
-			if (pte_dirty(ptep_get_and_clear(mm, addr, ptep)))
-				is_dirty = true;
-		}
-		if (is_dirty)
-			return pte_mkdirty(pte);
-		else
-			return pte;
-	} else {
+	int ncontig, i;
+	size_t pgsize;
+	pte_t orig_pte = huge_ptep_get(ptep);
+
+	if (!pte_cont(orig_pte))
 		return ptep_get_and_clear(mm, addr, ptep);
+
+	ncontig = find_num_contig(mm, addr, ptep, &pgsize);
+	for (i = 0; i < ncontig; i++, addr += pgsize, ptep++) {
+		/*
+		 * If HW_AFDBM is enabled, then the HW could
+		 * turn on the dirty bit for any of the page
+		 * in the set, so check them all.
+		 */
+		if (pte_dirty(ptep_get_and_clear(mm, addr, ptep)))
+			orig_pte = pte_mkdirty(orig_pte);
 	}
+
+	return orig_pte;
 }
 
 int huge_ptep_set_access_flags(struct vm_area_struct *vma,
 			       unsigned long addr, pte_t *ptep,
 			       pte_t pte, int dirty)
 {
-	if (pte_cont(pte)) {
-		int ncontig, i, changed = 0;
-		size_t pgsize = 0;
-		unsigned long pfn = pte_pfn(pte);
-		/* Select all bits except the pfn */
-		pgprot_t hugeprot = pte_pgprot(pte);
-
-		pfn = pte_pfn(pte);
-		ncontig = find_num_contig(vma->vm_mm, addr, ptep,
-					  &pgsize);
-		for (i = 0; i < ncontig; ++i, ++ptep, addr += pgsize) {
-			changed |= ptep_set_access_flags(vma, addr, ptep,
-							pfn_pte(pfn,
-								hugeprot),
-							dirty);
-			pfn += pgsize >> PAGE_SHIFT;
-		}
-		return changed;
-	} else {
+	int ncontig, i, changed = 0;
+	size_t pgsize = 0;
+	unsigned long pfn = pte_pfn(pte), dpfn;
+	pgprot_t hugeprot;
+
+	if (!pte_cont(pte))
 		return ptep_set_access_flags(vma, addr, ptep, pte, dirty);
+
+	ncontig = find_num_contig(vma->vm_mm, addr, ptep, &pgsize);
+	dpfn = pgsize >> PAGE_SHIFT;
+	hugeprot = pte_pgprot(pte);
+
+	for (i = 0; i < ncontig; i++, ptep++, addr += pgsize, pfn += dpfn) {
+		changed |= ptep_set_access_flags(vma, addr, ptep,
+				pfn_pte(pfn, hugeprot), dirty);
 	}
+
+	return changed;
 }
 
 void huge_ptep_set_wrprotect(struct mm_struct *mm,
 			     unsigned long addr, pte_t *ptep)
 {
-	if (pte_cont(*ptep)) {
-		int ncontig, i;
-		size_t pgsize = 0;
+	int ncontig, i;
+	size_t pgsize;
 
-		ncontig = find_num_contig(mm, addr, ptep, &pgsize);
-		for (i = 0; i < ncontig; ++i, ++ptep, addr += pgsize)
-			ptep_set_wrprotect(mm, addr, ptep);
-	} else {
+	if (!pte_cont(*ptep)) {
 		ptep_set_wrprotect(mm, addr, ptep);
+		return;
 	}
+
+	ncontig = find_num_contig(mm, addr, ptep, &pgsize);
+	for (i = 0; i < ncontig; i++, ptep++, addr += pgsize)
+		ptep_set_wrprotect(mm, addr, ptep);
 }
 
 void huge_ptep_clear_flush(struct vm_area_struct *vma,
 			   unsigned long addr, pte_t *ptep)
 {
-	if (pte_cont(*ptep)) {
-		int ncontig, i;
-		size_t pgsize = 0;
-
-		ncontig = find_num_contig(vma->vm_mm, addr, ptep,
-					  &pgsize);
-		for (i = 0; i < ncontig; ++i, ++ptep, addr += pgsize)
-			ptep_clear_flush(vma, addr, ptep);
-	} else {
+	int ncontig, i;
+	size_t pgsize;
+
+	if (!pte_cont(*ptep)) {
 		ptep_clear_flush(vma, addr, ptep);
+		return;
 	}
+
+	ncontig = find_num_contig(vma->vm_mm, addr, ptep, &pgsize);
+	for (i = 0; i < ncontig; i++, ptep++, addr += pgsize)
+		ptep_clear_flush(vma, addr, ptep);
 }
 
 static __init int setup_hugepagesz(char *opt)
-- 
2.13.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v7 4/9] arm64: hugetlb: Add break-before-make logic for contiguous entries

[Punit Agrawal]

From: Steve Capper <steve.capper@arm.com>

It has become apparent that one has to take special care when modifying
attributes of memory mappings that employ the contiguous bit.

Both the requirement and the architecturally correct "Break-Before-Make"
technique of updating contiguous entries can be found described in:
ARM DDI 0487A.k_iss10775, "Misprogramming of the Contiguous bit",
page D4-1762.

The huge pte accessors currently replace the attributes of contiguous
pte entries in place thus can, on certain platforms, lead to TLB
conflict aborts or even erroneous results returned from TLB lookups.

This patch adds two helper functions -

* get_clear_flush(.) - clears a contiguous entry and returns the head
  pte (whilst taking care to retain dirty bit information that could
  have been modified by DBM).

* clear_flush(.) that clears a contiguous entry

A tlb invalidate is performed to then ensure that there is no
possibility of multiple tlb entries being present for the same region.

Cc: David Woods <dwoods@mellanox.com>
Signed-off-by: Steve Capper <steve.capper@arm.com>
(Added helper clear_flush(), updated commit log, and some cleanup)
Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
---
 arch/arm64/mm/hugetlbpage.c | 112 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 91 insertions(+), 21 deletions(-)

diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
index 08deed7c71f0..594232598cac 100644
--- a/arch/arm64/mm/hugetlbpage.c
+++ b/arch/arm64/mm/hugetlbpage.c
@@ -68,6 +68,66 @@ static int find_num_contig(struct mm_struct *mm, unsigned long addr,
 	return CONT_PTES;
 }
 
+/*
+ * Changing some bits of contiguous entries requires us to follow a
+ * Break-Before-Make approach, breaking the whole contiguous set
+ * before we can change any entries. See ARM DDI 0487A.k_iss10775,
+ * "Misprogramming of the Contiguous bit", page D4-1762.
+ *
+ * This helper performs the break step.
+ */
+static pte_t get_clear_flush(struct mm_struct *mm,
+			     unsigned long addr,
+			     pte_t *ptep,
+			     unsigned long pgsize,
+			     unsigned long ncontig)
+{
+	struct vm_area_struct vma = { .vm_mm = mm };
+	pte_t orig_pte = huge_ptep_get(ptep);
+	bool valid = pte_valid(orig_pte);
+	unsigned long i, saddr = addr;
+
+	for (i = 0; i < ncontig; i++, addr += pgsize, ptep++) {
+		pte_t pte = ptep_get_and_clear(mm, addr, ptep);
+
+		/*
+		 * If HW_AFDBM is enabled, then the HW could turn on
+		 * the dirty bit for any page in the set, so check
+		 * them all.  All hugetlb entries are already young.
+		 */
+		if (IS_ENABLED(CONFIG_ARM64_HW_AFDBM) && pte_dirty(pte))
+			orig_pte = pte_mkdirty(orig_pte);
+	}
+
+	if (valid)
+		flush_tlb_range(&vma, saddr, addr);
+	return orig_pte;
+}
+
+/*
+ * Changing some bits of contiguous entries requires us to follow a
+ * Break-Before-Make approach, breaking the whole contiguous set
+ * before we can change any entries. See ARM DDI 0487A.k_iss10775,
+ * "Misprogramming of the Contiguous bit", page D4-1762.
+ *
+ * This helper performs the break step for use cases where the
+ * original pte is not needed.
+ */
+static void clear_flush(struct mm_struct *mm,
+			     unsigned long addr,
+			     pte_t *ptep,
+			     unsigned long pgsize,
+			     unsigned long ncontig)
+{
+	struct vm_area_struct vma = { .vm_mm = mm };
+	unsigned long i, saddr = addr;
+
+	for (i = 0; i < ncontig; i++, addr += pgsize, ptep++)
+		pte_clear(mm, addr, ptep);
+
+	flush_tlb_range(&vma, saddr, addr);
+}
+
 void set_huge_pte_at(struct mm_struct *mm, unsigned long addr,
 			    pte_t *ptep, pte_t pte)
 {
@@ -93,6 +153,8 @@ void set_huge_pte_at(struct mm_struct *mm, unsigned long addr,
 	dpfn = pgsize >> PAGE_SHIFT;
 	hugeprot = pte_pgprot(pte);
 
+	clear_flush(mm, addr, ptep, pgsize, ncontig);
+
 	for (i = 0; i < ncontig; i++, ptep++, addr += pgsize, pfn += dpfn) {
 		pr_debug("%s: set pte %p to 0x%llx\n", __func__, ptep,
 			 pte_val(pfn_pte(pfn, hugeprot)));
@@ -194,7 +256,7 @@ pte_t arch_make_huge_pte(pte_t entry, struct vm_area_struct *vma,
 pte_t huge_ptep_get_and_clear(struct mm_struct *mm,
 			      unsigned long addr, pte_t *ptep)
 {
-	int ncontig, i;
+	int ncontig;
 	size_t pgsize;
 	pte_t orig_pte = huge_ptep_get(ptep);
 
@@ -202,17 +264,8 @@ pte_t huge_ptep_get_and_clear(struct mm_struct *mm,
 		return ptep_get_and_clear(mm, addr, ptep);
 
 	ncontig = find_num_contig(mm, addr, ptep, &pgsize);
-	for (i = 0; i < ncontig; i++, addr += pgsize, ptep++) {
-		/*
-		 * If HW_AFDBM is enabled, then the HW could
-		 * turn on the dirty bit for any of the page
-		 * in the set, so check them all.
-		 */
-		if (pte_dirty(ptep_get_and_clear(mm, addr, ptep)))
-			orig_pte = pte_mkdirty(orig_pte);
-	}
 
-	return orig_pte;
+	return get_clear_flush(mm, addr, ptep, pgsize, ncontig);
 }
 
 int huge_ptep_set_access_flags(struct vm_area_struct *vma,
@@ -223,18 +276,25 @@ int huge_ptep_set_access_flags(struct vm_area_struct *vma,
 	size_t pgsize = 0;
 	unsigned long pfn = pte_pfn(pte), dpfn;
 	pgprot_t hugeprot;
+	pte_t orig_pte;
 
 	if (!pte_cont(pte))
 		return ptep_set_access_flags(vma, addr, ptep, pte, dirty);
 
 	ncontig = find_num_contig(vma->vm_mm, addr, ptep, &pgsize);
 	dpfn = pgsize >> PAGE_SHIFT;
-	hugeprot = pte_pgprot(pte);
 
-	for (i = 0; i < ncontig; i++, ptep++, addr += pgsize, pfn += dpfn) {
-		changed |= ptep_set_access_flags(vma, addr, ptep,
-				pfn_pte(pfn, hugeprot), dirty);
-	}
+	orig_pte = get_clear_flush(vma->vm_mm, addr, ptep, pgsize, ncontig);
+	if (!pte_same(orig_pte, pte))
+		changed = 1;
+
+	/* Make sure we don't lose the dirty state */
+	if (pte_dirty(orig_pte))
+		pte = pte_mkdirty(pte);
+
+	hugeprot = pte_pgprot(pte);
+	for (i = 0; i < ncontig; i++, ptep++, addr += pgsize, pfn += dpfn)
+		set_pte_at(vma->vm_mm, addr, ptep, pfn_pte(pfn, hugeprot));
 
 	return changed;
 }
@@ -242,8 +302,11 @@ int huge_ptep_set_access_flags(struct vm_area_struct *vma,
 void huge_ptep_set_wrprotect(struct mm_struct *mm,
 			     unsigned long addr, pte_t *ptep)
 {
+	unsigned long pfn, dpfn;
+	pgprot_t hugeprot;
 	int ncontig, i;
 	size_t pgsize;
+	pte_t pte;
 
 	if (!pte_cont(*ptep)) {
 		ptep_set_wrprotect(mm, addr, ptep);
@@ -251,15 +314,23 @@ void huge_ptep_set_wrprotect(struct mm_struct *mm,
 	}
 
 	ncontig = find_num_contig(mm, addr, ptep, &pgsize);
-	for (i = 0; i < ncontig; i++, ptep++, addr += pgsize)
-		ptep_set_wrprotect(mm, addr, ptep);
+	dpfn = pgsize >> PAGE_SHIFT;
+
+	pte = get_clear_flush(mm, addr, ptep, pgsize, ncontig);
+	pte = pte_wrprotect(pte);
+
+	hugeprot = pte_pgprot(pte);
+	pfn = pte_pfn(pte);
+
+	for (i = 0; i < ncontig; i++, ptep++, addr += pgsize, pfn += dpfn)
+		set_pte_at(mm, addr, ptep, pfn_pte(pfn, hugeprot));
 }
 
 void huge_ptep_clear_flush(struct vm_area_struct *vma,
 			   unsigned long addr, pte_t *ptep)
 {
-	int ncontig, i;
 	size_t pgsize;
+	int ncontig;
 
 	if (!pte_cont(*ptep)) {
 		ptep_clear_flush(vma, addr, ptep);
@@ -267,8 +338,7 @@ void huge_ptep_clear_flush(struct vm_area_struct *vma,
 	}
 
 	ncontig = find_num_contig(vma->vm_mm, addr, ptep, &pgsize);
-	for (i = 0; i < ncontig; i++, ptep++, addr += pgsize)
-		ptep_clear_flush(vma, addr, ptep);
+	clear_flush(vma->vm_mm, addr, ptep, pgsize, ncontig);
 }
 
 static __init int setup_hugepagesz(char *opt)
-- 
2.13.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v7 5/9] arm64: hugetlb: Handle swap entries in huge_pte_offset() for contiguous hugepages

[Punit Agrawal]

huge_pte_offset() was updated to correctly handle swap entries for
hugepages. With the addition of the size parameter, it is now possible
to disambiguate whether the request is for a regular hugepage or a
contiguous hugepage.

Fix huge_pte_offset() for contiguous hugepages by using the size to find
the correct page table entry.

Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
Cc: David Woods <dwoods@mellanox.com>
---
 arch/arm64/mm/hugetlbpage.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
index 594232598cac..b95e24dc3477 100644
--- a/arch/arm64/mm/hugetlbpage.c
+++ b/arch/arm64/mm/hugetlbpage.c
@@ -214,6 +214,7 @@ pte_t *huge_pte_offset(struct mm_struct *mm,
 	pgd_t *pgd;
 	pud_t *pud;
 	pmd_t *pmd;
+	pte_t *pte;
 
 	pgd = pgd_offset(mm, addr);
 	pr_debug("%s: addr:0x%lx pgd:%p\n", __func__, addr, pgd);
@@ -221,19 +222,29 @@ pte_t *huge_pte_offset(struct mm_struct *mm,
 		return NULL;
 
 	pud = pud_offset(pgd, addr);
-	if (pud_none(*pud))
+	if (sz != PUD_SIZE && pud_none(*pud))
 		return NULL;
-	/* swap or huge page */
-	if (!pud_present(*pud) || pud_huge(*pud))
+	/* hugepage or swap? */
+	if (pud_huge(*pud) || !pud_present(*pud))
 		return (pte_t *)pud;
 	/* table; check the next level */
 
+	if (sz == CONT_PMD_SIZE)
+		addr &= CONT_PMD_MASK;
+
 	pmd = pmd_offset(pud, addr);
-	if (pmd_none(*pmd))
+	if (!(sz == PMD_SIZE || sz == CONT_PMD_SIZE) &&
+	    pmd_none(*pmd))
 		return NULL;
-	if (!pmd_present(*pmd) || pmd_huge(*pmd))
+	if (pmd_huge(*pmd) || !pmd_present(*pmd))
 		return (pte_t *)pmd;
 
+	if (sz == CONT_PTE_SIZE) {
+		pte = pte_offset_kernel(
+			pmd, (addr & CONT_PTE_MASK));
+		return pte;
+	}
+
 	return NULL;
 }
 
-- 
2.13.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v7 6/9] arm64: hugetlb: Override huge_pte_clear() to support contiguous hugepages

[Punit Agrawal]

The default huge_pte_clear() implementation does not clear contiguous
page table entries when it encounters contiguous hugepages that are
supported on arm64.

Fix this by overriding the default implementation to clear all the
entries associated with contiguous hugepages.

Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
Cc: David Woods <dwoods@mellanox.com>
---
 arch/arm64/include/asm/hugetlb.h |  6 +++++-
 arch/arm64/mm/hugetlbpage.c      | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/hugetlb.h b/arch/arm64/include/asm/hugetlb.h
index 793bd73b0d07..df8c0aea0917 100644
--- a/arch/arm64/include/asm/hugetlb.h
+++ b/arch/arm64/include/asm/hugetlb.h
@@ -18,7 +18,6 @@
 #ifndef __ASM_HUGETLB_H
 #define __ASM_HUGETLB_H
 
-#include <asm-generic/hugetlb.h>
 #include <asm/page.h>
 
 static inline pte_t huge_ptep_get(pte_t *ptep)
@@ -82,6 +81,11 @@ extern void huge_ptep_set_wrprotect(struct mm_struct *mm,
 				    unsigned long addr, pte_t *ptep);
 extern void huge_ptep_clear_flush(struct vm_area_struct *vma,
 				  unsigned long addr, pte_t *ptep);
+extern void huge_pte_clear(struct mm_struct *mm, unsigned long addr,
+			   pte_t *ptep, unsigned long sz);
+#define huge_pte_clear huge_pte_clear
+
+#include <asm-generic/hugetlb.h>
 
 #ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE
 static inline bool gigantic_page_supported(void) { return true; }
diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
index b95e24dc3477..13acc9b47f51 100644
--- a/arch/arm64/mm/hugetlbpage.c
+++ b/arch/arm64/mm/hugetlbpage.c
@@ -68,6 +68,32 @@ static int find_num_contig(struct mm_struct *mm, unsigned long addr,
 	return CONT_PTES;
 }
 
+static inline int num_contig_ptes(unsigned long size, size_t *pgsize)
+{
+	int contig_ptes = 0;
+
+	*pgsize = size;
+
+	switch (size) {
+#ifdef CONFIG_ARM64_4K_PAGES
+	case PUD_SIZE:
+#endif
+	case PMD_SIZE:
+		contig_ptes = 1;
+		break;
+	case CONT_PMD_SIZE:
+		*pgsize = PMD_SIZE;
+		contig_ptes = CONT_PMDS;
+		break;
+	case CONT_PTE_SIZE:
+		*pgsize = PAGE_SIZE;
+		contig_ptes = CONT_PTES;
+		break;
+	}
+
+	return contig_ptes;
+}
+
 /*
  * Changing some bits of contiguous entries requires us to follow a
  * Break-Before-Make approach, breaking the whole contiguous set
@@ -264,6 +290,18 @@ pte_t arch_make_huge_pte(pte_t entry, struct vm_area_struct *vma,
 	return entry;
 }
 
+void huge_pte_clear(struct mm_struct *mm, unsigned long addr,
+		    pte_t *ptep, unsigned long sz)
+{
+	int i, ncontig;
+	size_t pgsize;
+
+	ncontig = num_contig_ptes(sz, &pgsize);
+
+	for (i = 0; i < ncontig; i++, addr += pgsize, ptep++)
+		pte_clear(mm, addr, ptep);
+}
+
 pte_t huge_ptep_get_and_clear(struct mm_struct *mm,
 			      unsigned long addr, pte_t *ptep)
 {
-- 
2.13.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v7 7/9] arm64: hugetlb: Override set_huge_swap_pte_at() to support contiguous hugepages

[Punit Agrawal]

The default implementation of set_huge_swap_pte_at() does not support
hugepages consisting of contiguous ptes. Override it to add support for
contiguous hugepages.

Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
Cc: David Woods <dwoods@mellanox.com>
---
 arch/arm64/include/asm/hugetlb.h |  3 +++
 arch/arm64/mm/hugetlbpage.c      | 12 ++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/arch/arm64/include/asm/hugetlb.h b/arch/arm64/include/asm/hugetlb.h
index df8c0aea0917..1dca41bea16a 100644
--- a/arch/arm64/include/asm/hugetlb.h
+++ b/arch/arm64/include/asm/hugetlb.h
@@ -84,6 +84,9 @@ extern void huge_ptep_clear_flush(struct vm_area_struct *vma,
 extern void huge_pte_clear(struct mm_struct *mm, unsigned long addr,
 			   pte_t *ptep, unsigned long sz);
 #define huge_pte_clear huge_pte_clear
+extern void set_huge_swap_pte_at(struct mm_struct *mm, unsigned long addr,
+				 pte_t *ptep, pte_t pte, unsigned long sz);
+#define set_huge_swap_pte_at set_huge_swap_pte_at
 
 #include <asm-generic/hugetlb.h>
 
diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
index 13acc9b47f51..01f0d7e8a8d8 100644
--- a/arch/arm64/mm/hugetlbpage.c
+++ b/arch/arm64/mm/hugetlbpage.c
@@ -188,6 +188,18 @@ void set_huge_pte_at(struct mm_struct *mm, unsigned long addr,
 	}
 }
 
+void set_huge_swap_pte_at(struct mm_struct *mm, unsigned long addr,
+			  pte_t *ptep, pte_t pte, unsigned long sz)
+{
+	int i, ncontig;
+	size_t pgsize;
+
+	ncontig = num_contig_ptes(sz, &pgsize);
+
+	for (i = 0; i < ncontig; i++, ptep++)
+		set_pte(ptep, pte);
+}
+
 pte_t *huge_pte_alloc(struct mm_struct *mm,
 		      unsigned long addr, unsigned long sz)
 {
-- 
2.13.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v7 8/9] arm64: Re-enable support for contiguous hugepages

[Punit Agrawal]

also known as -

Revert "Revert "Revert "commit 66b3923a1a0f ("arm64: hugetlb: add
support for PTE contiguous bit")"""

Now that our hugetlb implementation is compliant with the
break-before-make requirements of the architecture and we have addressed
some of the issues in core code required for properly dealing with
hardware poisoning of contiguous hugepages let's re-enable support for
contiguous hugepages.

This reverts commit 6ae979ab39a368c18ceb0424bf824d172d6ab56f.

Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
---
 arch/arm64/mm/hugetlbpage.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
index 01f0d7e8a8d8..b5e9c5b5e897 100644
--- a/arch/arm64/mm/hugetlbpage.c
+++ b/arch/arm64/mm/hugetlbpage.c
@@ -410,6 +410,10 @@ static __init int setup_hugepagesz(char *opt)
 		hugetlb_add_hstate(PMD_SHIFT - PAGE_SHIFT);
 	} else if (ps == PUD_SIZE) {
 		hugetlb_add_hstate(PUD_SHIFT - PAGE_SHIFT);
+	} else if (ps == (PAGE_SIZE * CONT_PTES)) {
+		hugetlb_add_hstate(CONT_PTE_SHIFT);
+	} else if (ps == (PMD_SIZE * CONT_PMDS)) {
+		hugetlb_add_hstate((PMD_SHIFT + CONT_PMD_SHIFT) - PAGE_SHIFT);
 	} else {
 		hugetlb_bad_size();
 		pr_err("hugepagesz: Unsupported page size %lu K\n", ps >> 10);
@@ -418,3 +422,13 @@ static __init int setup_hugepagesz(char *opt)
 	return 1;
 }
 __setup("hugepagesz=", setup_hugepagesz);
+
+#ifdef CONFIG_ARM64_64K_PAGES
+static __init int add_default_hugepagesz(void)
+{
+	if (size_to_hstate(CONT_PTES * PAGE_SIZE) == NULL)
+		hugetlb_add_hstate(CONT_PTE_SHIFT);
+	return 0;
+}
+arch_initcall(add_default_hugepagesz);
+#endif
-- 
2.13.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v7 9/9] arm64: hugetlb: Cleanup setup_hugepagesz

[Punit Agrawal]

From: Steve Capper <steve.capper@arm.com>

Replace a lot of if statements with switch and case labels to make it
much clearer which huge page sizes are supported.

Also, we prevent PUD_SIZE from being used on systems not running with
4KB PAGE_SIZE. Before if one supplied PUD_SIZE in these circumstances,
then unusuable huge page sizes would be in use.

Fixes: 084bd29810a5 ("ARM64: mm: HugeTLB support.")
Cc: David Woods <dwoods@mellanox.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Steve Capper <steve.capper@arm.com>
Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
---
 arch/arm64/mm/hugetlbpage.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
index b5e9c5b5e897..19f6604018dd 100644
--- a/arch/arm64/mm/hugetlbpage.c
+++ b/arch/arm64/mm/hugetlbpage.c
@@ -406,20 +406,20 @@ static __init int setup_hugepagesz(char *opt)
 {
 	unsigned long ps = memparse(opt, &opt);
 
-	if (ps == PMD_SIZE) {
-		hugetlb_add_hstate(PMD_SHIFT - PAGE_SHIFT);
-	} else if (ps == PUD_SIZE) {
-		hugetlb_add_hstate(PUD_SHIFT - PAGE_SHIFT);
-	} else if (ps == (PAGE_SIZE * CONT_PTES)) {
-		hugetlb_add_hstate(CONT_PTE_SHIFT);
-	} else if (ps == (PMD_SIZE * CONT_PMDS)) {
-		hugetlb_add_hstate((PMD_SHIFT + CONT_PMD_SHIFT) - PAGE_SHIFT);
-	} else {
-		hugetlb_bad_size();
-		pr_err("hugepagesz: Unsupported page size %lu K\n", ps >> 10);
-		return 0;
+	switch (ps) {
+#ifdef CONFIG_ARM64_4K_PAGES
+	case PUD_SIZE:
+#endif
+	case PMD_SIZE * CONT_PMDS:
+	case PMD_SIZE:
+	case PAGE_SIZE * CONT_PTES:
+		hugetlb_add_hstate(ilog2(ps) - PAGE_SHIFT);
+		return 1;
 	}
-	return 1;
+
+	hugetlb_bad_size();
+	pr_err("hugepagesz: Unsupported page size %lu K\n", ps >> 10);
+	return 0;
 }
 __setup("hugepagesz=", setup_hugepagesz);
 
-- 
2.13.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v7 5/9] arm64: hugetlb: Handle swap entries in huge_pte_offset() for contiguous hugepages

[Julien Thierry]

Hi Punit,

On 22/08/17 11:42, Punit Agrawal wrote:
> huge_pte_offset() was updated to correctly handle swap entries for
> hugepages. With the addition of the size parameter, it is now possible
> to disambiguate whether the request is for a regular hugepage or a
> contiguous hugepage.
> 
> Fix huge_pte_offset() for contiguous hugepages by using the size to find
> the correct page table entry.
> 
> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
> Cc: David Woods <dwoods@mellanox.com>
> ---
>   arch/arm64/mm/hugetlbpage.c | 21 ++++++++++++++++-----
>   1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
> index 594232598cac..b95e24dc3477 100644
> --- a/arch/arm64/mm/hugetlbpage.c
> +++ b/arch/arm64/mm/hugetlbpage.c
> @@ -214,6 +214,7 @@ pte_t *huge_pte_offset(struct mm_struct *mm,
>   	pgd_t *pgd;
>   	pud_t *pud;
>   	pmd_t *pmd;
> +	pte_t *pte;
>   
>   	pgd = pgd_offset(mm, addr);
>   	pr_debug("%s: addr:0x%lx pgd:%p\n", __func__, addr, pgd);
> @@ -221,19 +222,29 @@ pte_t *huge_pte_offset(struct mm_struct *mm,
>   		return NULL;
>   
>   	pud = pud_offset(pgd, addr);
> -	if (pud_none(*pud))
> +	if (sz != PUD_SIZE && pud_none(*pud))
>   		return NULL;
> -	/* swap or huge page */
> -	if (!pud_present(*pud) || pud_huge(*pud))
> +	/* hugepage or swap? */
> +	if (pud_huge(*pud) || !pud_present(*pud))
>   		return (pte_t *)pud;
>   	/* table; check the next level */
>   
> +	if (sz == CONT_PMD_SIZE)
> +		addr &= CONT_PMD_MASK;
> +
>   	pmd = pmd_offset(pud, addr);
> -	if (pmd_none(*pmd))
> +	if (!(sz == PMD_SIZE || sz == CONT_PMD_SIZE) &&
> +	    pmd_none(*pmd))
>   		return NULL;
> -	if (!pmd_present(*pmd) || pmd_huge(*pmd))
> +	if (pmd_huge(*pmd) || !pmd_present(*pmd))
>   		return (pte_t *)pmd;
>   
> +	if (sz == CONT_PTE_SIZE) {
> +		pte = pte_offset_kernel(
> +			pmd, (addr & CONT_PTE_MASK));
> +		return pte;

Nit: Looks like this is the only place the new variable pte is used. 
Since we don't need to test its value, why not just write:
	return pte_offset_kernel(pmd, (addr & CONT_PTE_MASK));

and get rid of the pte variable?

Cheers,

-- 
Julien Thierry

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v7 5/9] arm64: hugetlb: Handle swap entries in huge_pte_offset() for contiguous hugepages

[Punit Agrawal]

Julien Thierry <julien.thierry@arm.com> writes:

> Hi Punit,
>
> On 22/08/17 11:42, Punit Agrawal wrote:
>> huge_pte_offset() was updated to correctly handle swap entries for
>> hugepages. With the addition of the size parameter, it is now possible
>> to disambiguate whether the request is for a regular hugepage or a
>> contiguous hugepage.
>>
>> Fix huge_pte_offset() for contiguous hugepages by using the size to find
>> the correct page table entry.
>>
>> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
>> Cc: David Woods <dwoods@mellanox.com>
>> ---
>>   arch/arm64/mm/hugetlbpage.c | 21 ++++++++++++++++-----
>>   1 file changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
>> index 594232598cac..b95e24dc3477 100644
>> --- a/arch/arm64/mm/hugetlbpage.c
>> +++ b/arch/arm64/mm/hugetlbpage.c
>> @@ -214,6 +214,7 @@ pte_t *huge_pte_offset(struct mm_struct *mm,
>>   	pgd_t *pgd;
>>   	pud_t *pud;
>>   	pmd_t *pmd;
>> +	pte_t *pte;
>>     	pgd = pgd_offset(mm, addr);
>>   	pr_debug("%s: addr:0x%lx pgd:%p\n", __func__, addr, pgd);
>> @@ -221,19 +222,29 @@ pte_t *huge_pte_offset(struct mm_struct *mm,
>>   		return NULL;
>>     	pud = pud_offset(pgd, addr);
>> -	if (pud_none(*pud))
>> +	if (sz != PUD_SIZE && pud_none(*pud))
>>   		return NULL;
>> -	/* swap or huge page */
>> -	if (!pud_present(*pud) || pud_huge(*pud))
>> +	/* hugepage or swap? */
>> +	if (pud_huge(*pud) || !pud_present(*pud))
>>   		return (pte_t *)pud;
>>   	/* table; check the next level */
>>   +	if (sz == CONT_PMD_SIZE)
>> +		addr &= CONT_PMD_MASK;
>> +
>>   	pmd = pmd_offset(pud, addr);
>> -	if (pmd_none(*pmd))
>> +	if (!(sz == PMD_SIZE || sz == CONT_PMD_SIZE) &&
>> +	    pmd_none(*pmd))
>>   		return NULL;
>> -	if (!pmd_present(*pmd) || pmd_huge(*pmd))
>> +	if (pmd_huge(*pmd) || !pmd_present(*pmd))
>>   		return (pte_t *)pmd;
>>   +	if (sz == CONT_PTE_SIZE) {
>> +		pte = pte_offset_kernel(
>> +			pmd, (addr & CONT_PTE_MASK));
>> +		return pte;
>
> Nit: Looks like this is the only place the new variable pte is
> used. Since we don't need to test its value, why not just write:
> 	return pte_offset_kernel(pmd, (addr & CONT_PTE_MASK));
>
> and get rid of the pte variable?

There is no benefit to getting rid of "pte" other than conciseness of
the patch. Having an explicit identifier helps highlight the level of
the page tables we are accessing.

And we always want to prioritise readability vs conciseness of the
patch, no?

>
> Cheers,

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v7 5/9] arm64: hugetlb: Handle swap entries in huge_pte_offset() for contiguous hugepages

[Julien Thierry]

On 22/08/17 15:41, Punit Agrawal wrote:
> Julien Thierry <julien.thierry@arm.com> writes:
> 
>> Hi Punit,
>>
>> On 22/08/17 11:42, Punit Agrawal wrote:
>>> huge_pte_offset() was updated to correctly handle swap entries for
>>> hugepages. With the addition of the size parameter, it is now possible
>>> to disambiguate whether the request is for a regular hugepage or a
>>> contiguous hugepage.
>>>
>>> Fix huge_pte_offset() for contiguous hugepages by using the size to find
>>> the correct page table entry.
>>>
>>> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
>>> Cc: David Woods <dwoods@mellanox.com>
>>> ---
>>>    arch/arm64/mm/hugetlbpage.c | 21 ++++++++++++++++-----
>>>    1 file changed, 16 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
>>> index 594232598cac..b95e24dc3477 100644
>>> --- a/arch/arm64/mm/hugetlbpage.c
>>> +++ b/arch/arm64/mm/hugetlbpage.c
>>> @@ -214,6 +214,7 @@ pte_t *huge_pte_offset(struct mm_struct *mm,
>>>    	pgd_t *pgd;
>>>    	pud_t *pud;
>>>    	pmd_t *pmd;
>>> +	pte_t *pte;
>>>      	pgd = pgd_offset(mm, addr);
>>>    	pr_debug("%s: addr:0x%lx pgd:%p\n", __func__, addr, pgd);
>>> @@ -221,19 +222,29 @@ pte_t *huge_pte_offset(struct mm_struct *mm,
>>>    		return NULL;
>>>      	pud = pud_offset(pgd, addr);
>>> -	if (pud_none(*pud))
>>> +	if (sz != PUD_SIZE && pud_none(*pud))
>>>    		return NULL;
>>> -	/* swap or huge page */
>>> -	if (!pud_present(*pud) || pud_huge(*pud))
>>> +	/* hugepage or swap? */
>>> +	if (pud_huge(*pud) || !pud_present(*pud))
>>>    		return (pte_t *)pud;
>>>    	/* table; check the next level */
>>>    +	if (sz == CONT_PMD_SIZE)
>>> +		addr &= CONT_PMD_MASK;
>>> +
>>>    	pmd = pmd_offset(pud, addr);
>>> -	if (pmd_none(*pmd))
>>> +	if (!(sz == PMD_SIZE || sz == CONT_PMD_SIZE) &&
>>> +	    pmd_none(*pmd))
>>>    		return NULL;
>>> -	if (!pmd_present(*pmd) || pmd_huge(*pmd))
>>> +	if (pmd_huge(*pmd) || !pmd_present(*pmd))
>>>    		return (pte_t *)pmd;
>>>    +	if (sz == CONT_PTE_SIZE) {
>>> +		pte = pte_offset_kernel(
>>> +			pmd, (addr & CONT_PTE_MASK));
>>> +		return pte;
>>
>> Nit: Looks like this is the only place the new variable pte is
>> used. Since we don't need to test its value, why not just write:
>> 	return pte_offset_kernel(pmd, (addr & CONT_PTE_MASK));
>>
>> and get rid of the pte variable?
> 
> There is no benefit to getting rid of "pte" other than conciseness of
> the patch. Having an explicit identifier helps highlight the level of
> the page tables we are accessing.
> 
> And we always want to prioritise readability vs conciseness of the
> patch, no?
> 

I agree, but I feel here it is more redundancy than increase of 
readability, because we know pte_offset_kernel returns the address of a 
pte, no? (otherwise I feel a comment would fit better than a variable).

Also, we end up with a variable declared in one scope where it is not 
used, and it is referenced in a single inner scope, which seems a bit 
odd to me. Might make the reader pointlessly wonder where else it is used.

-- 
Julien Thierry

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v7 5/9] arm64: hugetlb: Handle swap entries in huge_pte_offset() for contiguous hugepages

[Punit Agrawal]

Julien Thierry <julien.thierry@arm.com> writes:

> On 22/08/17 15:41, Punit Agrawal wrote:
>> Julien Thierry <julien.thierry@arm.com> writes:
>>
>>> Hi Punit,
>>>
>>> On 22/08/17 11:42, Punit Agrawal wrote:
>>>> huge_pte_offset() was updated to correctly handle swap entries for
>>>> hugepages. With the addition of the size parameter, it is now possible
>>>> to disambiguate whether the request is for a regular hugepage or a
>>>> contiguous hugepage.
>>>>
>>>> Fix huge_pte_offset() for contiguous hugepages by using the size to find
>>>> the correct page table entry.
>>>>
>>>> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
>>>> Cc: David Woods <dwoods@mellanox.com>
>>>> ---
>>>>    arch/arm64/mm/hugetlbpage.c | 21 ++++++++++++++++-----
>>>>    1 file changed, 16 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
>>>> index 594232598cac..b95e24dc3477 100644
>>>> --- a/arch/arm64/mm/hugetlbpage.c
>>>> +++ b/arch/arm64/mm/hugetlbpage.c
>>>> @@ -214,6 +214,7 @@ pte_t *huge_pte_offset(struct mm_struct *mm,
>>>>    	pgd_t *pgd;
>>>>    	pud_t *pud;
>>>>    	pmd_t *pmd;
>>>> +	pte_t *pte;
>>>>      	pgd = pgd_offset(mm, addr);
>>>>    	pr_debug("%s: addr:0x%lx pgd:%p\n", __func__, addr, pgd);
>>>> @@ -221,19 +222,29 @@ pte_t *huge_pte_offset(struct mm_struct *mm,
>>>>    		return NULL;
>>>>      	pud = pud_offset(pgd, addr);
>>>> -	if (pud_none(*pud))
>>>> +	if (sz != PUD_SIZE && pud_none(*pud))
>>>>    		return NULL;
>>>> -	/* swap or huge page */
>>>> -	if (!pud_present(*pud) || pud_huge(*pud))
>>>> +	/* hugepage or swap? */
>>>> +	if (pud_huge(*pud) || !pud_present(*pud))
>>>>    		return (pte_t *)pud;
>>>>    	/* table; check the next level */
>>>>    +	if (sz == CONT_PMD_SIZE)
>>>> +		addr &= CONT_PMD_MASK;
>>>> +
>>>>    	pmd = pmd_offset(pud, addr);
>>>> -	if (pmd_none(*pmd))
>>>> +	if (!(sz == PMD_SIZE || sz == CONT_PMD_SIZE) &&
>>>> +	    pmd_none(*pmd))
>>>>    		return NULL;
>>>> -	if (!pmd_present(*pmd) || pmd_huge(*pmd))
>>>> +	if (pmd_huge(*pmd) || !pmd_present(*pmd))
>>>>    		return (pte_t *)pmd;
>>>>    +	if (sz == CONT_PTE_SIZE) {
>>>> +		pte = pte_offset_kernel(
>>>> +			pmd, (addr & CONT_PTE_MASK));
>>>> +		return pte;
>>>
>>> Nit: Looks like this is the only place the new variable pte is
>>> used. Since we don't need to test its value, why not just write:
>>> 	return pte_offset_kernel(pmd, (addr & CONT_PTE_MASK));
>>>
>>> and get rid of the pte variable?
>>
>> There is no benefit to getting rid of "pte" other than conciseness of
>> the patch. Having an explicit identifier helps highlight the level of
>> the page tables we are accessing.
>>
>> And we always want to prioritise readability vs conciseness of the
>> patch, no?
>>
>
> I agree, but I feel here it is more redundancy than increase of
> readability, because we know pte_offset_kernel returns the address of
> a pte, no? (otherwise I feel a comment would fit better than a
> variable).
>
> Also, we end up with a variable declared in one scope where it is not
> used, and it is referenced in a single inner scope, which seems a bit
> odd to me. Might make the reader pointlessly wonder where else it is
> used.

I would've thought looking at the function makes the variable usage
quite clear. But I think at this stage we are disagreeing over personal
preferences rather than any real issues (imho) with the code.

If you feel strongly about this, I can update the code if there is a
need for another version. But I am reluctant to send a new version just
for this change.

Thanks,
Punit

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v7 5/9] arm64: hugetlb: Handle swap entries in huge_pte_offset() for contiguous hugepages

[Julien Thierry]

On 22/08/17 17:18, Punit Agrawal wrote:
> Julien Thierry <julien.thierry@arm.com> writes:
> 
>> On 22/08/17 15:41, Punit Agrawal wrote:
>>> Julien Thierry <julien.thierry@arm.com> writes:
>>>
>>>> Hi Punit,
>>>>
>>>> On 22/08/17 11:42, Punit Agrawal wrote:
>>>>> huge_pte_offset() was updated to correctly handle swap entries for
>>>>> hugepages. With the addition of the size parameter, it is now possible
>>>>> to disambiguate whether the request is for a regular hugepage or a
>>>>> contiguous hugepage.
>>>>>
>>>>> Fix huge_pte_offset() for contiguous hugepages by using the size to find
>>>>> the correct page table entry.
>>>>>
>>>>> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
>>>>> Cc: David Woods <dwoods@mellanox.com>
>>>>> ---
>>>>>     arch/arm64/mm/hugetlbpage.c | 21 ++++++++++++++++-----
>>>>>     1 file changed, 16 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
>>>>> index 594232598cac..b95e24dc3477 100644
>>>>> --- a/arch/arm64/mm/hugetlbpage.c
>>>>> +++ b/arch/arm64/mm/hugetlbpage.c
>>>>> @@ -214,6 +214,7 @@ pte_t *huge_pte_offset(struct mm_struct *mm,
>>>>>     	pgd_t *pgd;
>>>>>     	pud_t *pud;
>>>>>     	pmd_t *pmd;
>>>>> +	pte_t *pte;
>>>>>       	pgd = pgd_offset(mm, addr);
>>>>>     	pr_debug("%s: addr:0x%lx pgd:%p\n", __func__, addr, pgd);
>>>>> @@ -221,19 +222,29 @@ pte_t *huge_pte_offset(struct mm_struct *mm,
>>>>>     		return NULL;
>>>>>       	pud = pud_offset(pgd, addr);
>>>>> -	if (pud_none(*pud))
>>>>> +	if (sz != PUD_SIZE && pud_none(*pud))
>>>>>     		return NULL;
>>>>> -	/* swap or huge page */
>>>>> -	if (!pud_present(*pud) || pud_huge(*pud))
>>>>> +	/* hugepage or swap? */
>>>>> +	if (pud_huge(*pud) || !pud_present(*pud))
>>>>>     		return (pte_t *)pud;
>>>>>     	/* table; check the next level */
>>>>>     +	if (sz == CONT_PMD_SIZE)
>>>>> +		addr &= CONT_PMD_MASK;
>>>>> +
>>>>>     	pmd = pmd_offset(pud, addr);
>>>>> -	if (pmd_none(*pmd))
>>>>> +	if (!(sz == PMD_SIZE || sz == CONT_PMD_SIZE) &&
>>>>> +	    pmd_none(*pmd))
>>>>>     		return NULL;
>>>>> -	if (!pmd_present(*pmd) || pmd_huge(*pmd))
>>>>> +	if (pmd_huge(*pmd) || !pmd_present(*pmd))
>>>>>     		return (pte_t *)pmd;
>>>>>     +	if (sz == CONT_PTE_SIZE) {
>>>>> +		pte = pte_offset_kernel(
>>>>> +			pmd, (addr & CONT_PTE_MASK));
>>>>> +		return pte;
>>>>
>>>> Nit: Looks like this is the only place the new variable pte is
>>>> used. Since we don't need to test its value, why not just write:
>>>> 	return pte_offset_kernel(pmd, (addr & CONT_PTE_MASK));
>>>>
>>>> and get rid of the pte variable?
>>>
>>> There is no benefit to getting rid of "pte" other than conciseness of
>>> the patch. Having an explicit identifier helps highlight the level of
>>> the page tables we are accessing.
>>>
>>> And we always want to prioritise readability vs conciseness of the
>>> patch, no?
>>>
>>
>> I agree, but I feel here it is more redundancy than increase of
>> readability, because we know pte_offset_kernel returns the address of
>> a pte, no? (otherwise I feel a comment would fit better than a
>> variable).
>>
>> Also, we end up with a variable declared in one scope where it is not
>> used, and it is referenced in a single inner scope, which seems a bit
>> odd to me. Might make the reader pointlessly wonder where else it is
>> used.
> 
> I would've thought looking at the function makes the variable usage
> quite clear. But I think at this stage we are disagreeing over personal
> preferences rather than any real issues (imho) with the code.
> 
> If you feel strongly about this, I can update the code if there is a
> need for another version. But I am reluctant to send a new version just
> for this change.
> 

Fair enough, it was just nitpicking.

Rest of the patchset looks good from my understanding.

Cheers,

-- 
Julien Thierry

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v7 5/9] arm64: hugetlb: Handle swap entries in huge_pte_offset() for contiguous hugepages

[Catalin Marinas]

On Tue, Aug 22, 2017 at 11:42:45AM +0100, Punit Agrawal wrote:
> diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
> index 594232598cac..b95e24dc3477 100644
> --- a/arch/arm64/mm/hugetlbpage.c
> +++ b/arch/arm64/mm/hugetlbpage.c
> @@ -214,6 +214,7 @@ pte_t *huge_pte_offset(struct mm_struct *mm,
>  	pgd_t *pgd;
>  	pud_t *pud;
>  	pmd_t *pmd;
> +	pte_t *pte;
>  
>  	pgd = pgd_offset(mm, addr);
>  	pr_debug("%s: addr:0x%lx pgd:%p\n", __func__, addr, pgd);
> @@ -221,19 +222,29 @@ pte_t *huge_pte_offset(struct mm_struct *mm,
>  		return NULL;
>  
>  	pud = pud_offset(pgd, addr);
> -	if (pud_none(*pud))
> +	if (sz != PUD_SIZE && pud_none(*pud))
>  		return NULL;
> -	/* swap or huge page */
> -	if (!pud_present(*pud) || pud_huge(*pud))
> +	/* hugepage or swap? */
> +	if (pud_huge(*pud) || !pud_present(*pud))
>  		return (pte_t *)pud;
>  	/* table; check the next level */
>  
> +	if (sz == CONT_PMD_SIZE)
> +		addr &= CONT_PMD_MASK;
> +
>  	pmd = pmd_offset(pud, addr);
> -	if (pmd_none(*pmd))
> +	if (!(sz == PMD_SIZE || sz == CONT_PMD_SIZE) &&
> +	    pmd_none(*pmd))
>  		return NULL;
> -	if (!pmd_present(*pmd) || pmd_huge(*pmd))
> +	if (pmd_huge(*pmd) || !pmd_present(*pmd))
>  		return (pte_t *)pmd;
>  
> +	if (sz == CONT_PTE_SIZE) {
> +		pte = pte_offset_kernel(
> +			pmd, (addr & CONT_PTE_MASK));
> +		return pte;
> +	}
> +
>  	return NULL;
>  }

I merged the patch almost as is (with the pte variable but
pte_offset_kernel() arguments on the same line); the pte variable is a
minor personal preference, so I'm not going to argue either way ;)).

Anyway, I pulled the whole series for 4.14, though I'll run some tests
over the next day or so.

Thanks.

-- 
Catalin

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v7 5/9] arm64: hugetlb: Handle swap entries in huge_pte_offset() for contiguous hugepages

[Punit Agrawal]

Catalin Marinas <catalin.marinas@arm.com> writes:

> On Tue, Aug 22, 2017 at 11:42:45AM +0100, Punit Agrawal wrote:
>> diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
>> index 594232598cac..b95e24dc3477 100644
>> --- a/arch/arm64/mm/hugetlbpage.c
>> +++ b/arch/arm64/mm/hugetlbpage.c
>> @@ -214,6 +214,7 @@ pte_t *huge_pte_offset(struct mm_struct *mm,
>>  	pgd_t *pgd;
>>  	pud_t *pud;
>>  	pmd_t *pmd;
>> +	pte_t *pte;
>>  
>>  	pgd = pgd_offset(mm, addr);
>>  	pr_debug("%s: addr:0x%lx pgd:%p\n", __func__, addr, pgd);
>> @@ -221,19 +222,29 @@ pte_t *huge_pte_offset(struct mm_struct *mm,
>>  		return NULL;
>>  
>>  	pud = pud_offset(pgd, addr);
>> -	if (pud_none(*pud))
>> +	if (sz != PUD_SIZE && pud_none(*pud))
>>  		return NULL;
>> -	/* swap or huge page */
>> -	if (!pud_present(*pud) || pud_huge(*pud))
>> +	/* hugepage or swap? */
>> +	if (pud_huge(*pud) || !pud_present(*pud))
>>  		return (pte_t *)pud;
>>  	/* table; check the next level */
>>  
>> +	if (sz == CONT_PMD_SIZE)
>> +		addr &= CONT_PMD_MASK;
>> +
>>  	pmd = pmd_offset(pud, addr);
>> -	if (pmd_none(*pmd))
>> +	if (!(sz == PMD_SIZE || sz == CONT_PMD_SIZE) &&
>> +	    pmd_none(*pmd))
>>  		return NULL;
>> -	if (!pmd_present(*pmd) || pmd_huge(*pmd))
>> +	if (pmd_huge(*pmd) || !pmd_present(*pmd))
>>  		return (pte_t *)pmd;
>>  
>> +	if (sz == CONT_PTE_SIZE) {
>> +		pte = pte_offset_kernel(
>> +			pmd, (addr & CONT_PTE_MASK));
>> +		return pte;
>> +	}
>> +
>>  	return NULL;
>>  }
>
> I merged the patch almost as is (with the pte variable but
> pte_offset_kernel() arguments on the same line); the pte variable is a
> minor personal preference, so I'm not going to argue either way ;)).
>
> Anyway, I pulled the whole series for 4.14, though I'll run some tests
> over the next day or so.

Thanks for picking up the patches. Hopefully, there won't be a need for
another revert this time around. :)

>
> Thanks.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>