On 22/08/17 15:41, Punit Agrawal wrote:
Julien Thierry <julien.thierry@xxxxxxx> 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@xxxxxxx>
Cc: David Woods <dwoods@xxxxxxxxxxxx>
---
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@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
