[PATCH V2 0/6] arm64/mm: Enable accounting for page table pages

[PATCH V2 0/6] arm64/mm: Enable accounting for page table pages

[Anshuman Khandual]

Currently page table page allocation and release has multiple combinations
for user space and kernel mapping page tables. Not only this is vulnerable
for bad page state errors it also prevents accurate accounting for those
pages through NR_PAGETABLE. This patch series attempts to standardize all
(except PGD slab cache 'pdg_cache') page table page allocation and release
through pte_alloc_one()/pte_free() which in turn guarantees that all these
pages will go trough pgtable_page_[ctor|dtor] constructs. It also takes
care of some of the pages who might have been allocated early via memblock.

This enables memcg based accounting for user process page table pages via
__GFP_ACCOUNT. For now this also removes standard ptable page allocators
usage from KVM stage-2 as some of them use multi order allocation which is
not supported in a generic way.

While here, enable ARCH_ENABLE_SPLIT_PMD_PTLOCK which can improve memory
scalability on larger systems. This enablement was first proposed [1] by
Yu Zhao using generic pgtable_pmd_page_ctor() which does not take zone stat
and page state into account. Hence the current proposal tries to merge both
through an arm64 specific helper pgtable_pmd_page_ctor_workaround() which
should eventually be dropped once we fix generic functions.

[1] https://patchwork.kernel.org/patch/10813793/

Changes in V2:

- Created temporary page structures in __[pmd|pud]_free_tlb per Will
- Incorporated efi_mm while determining pgtable allocation flag per Will
- Dropped parts of commit message from patch 'arm64/mm: Make all page table pages'
- Dropped patch 'arm64/mm: Ensure PGD kmem cache object is actually allocated'
- Added new patch 'arm64/mm: Enable ARCH_ENABLE_SPLIT_PMD_PTLOCK'

V1: https://www.spinics.net/lists/arm-kernel/msg701954.html

Anshuman Khandual (6):
  KVM: ARM: Remove pgtable standard functions from stage-2 page tables
  arm64/mm: Make pgd_pgtable_alloc() call pte_alloc_one() always
  arm64/mm: Make all page table pages cycles through standard constructs
  arm64/mm: Call pgtable_page_dtor() for both PMD and PUD page table pages
  arm64/mm: Enable page table page accounting for user space
  arm64/mm: Enable ARCH_ENABLE_SPLIT_PMD_PTLOCK

 arch/arm/include/asm/stage2_pgtable.h   |  4 +-
 arch/arm64/Kconfig                      |  4 ++
 arch/arm64/include/asm/pgalloc.h        | 95 ++++++++++++++++++++++---
 arch/arm64/include/asm/stage2_pgtable.h |  4 +-
 arch/arm64/include/asm/tlb.h            |  6 ++
 arch/arm64/mm/mmu.c                     | 17 +++--
 arch/arm64/mm/pgd.c                     |  4 +-
 7 files changed, 115 insertions(+), 19 deletions(-)

-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V2 1/6] KVM: ARM: Remove pgtable standard functions from stage-2 page tables

[Anshuman Khandual]

ARM64 standard pgtable functions are going to use pgtable_page_[ctor|dtor]
constructs. Certain stage-2 page table allocations are multi order which
cannot be allocated through a generic pgtable function as it does not exist
right now. This prevents all pgtable allocations including multi order ones
in stage-2 from moving into new ARM64 (pgtable_page_[ctor|dtor]) pgtable
functions. Hence remove all generic pgtable allocation function dependency
from stage-2 page tables till there is one multi-order allocation function
available.

Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 arch/arm/include/asm/stage2_pgtable.h   | 4 ++--
 arch/arm64/include/asm/stage2_pgtable.h | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/include/asm/stage2_pgtable.h b/arch/arm/include/asm/stage2_pgtable.h
index de2089501b8b..417a3be00718 100644
--- a/arch/arm/include/asm/stage2_pgtable.h
+++ b/arch/arm/include/asm/stage2_pgtable.h
@@ -32,14 +32,14 @@
 #define stage2_pgd_present(kvm, pgd)		pgd_present(pgd)
 #define stage2_pgd_populate(kvm, pgd, pud)	pgd_populate(NULL, pgd, pud)
 #define stage2_pud_offset(kvm, pgd, address)	pud_offset(pgd, address)
-#define stage2_pud_free(kvm, pud)		pud_free(NULL, pud)
+#define stage2_pud_free(kvm, pud)		free_page((unsigned long)pud)
 
 #define stage2_pud_none(kvm, pud)		pud_none(pud)
 #define stage2_pud_clear(kvm, pud)		pud_clear(pud)
 #define stage2_pud_present(kvm, pud)		pud_present(pud)
 #define stage2_pud_populate(kvm, pud, pmd)	pud_populate(NULL, pud, pmd)
 #define stage2_pmd_offset(kvm, pud, address)	pmd_offset(pud, address)
-#define stage2_pmd_free(kvm, pmd)		pmd_free(NULL, pmd)
+#define stage2_pmd_free(kvm, pmd)		free_page((unsigned long)pmd)
 
 #define stage2_pud_huge(kvm, pud)		pud_huge(pud)
 
diff --git a/arch/arm64/include/asm/stage2_pgtable.h b/arch/arm64/include/asm/stage2_pgtable.h
index 5412fa40825e..915809e4ac32 100644
--- a/arch/arm64/include/asm/stage2_pgtable.h
+++ b/arch/arm64/include/asm/stage2_pgtable.h
@@ -119,7 +119,7 @@ static inline pud_t *stage2_pud_offset(struct kvm *kvm,
 static inline void stage2_pud_free(struct kvm *kvm, pud_t *pud)
 {
 	if (kvm_stage2_has_pud(kvm))
-		pud_free(NULL, pud);
+		free_page((unsigned long)pud);
 }
 
 static inline bool stage2_pud_table_empty(struct kvm *kvm, pud_t *pudp)
@@ -192,7 +192,7 @@ static inline pmd_t *stage2_pmd_offset(struct kvm *kvm,
 static inline void stage2_pmd_free(struct kvm *kvm, pmd_t *pmd)
 {
 	if (kvm_stage2_has_pmd(kvm))
-		pmd_free(NULL, pmd);
+		free_page((unsigned long)pmd);
 }
 
 static inline bool stage2_pud_huge(struct kvm *kvm, pud_t pud)
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V2 2/6] arm64/mm: Make pgd_pgtable_alloc() call pte_alloc_one() always

[Anshuman Khandual]

pgd_pgtable_alloc() provides page allocation function while creating all
levels of page table (PGD, PUD, CONT PMD etc) for various kernel mappings.
It calls __get_free_page() and initializes page with pagetable_page_ctor().
pte_alloc_one() already provides a standard interface for allocating a page
table page and initializes it with pagetable_page_ctor(). This removes the
redundancy and instead make it call pte_alloc_one() directly.

Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 arch/arm64/mm/mmu.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index b6f5aa52ac67..2dbd72319152 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -372,13 +372,22 @@ static void __create_pgd_mapping(pgd_t *pgdir, phys_addr_t phys,
 
 static phys_addr_t pgd_pgtable_alloc(void)
 {
-	void *ptr = (void *)__get_free_page(PGALLOC_GFP);
-	if (!ptr || !pgtable_page_ctor(virt_to_page(ptr)))
-		BUG();
+
+	pgtable_t ptr;
+
+	/*
+	 * pgd_pgtable_alloc() is called while creating kernel mappings
+	 * through __create_pgd_mapping() might not install it through
+	 * swapper_pg_dir (&init_mm). Even then init_mm is passed here
+	 * just to indicate that the allocation is kernel specific not
+	 * for the user space page tables.
+	 */
+	ptr = pte_alloc_one(&init_mm);
+	BUG_ON(!ptr);
 
 	/* Ensure the zeroed page is visible to the page table walker */
 	dsb(ishst);
-	return __pa(ptr);
+	return page_to_phys(ptr);
 }
 
 /*
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V2 3/6] arm64/mm: Make all page table pages cycles through standard constructs

[Anshuman Khandual]

Currently there are inconsistencies during page table page's allocation,
initialization and destruction. Not all those pages go through standard
constructors pgtable_page_ctor/pgtable_page_dtor which is very much
essential for proper initialization and accounting. Not going through
proper pgtable_page_ctor/pgtable_page_dtor cycle makes them vulnerable
for bad page state errors during page table tear down process.

This makes all levels of page table pages either for the user space or
for the kernel mappings go through these standard constructs. While
here pte_free() has been modified to accommodate pages which might not
have been allocated through buddy system but instead came from memblock
[1] directly during early boot process. Those pages must not go through
the pgtable_page_dtor() and instead are freed with free_reserved_page().

PGD based page table page which is allocated from 'pgd_cache' kmem cache
in certain situations (PGD_SIZE != PAGE_SIZE ) is excluded from being
cycled through pgtable_page_[ctor/dtor]. This is not a problem because
this slab never gets freed back to buddy allocator.

[1] Page table page allocation from memblock

(a) early_pgtable_alloc (passed with __create_pgd_mapping)
(b) vmemmap_pgd|pud_populate (vmemmap_populate)

Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 arch/arm64/include/asm/pgalloc.h | 42 ++++++++++++++++++++++++++------
 arch/arm64/mm/pgd.c              |  4 +--
 2 files changed, 36 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/include/asm/pgalloc.h b/arch/arm64/include/asm/pgalloc.h
index 52fa47c73bf0..076764f66fb0 100644
--- a/arch/arm64/include/asm/pgalloc.h
+++ b/arch/arm64/include/asm/pgalloc.h
@@ -29,17 +29,21 @@
 #define PGALLOC_GFP	(GFP_KERNEL | __GFP_ZERO)
 #define PGD_SIZE	(PTRS_PER_PGD * sizeof(pgd_t))
 
+static inline pgtable_t pte_alloc_one(struct mm_struct *mm);
+static inline unsigned long pte_alloc_one_virt(struct mm_struct *mm);
+static inline void pte_free(struct mm_struct *mm, pgtable_t pte);
+
 #if CONFIG_PGTABLE_LEVELS > 2
 
 static inline pmd_t *pmd_alloc_one(struct mm_struct *mm, unsigned long addr)
 {
-	return (pmd_t *)__get_free_page(PGALLOC_GFP);
+	return (pmd_t *)pte_alloc_one_virt(mm);
 }
 
 static inline void pmd_free(struct mm_struct *mm, pmd_t *pmdp)
 {
 	BUG_ON((unsigned long)pmdp & (PAGE_SIZE-1));
-	free_page((unsigned long)pmdp);
+	pte_free(mm, virt_to_page(pmdp));
 }
 
 static inline void __pud_populate(pud_t *pudp, phys_addr_t pmdp, pudval_t prot)
@@ -62,13 +66,13 @@ static inline void __pud_populate(pud_t *pudp, phys_addr_t pmdp, pudval_t prot)
 
 static inline pud_t *pud_alloc_one(struct mm_struct *mm, unsigned long addr)
 {
-	return (pud_t *)__get_free_page(PGALLOC_GFP);
+	return (pud_t *)pte_alloc_one_virt(mm);
 }
 
 static inline void pud_free(struct mm_struct *mm, pud_t *pudp)
 {
 	BUG_ON((unsigned long)pudp & (PAGE_SIZE-1));
-	free_page((unsigned long)pudp);
+	pte_free(mm, virt_to_page(pudp));
 }
 
 static inline void __pgd_populate(pgd_t *pgdp, phys_addr_t pudp, pgdval_t prot)
@@ -93,7 +97,7 @@ extern void pgd_free(struct mm_struct *mm, pgd_t *pgdp);
 static inline pte_t *
 pte_alloc_one_kernel(struct mm_struct *mm)
 {
-	return (pte_t *)__get_free_page(PGALLOC_GFP);
+	return (pte_t *)pte_alloc_one_virt(mm);
 }
 
 static inline pgtable_t
@@ -111,19 +115,41 @@ pte_alloc_one(struct mm_struct *mm)
 	return pte;
 }
 
+static inline unsigned long
+pte_alloc_one_virt(struct mm_struct *mm)
+{
+	pgtable_t ptr;
+
+	ptr = pte_alloc_one(mm);
+	if (!ptr)
+		return 0;
+
+	return (unsigned long) page_to_virt(ptr);
+}
+
 /*
  * Free a PTE table.
  */
 static inline void pte_free_kernel(struct mm_struct *mm, pte_t *ptep)
 {
 	if (ptep)
-		free_page((unsigned long)ptep);
+		pte_free(mm, virt_to_page(ptep));
 }
 
 static inline void pte_free(struct mm_struct *mm, pgtable_t pte)
 {
-	pgtable_page_dtor(pte);
-	__free_page(pte);
+	/*
+	 * Some page table pages might have come from memblock either
+	 * with vmemmap_alloc_block (during vmemmap_populate) or with
+	 * early_pgtable_alloc (during __create_pgd_mapping). These
+	 * pages should be freed with free_reserved_page() instead.
+	 */
+	if (PageReserved(pte)) {
+		free_reserved_page(pte);
+	} else {
+		pgtable_page_dtor(pte);
+		__free_page(pte);
+	}
 }
 
 static inline void __pmd_populate(pmd_t *pmdp, phys_addr_t ptep,
diff --git a/arch/arm64/mm/pgd.c b/arch/arm64/mm/pgd.c
index 289f9113a27a..ea588c8825e2 100644
--- a/arch/arm64/mm/pgd.c
+++ b/arch/arm64/mm/pgd.c
@@ -31,7 +31,7 @@ static struct kmem_cache *pgd_cache __ro_after_init;
 pgd_t *pgd_alloc(struct mm_struct *mm)
 {
 	if (PGD_SIZE == PAGE_SIZE)
-		return (pgd_t *)__get_free_page(PGALLOC_GFP);
+		return (pgd_t *)pte_alloc_one_virt(mm);
 	else
 		return kmem_cache_alloc(pgd_cache, PGALLOC_GFP);
 }
@@ -39,7 +39,7 @@ pgd_t *pgd_alloc(struct mm_struct *mm)
 void pgd_free(struct mm_struct *mm, pgd_t *pgd)
 {
 	if (PGD_SIZE == PAGE_SIZE)
-		free_page((unsigned long)pgd);
+		pte_free(mm, virt_to_page(pgd));
 	else
 		kmem_cache_free(pgd_cache, pgd);
 }
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V2 4/6] arm64/mm: Call pgtable_page_dtor() for both PMD and PUD page table pages

[Anshuman Khandual]

TLB flush during PMD and PUD level page table page tear down currently does
not call pgtable_page_dtor() like that for PTE page table pages. Now that
all the page table pages which are allocated from the buddy allocator must
have been initialized through pgtable_page_ctor() they must be freed back
only after being called into pgtable_page_dtor() destructor. Else these
pages might run into bad page state errors while getting freed into buddy
allocator.

Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
Acked-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/include/asm/tlb.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm64/include/asm/tlb.h b/arch/arm64/include/asm/tlb.h
index 106fdc951b6e..3ef2d7ffafee 100644
--- a/arch/arm64/include/asm/tlb.h
+++ b/arch/arm64/include/asm/tlb.h
@@ -62,6 +62,9 @@ static inline void __pte_free_tlb(struct mmu_gather *tlb, pgtable_t pte,
 static inline void __pmd_free_tlb(struct mmu_gather *tlb, pmd_t *pmdp,
 				  unsigned long addr)
 {
+	struct page *page = virt_to_page(pmdp);
+
+	pgtable_page_dtor(page);
 	tlb_remove_table(tlb, virt_to_page(pmdp));
 }
 #endif
@@ -70,6 +73,9 @@ static inline void __pmd_free_tlb(struct mmu_gather *tlb, pmd_t *pmdp,
 static inline void __pud_free_tlb(struct mmu_gather *tlb, pud_t *pudp,
 				  unsigned long addr)
 {
+	struct page *page = virt_to_page(pudp);
+
+	pgtable_page_dtor(page);
 	tlb_remove_table(tlb, virt_to_page(pudp));
 }
 #endif
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V2 5/6] arm64/mm: Enable page table page accounting for user space

[Anshuman Khandual]

Page table pages created for user space processes must be accounted against
their memory control groups if initialized. This just introduces a helper
function pgtable_gfp_flags() which returns PGALLOC_GFP for all kernel page
table page allocations but adds __GFP_ACCOUNT for user page table pages.

Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 arch/arm64/include/asm/pgalloc.h | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/pgalloc.h b/arch/arm64/include/asm/pgalloc.h
index 076764f66fb0..a02a4d1d967d 100644
--- a/arch/arm64/include/asm/pgalloc.h
+++ b/arch/arm64/include/asm/pgalloc.h
@@ -100,12 +100,24 @@ pte_alloc_one_kernel(struct mm_struct *mm)
 	return (pte_t *)pte_alloc_one_virt(mm);
 }
 
+extern struct mm_struct efi_mm;
+
+static inline gfp_t pgtable_gfp_flags(struct mm_struct *mm)
+{
+	if (unlikely((mm == &init_mm) || (mm == &efi_mm)))
+		return PGALLOC_GFP;
+	else
+		return PGALLOC_GFP | __GFP_ACCOUNT;
+}
+
 static inline pgtable_t
 pte_alloc_one(struct mm_struct *mm)
 {
 	struct page *pte;
+	gfp_t gfp;
 
-	pte = alloc_pages(PGALLOC_GFP, 0);
+	gfp = pgtable_gfp_flags(mm);
+	pte = alloc_pages(gfp, 0);
 	if (!pte)
 		return NULL;
 	if (!pgtable_page_ctor(pte)) {
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V2 6/6] arm64/mm: Enable ARCH_ENABLE_SPLIT_PMD_PTLOCK

[Anshuman Khandual]

Enabling ARCH_ENABLE_SPLIT_PMD_PTLOCK should help reduce lock contention on
larger systems when the page tables are being modified concurrently. This
moves locking granularity from mm_struct (mm->page_table_lock) to per pmd
page table lock (page->ptl).

Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 arch/arm64/Kconfig               |  4 +++
 arch/arm64/include/asm/pgalloc.h | 43 ++++++++++++++++++++++++++++++--
 2 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index a4168d366127..b909e5d3b951 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -285,6 +285,10 @@ config PGTABLE_LEVELS
 	default 3 if ARM64_16K_PAGES && ARM64_VA_BITS_47
 	default 4 if !ARM64_64K_PAGES && ARM64_VA_BITS_48
 
+config ARCH_ENABLE_SPLIT_PMD_PTLOCK
+	def_bool y
+	depends on HAVE_ARCH_TRANSPARENT_HUGEPAGE && (PGTABLE_LEVELS > 2)
+
 config ARCH_SUPPORTS_UPROBES
 	def_bool y
 
diff --git a/arch/arm64/include/asm/pgalloc.h b/arch/arm64/include/asm/pgalloc.h
index a02a4d1d967d..9e06238be9b6 100644
--- a/arch/arm64/include/asm/pgalloc.h
+++ b/arch/arm64/include/asm/pgalloc.h
@@ -35,15 +35,54 @@ static inline void pte_free(struct mm_struct *mm, pgtable_t pte);
 
 #if CONFIG_PGTABLE_LEVELS > 2
 
+/*
+ * FIXME: _workaround might not be the right suffix here but it does
+ * indicate the intent behind it. Generic pgtable_pmd_page_ctor/dtor
+ * constructs neither do account the PMD page towards NR_PAGETABLE
+ * nor do they update page state with the new page type PageTable.
+ * Ideally pgtable_pmd_page_ctor/dtor should have been just taking care
+ * of page->pmd_huge_pte when applicable along with what is already
+ * achieved with pgtable_page_ctor/dtor constructs. Unfortunately that
+ * is not the case currently and changing those generic mm constructs
+ * might impact other archs. For now lets do the right thing here and
+ * drop this when generic PMD constructs accommodate required changes.
+ */
+#if defined(CONFIG_TRANSPARENT_HUGEPAGE) && USE_SPLIT_PMD_PTLOCKS
+static inline void pgtable_pmd_page_ctor_workaround(struct page *page)
+{
+	page->pmd_huge_pte = NULL;
+}
+
+static inline void pgtable_pmd_page_dtor_workaround(struct page *page)
+{
+	VM_BUG_ON_PAGE(page->pmd_huge_pte, page);
+}
+#else
+static inline void pgtable_pmd_page_ctor_workaround(struct page *page) { }
+static inline void pgtable_pmd_page_dtor_workaround(struct page *page) { }
+#endif
+
 static inline pmd_t *pmd_alloc_one(struct mm_struct *mm, unsigned long addr)
 {
-	return (pmd_t *)pte_alloc_one_virt(mm);
+	pgtable_t ptr;
+
+	ptr = pte_alloc_one(mm);
+	if (!ptr)
+		return 0;
+
+	pgtable_pmd_page_ctor_workaround(ptr);
+	return (pmd_t *)page_to_virt(ptr);
 }
 
 static inline void pmd_free(struct mm_struct *mm, pmd_t *pmdp)
 {
+	struct page *page;
+
 	BUG_ON((unsigned long)pmdp & (PAGE_SIZE-1));
-	pte_free(mm, virt_to_page(pmdp));
+	page = virt_to_page(pmdp);
+
+	pgtable_pmd_page_dtor_workaround(page);
+	pte_free(mm, page);
 }
 
 static inline void __pud_populate(pud_t *pudp, phys_addr_t pmdp, pudval_t prot)
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 1/6] KVM: ARM: Remove pgtable standard functions from stage-2 page tables

[Mark Rutland]

Hi Anshuman,

On Mon, Feb 25, 2019 at 10:33:54AM +0530, Anshuman Khandual wrote:
> ARM64 standard pgtable functions are going to use pgtable_page_[ctor|dtor]
> constructs. Certain stage-2 page table allocations are multi order which
> cannot be allocated through a generic pgtable function as it does not exist
> right now. This prevents all pgtable allocations including multi order ones
> in stage-2 from moving into new ARM64 (pgtable_page_[ctor|dtor]) pgtable
> functions. Hence remove all generic pgtable allocation function dependency
> from stage-2 page tables till there is one multi-order allocation function
> available.

I'm a bit confused by this. Which allocations are multi-order?

Why does that prevent other allcoations from using the regular routines?

Thanks,
Mark.

> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
>  arch/arm/include/asm/stage2_pgtable.h   | 4 ++--
>  arch/arm64/include/asm/stage2_pgtable.h | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/include/asm/stage2_pgtable.h b/arch/arm/include/asm/stage2_pgtable.h
> index de2089501b8b..417a3be00718 100644
> --- a/arch/arm/include/asm/stage2_pgtable.h
> +++ b/arch/arm/include/asm/stage2_pgtable.h
> @@ -32,14 +32,14 @@
>  #define stage2_pgd_present(kvm, pgd)		pgd_present(pgd)
>  #define stage2_pgd_populate(kvm, pgd, pud)	pgd_populate(NULL, pgd, pud)
>  #define stage2_pud_offset(kvm, pgd, address)	pud_offset(pgd, address)
> -#define stage2_pud_free(kvm, pud)		pud_free(NULL, pud)
> +#define stage2_pud_free(kvm, pud)		free_page((unsigned long)pud)
>  
>  #define stage2_pud_none(kvm, pud)		pud_none(pud)
>  #define stage2_pud_clear(kvm, pud)		pud_clear(pud)
>  #define stage2_pud_present(kvm, pud)		pud_present(pud)
>  #define stage2_pud_populate(kvm, pud, pmd)	pud_populate(NULL, pud, pmd)
>  #define stage2_pmd_offset(kvm, pud, address)	pmd_offset(pud, address)
> -#define stage2_pmd_free(kvm, pmd)		pmd_free(NULL, pmd)
> +#define stage2_pmd_free(kvm, pmd)		free_page((unsigned long)pmd)
>  
>  #define stage2_pud_huge(kvm, pud)		pud_huge(pud)
>  
> diff --git a/arch/arm64/include/asm/stage2_pgtable.h b/arch/arm64/include/asm/stage2_pgtable.h
> index 5412fa40825e..915809e4ac32 100644
> --- a/arch/arm64/include/asm/stage2_pgtable.h
> +++ b/arch/arm64/include/asm/stage2_pgtable.h
> @@ -119,7 +119,7 @@ static inline pud_t *stage2_pud_offset(struct kvm *kvm,
>  static inline void stage2_pud_free(struct kvm *kvm, pud_t *pud)
>  {
>  	if (kvm_stage2_has_pud(kvm))
> -		pud_free(NULL, pud);
> +		free_page((unsigned long)pud);
>  }
>  
>  static inline bool stage2_pud_table_empty(struct kvm *kvm, pud_t *pudp)
> @@ -192,7 +192,7 @@ static inline pmd_t *stage2_pmd_offset(struct kvm *kvm,
>  static inline void stage2_pmd_free(struct kvm *kvm, pmd_t *pmd)
>  {
>  	if (kvm_stage2_has_pmd(kvm))
> -		pmd_free(NULL, pmd);
> +		free_page((unsigned long)pmd);
>  }
>  
>  static inline bool stage2_pud_huge(struct kvm *kvm, pud_t pud)
> -- 
> 2.20.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 2/6] arm64/mm: Make pgd_pgtable_alloc() call pte_alloc_one() always

[Mark Rutland]

On Mon, Feb 25, 2019 at 10:33:55AM +0530, Anshuman Khandual wrote:
> pgd_pgtable_alloc() provides page allocation function while creating all
> levels of page table (PGD, PUD, CONT PMD etc) for various kernel mappings.
> It calls __get_free_page() and initializes page with pagetable_page_ctor().
> pte_alloc_one() already provides a standard interface for allocating a page
> table page and initializes it with pagetable_page_ctor(). This removes the
> redundancy and instead make it call pte_alloc_one() directly.

I see we also have pte_alloc_one_kernel(), which does not call
pgtable_page_ctor().

Was it deliberate that we don't call the ctor on kernel mappings?

> 
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
>  arch/arm64/mm/mmu.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index b6f5aa52ac67..2dbd72319152 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -372,13 +372,22 @@ static void __create_pgd_mapping(pgd_t *pgdir, phys_addr_t phys,
>  
>  static phys_addr_t pgd_pgtable_alloc(void)
>  {
> -	void *ptr = (void *)__get_free_page(PGALLOC_GFP);
> -	if (!ptr || !pgtable_page_ctor(virt_to_page(ptr)))
> -		BUG();
> +
> +	pgtable_t ptr;
> +
> +	/*
> +	 * pgd_pgtable_alloc() is called while creating kernel mappings
> +	 * through __create_pgd_mapping() might not install it through
> +	 * swapper_pg_dir (&init_mm). Even then init_mm is passed here
> +	 * just to indicate that the allocation is kernel specific not
> +	 * for the user space page tables.
> +	 */

I'm not sure what this is trying to convey. I guess we also use this for
creating the EFI mappings?

No other architecture seems to use pte_alloc_one() for kernel tables,
and it's not clear to me how the mm argument is intended to be used by
the implementation.

Thanks,
Mark.

> +	ptr = pte_alloc_one(&init_mm);
> +	BUG_ON(!ptr);
>  
>  	/* Ensure the zeroed page is visible to the page table walker */
>  	dsb(ishst);
> -	return __pa(ptr);
> +	return page_to_phys(ptr);
>  }
>  
>  /*
> -- 
> 2.20.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 5/6] arm64/mm: Enable page table page accounting for user space

[Mark Rutland]

On Mon, Feb 25, 2019 at 10:33:58AM +0530, Anshuman Khandual wrote:
> Page table pages created for user space processes must be accounted against
> their memory control groups if initialized. This just introduces a helper
> function pgtable_gfp_flags() which returns PGALLOC_GFP for all kernel page
> table page allocations but adds __GFP_ACCOUNT for user page table pages.

Can't we have pte_alloc_one_kernel() and pte_alloc_one() explicitly pass
the GFP flags down to a __pte_alloc_one() helper, and consistently use
pte_alloc_one_kernel() for kernel mappings?

That would seem less surprising than hiding that detail in another
function.

Thanks,
Mark.

> 
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
>  arch/arm64/include/asm/pgalloc.h | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/pgalloc.h b/arch/arm64/include/asm/pgalloc.h
> index 076764f66fb0..a02a4d1d967d 100644
> --- a/arch/arm64/include/asm/pgalloc.h
> +++ b/arch/arm64/include/asm/pgalloc.h
> @@ -100,12 +100,24 @@ pte_alloc_one_kernel(struct mm_struct *mm)
>  	return (pte_t *)pte_alloc_one_virt(mm);
>  }
>  
> +extern struct mm_struct efi_mm;
> +
> +static inline gfp_t pgtable_gfp_flags(struct mm_struct *mm)
> +{
> +	if (unlikely((mm == &init_mm) || (mm == &efi_mm)))
> +		return PGALLOC_GFP;
> +	else
> +		return PGALLOC_GFP | __GFP_ACCOUNT;
> +}
> +
>  static inline pgtable_t
>  pte_alloc_one(struct mm_struct *mm)
>  {
>  	struct page *pte;
> +	gfp_t gfp;
>  
> -	pte = alloc_pages(PGALLOC_GFP, 0);
> +	gfp = pgtable_gfp_flags(mm);
> +	pte = alloc_pages(gfp, 0);
>  	if (!pte)
>  		return NULL;
>  	if (!pgtable_page_ctor(pte)) {
> -- 
> 2.20.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 1/6] KVM: ARM: Remove pgtable standard functions from stage-2 page tables

[Anshuman Khandual]

On 02/25/2019 04:30 PM, Mark Rutland wrote:
> Hi Anshuman,
> 
> On Mon, Feb 25, 2019 at 10:33:54AM +0530, Anshuman Khandual wrote:
>> ARM64 standard pgtable functions are going to use pgtable_page_[ctor|dtor]
>> constructs. Certain stage-2 page table allocations are multi order which
>> cannot be allocated through a generic pgtable function as it does not exist
>> right now. This prevents all pgtable allocations including multi order ones
>> in stage-2 from moving into new ARM64 (pgtable_page_[ctor|dtor]) pgtable
>> functions. Hence remove all generic pgtable allocation function dependency
>> from stage-2 page tables till there is one multi-order allocation function
>> available.
> 
> I'm a bit confused by this. Which allocations are multi-order?
> 

Stage-2 PGD.

kvm_alloc_stage2_pgd -> alloc_pages_exact
kvm_free_stage2_pgd -> free_pages_exact
 
> Why does that prevent other allcoations from using the regular routines?

At present both stage-2 PGD (kvm_alloc_stage2_pgd -> alloc_pages_exact), PUD|PMD
(mmu_memory_cache_alloc) allocates directly from buddy allocator but then while
freeing back stage-2 PGD directly calls buddy allocator via free_pages_exact but
PUD|PMD get freed with stage2_[pud|pmd]_free which calls pud|pmd_free instead
of calling free_pages() directly.

All of these worked fine because pud|pmd_free called free_pages() directly with
out going through pgtable_page_dtor(). But now we are changing pud|pmd_free to
use pgtable_page_dtor() both for user and host kernel page tables. This will
break stage2 page table (bad page state errors) because the new free path which
would call pgtable_page_dtor() where as alloc patch never called pgtable_page_ctor().

To fix this situation either we move all stage-2 page table to use pte_alloc_one()
and pte_free() which goes through pgtable_page_ctor/dtor cycle or just keep the
virtualization page tables out of it (stage2/hyp) and remove the only dependency
which breaks because of these changes. This series went with the second option. 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 2/6] arm64/mm: Make pgd_pgtable_alloc() call pte_alloc_one() always

[Anshuman Khandual]

pgtable_page_ctor/pgtable

On 02/25/2019 04:38 PM, Mark Rutland wrote:
> On Mon, Feb 25, 2019 at 10:33:55AM +0530, Anshuman Khandual wrote:
>> pgd_pgtable_alloc() provides page allocation function while creating all
>> levels of page table (PGD, PUD, CONT PMD etc) for various kernel mappings.
>> It calls __get_free_page() and initializes page with pagetable_page_ctor().
>> pte_alloc_one() already provides a standard interface for allocating a page
>> table page and initializes it with pagetable_page_ctor(). This removes the
>> redundancy and instead make it call pte_alloc_one() directly.
> 
> I see we also have pte_alloc_one_kernel(), which does not call
> pgtable_page_ctor().

We are changing it.

> 
> Was it deliberate that we don't call the ctor on kernel mappings?

Right now parts of the kernel mapping call pgtable_page_ctor() like everything
with __create_pgd_mapping(pgd_pgtabble_alloc,....) but pgtable_page_ctor()
never gets called with pte_alloc_one_kernel() based allocations. This series
is trying to move all possible kernel mappings to use pgtable_page_ctor/pgtable
_page_dtor() constructs.

It makes sense to call pgtable_page_ctor/pgtable_page_dtor for kernel mappings
as it helps put the page in right state PageTable and also account towards zone
statistics (NR_PAGETABLE).

>
>>
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>>  arch/arm64/mm/mmu.c | 17 +++++++++++++----
>>  1 file changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index b6f5aa52ac67..2dbd72319152 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -372,13 +372,22 @@ static void __create_pgd_mapping(pgd_t *pgdir, phys_addr_t phys,
>>  
>>  static phys_addr_t pgd_pgtable_alloc(void)
>>  {
>> -	void *ptr = (void *)__get_free_page(PGALLOC_GFP);
>> -	if (!ptr || !pgtable_page_ctor(virt_to_page(ptr)))
>> -		BUG();
>> +
>> +	pgtable_t ptr;
>> +
>> +	/*
>> +	 * pgd_pgtable_alloc() is called while creating kernel mappings
>> +	 * through __create_pgd_mapping() might not install it through
>> +	 * swapper_pg_dir (&init_mm). Even then init_mm is passed here
>> +	 * just to indicate that the allocation is kernel specific not
>> +	 * for the user space page tables.
>> +	 */
> 
> I'm not sure what this is trying to convey. I guess we also use this for
> creating the EFI mappings?

The argument here will be used to select right GFP flags. With this init_mm
would pick the right one whether its used for init_mm or efi_mm mappings.
Though we still check for both the options while picking up the GFP flags
in one of the later patches.

> 
> No other architecture seems to use pte_alloc_one() for kernel tables,
> and it's not clear to me how the mm argument is intended to be used by
> the implementation.

It selects appropriate GFP flags (with or without __GFP_ACCOUNT). To make
things better will probably pass mm_struct in pgtable_alloc() which is used
with __create_pgd_mappings. Parts of arm64 kernel mappings already use it.
This is just making it uniform across the kernel.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 5/6] arm64/mm: Enable page table page accounting for user space

[Anshuman Khandual]

On 02/25/2019 04:41 PM, Mark Rutland wrote:
> On Mon, Feb 25, 2019 at 10:33:58AM +0530, Anshuman Khandual wrote:
>> Page table pages created for user space processes must be accounted against
>> their memory control groups if initialized. This just introduces a helper
>> function pgtable_gfp_flags() which returns PGALLOC_GFP for all kernel page
>> table page allocations but adds __GFP_ACCOUNT for user page table pages.
> 
> Can't we have pte_alloc_one_kernel() and pte_alloc_one() explicitly pass
> the GFP flags down to a __pte_alloc_one() helper, and consistently use
> pte_alloc_one_kernel() for kernel mappings?
> 
> That would seem less surprising than hiding that detail in another
> function.

Yes that will work as well. But its better to have this mm_struct based GFP switch
which is used in other archs as well to create generic MM helper functions going
forward.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 5/6] arm64/mm: Enable page table page accounting for user space

[Mark Rutland]

On Mon, Feb 25, 2019 at 08:19:53PM +0530, Anshuman Khandual wrote:
> 
> 
> On 02/25/2019 04:41 PM, Mark Rutland wrote:
> > On Mon, Feb 25, 2019 at 10:33:58AM +0530, Anshuman Khandual wrote:
> >> Page table pages created for user space processes must be accounted against
> >> their memory control groups if initialized. This just introduces a helper
> >> function pgtable_gfp_flags() which returns PGALLOC_GFP for all kernel page
> >> table page allocations but adds __GFP_ACCOUNT for user page table pages.
> > 
> > Can't we have pte_alloc_one_kernel() and pte_alloc_one() explicitly pass
> > the GFP flags down to a __pte_alloc_one() helper, and consistently use
> > pte_alloc_one_kernel() for kernel mappings?
> > 
> > That would seem less surprising than hiding that detail in another
> > function.
> 
> Yes that will work as well. But its better to have this mm_struct based GFP switch
> which is used in other archs as well to create generic MM helper functions going
> forward.

I don't follow -- if we can make pte_alloc_one() generic, then we can
*also* make pte_alloc_one_kernel() generic.

Looking at x86, I see:

pte_t *pte_alloc_one_kernel(struct mm_struct *mm)
{
        return (pte_t *)__get_free_page(PGALLOC_GFP & ~__GFP_ACCOUNT);
}

... which arhitectures do you see which look at the mm struct to decide
the flags?

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 1/6] KVM: ARM: Remove pgtable standard functions from stage-2 page tables

[Mark Rutland]

On Mon, Feb 25, 2019 at 07:50:27PM +0530, Anshuman Khandual wrote:
> 
> 
> On 02/25/2019 04:30 PM, Mark Rutland wrote:
> > Hi Anshuman,
> > 
> > On Mon, Feb 25, 2019 at 10:33:54AM +0530, Anshuman Khandual wrote:
> >> ARM64 standard pgtable functions are going to use pgtable_page_[ctor|dtor]
> >> constructs. Certain stage-2 page table allocations are multi order which
> >> cannot be allocated through a generic pgtable function as it does not exist
> >> right now. This prevents all pgtable allocations including multi order ones
> >> in stage-2 from moving into new ARM64 (pgtable_page_[ctor|dtor]) pgtable
> >> functions. Hence remove all generic pgtable allocation function dependency
> >> from stage-2 page tables till there is one multi-order allocation function
> >> available.
> > 
> > I'm a bit confused by this. Which allocations are multi-order?
> > 
> 
> Stage-2 PGD.
> 
> kvm_alloc_stage2_pgd -> alloc_pages_exact
> kvm_free_stage2_pgd -> free_pages_exact
>  
> > Why does that prevent other allcoations from using the regular routines?
> 
> At present both stage-2 PGD (kvm_alloc_stage2_pgd -> alloc_pages_exact), PUD|PMD
> (mmu_memory_cache_alloc) allocates directly from buddy allocator but then while
> freeing back stage-2 PGD directly calls buddy allocator via free_pages_exact but
> PUD|PMD get freed with stage2_[pud|pmd]_free which calls pud|pmd_free instead
> of calling free_pages() directly.

If we allocate/free the stage-2 PGD with {alloc,free}_pages_exact(),
then the PGD level is balanced today.

I don't see what that has to do with the other levels of table.

> All of these worked fine because pud|pmd_free called free_pages() directly with
> out going through pgtable_page_dtor(). But now we are changing pud|pmd_free to
> use pgtable_page_dtor() both for user and host kernel page tables. This will
> break stage2 page table (bad page state errors) because the new free path which
> would call pgtable_page_dtor() where as alloc patch never called pgtable_page_ctor().

I'm lost as to how that relates to the alloc/free of the PGD. AFAICT,
that's unrelated to the problem at hand.

What subtlety am I missing?

> To fix this situation either we move all stage-2 page table to use pte_alloc_one()
> and pte_free() which goes through pgtable_page_ctor/dtor cycle or just keep the
> virtualization page tables out of it (stage2/hyp) and remove the only dependency
> which breaks because of these changes. This series went with the second option. 

It sounds to me like this is just a mismatch between the alloc/free
paths for the PUD and PMD levels of table.

IIUC, we allocate those with {pmd,pud}_alloc_one(), and free them with
stage2_{pud,pmd}_free(), which call {pud,pmd}_free().

I would naively expect p?d_alloc_one() to pair with p?d_free(), so that
being a problem is surprising to me.

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 1/6] KVM: ARM: Remove pgtable standard functions from stage-2 page tables

[Anshuman Khandual]

On 02/25/2019 09:19 PM, Mark Rutland wrote:
> On Mon, Feb 25, 2019 at 07:50:27PM +0530, Anshuman Khandual wrote:
>>
>>
>> On 02/25/2019 04:30 PM, Mark Rutland wrote:
>>> Hi Anshuman,
>>>
>>> On Mon, Feb 25, 2019 at 10:33:54AM +0530, Anshuman Khandual wrote:
>>>> ARM64 standard pgtable functions are going to use pgtable_page_[ctor|dtor]
>>>> constructs. Certain stage-2 page table allocations are multi order which
>>>> cannot be allocated through a generic pgtable function as it does not exist
>>>> right now. This prevents all pgtable allocations including multi order ones
>>>> in stage-2 from moving into new ARM64 (pgtable_page_[ctor|dtor]) pgtable
>>>> functions. Hence remove all generic pgtable allocation function dependency
>>>> from stage-2 page tables till there is one multi-order allocation function
>>>> available.
>>>
>>> I'm a bit confused by this. Which allocations are multi-order?
>>>
>>
>> Stage-2 PGD.
>>
>> kvm_alloc_stage2_pgd -> alloc_pages_exact
>> kvm_free_stage2_pgd -> free_pages_exact
>>  
>>> Why does that prevent other allcoations from using the regular routines?
>>
>> At present both stage-2 PGD (kvm_alloc_stage2_pgd -> alloc_pages_exact), PUD|PMD
>> (mmu_memory_cache_alloc) allocates directly from buddy allocator but then while
>> freeing back stage-2 PGD directly calls buddy allocator via free_pages_exact but
>> PUD|PMD get freed with stage2_[pud|pmd]_free which calls pud|pmd_free instead
>> of calling free_pages() directly.
> 
> If we allocate/free the stage-2 PGD with {alloc,free}_pages_exact(),
> then the PGD level is balanced today.
> 
> I don't see what that has to do with the other levels of table.

The idea is either all page table levels pages go through ctor/dtor constructs
or none of them do. Not only this makes sense logically but also prevents getting
into bad page state errors when freeing without calling dtor. For other non-KVM
use cases like kernel linear or vmmap it helps while tearing down the page table
for memory hot-remove cases.

> 
>> All of these worked fine because pud|pmd_free called free_pages() directly with
>> out going through pgtable_page_dtor(). But now we are changing pud|pmd_free to
>> use pgtable_page_dtor() both for user and host kernel page tables. This will
>> break stage2 page table (bad page state errors) because the new free path which
>> would call pgtable_page_dtor() where as alloc patch never called pgtable_page_ctor().
> 
> I'm lost as to how that relates to the alloc/free of the PGD. AFAICT,
> that's unrelated to the problem at hand.

As mentioned above either all page table use ctor/dtor based allocation/free or
none of them do.

> 
> What subtlety am I missing?

pmd|pud_free() will call dtor() after the series hence they cannot be used by
stage2_pmd|pud_free because stage2_pud|pmd get allocated from pre-allocated
mmu_memory_cache_alloc which builds with __get_free_page(PGALLOC_GFP). Is not
that right ? 

> 
>> To fix this situation either we move all stage-2 page table to use pte_alloc_one()
>> and pte_free() which goes through pgtable_page_ctor/dtor cycle or just keep the
>> virtualization page tables out of it (stage2/hyp) and remove the only dependency
>> which breaks because of these changes. This series went with the second option. 
> 
> It sounds to me like this is just a mismatch between the alloc/free
> paths for the PUD and PMD levels of table.
> 
> IIUC, we allocate those with {pmd,pud}_alloc_one(), and free them with
> stage2_{pud,pmd}_free(), which call {pud,pmd}_free().

AFAICS they get allocated from mmu_memory_cache_alloc instead. pud|pmd_alloc_one()
get used only for the _hyp_ mappings not for the stage2 table.

kvm_handle_guest_abort
	user_mem_abort
		stage2_set_pud_huge -> stage2_get_pud -> mmu_memory_cache_alloc
		stage2_set_pmd_huge -> stage2_get_pmd -> mmu_memory_cache_alloc
		stage2_set_pte -> mmu_memory_cache_alloc

Am I missing something here.

> 
> I would naively expect p?d_alloc_one() to pair with p?d_free(), so that
> being a problem is surprising to me.

They work absolutely fine. But thats not the case here. stage2_pud|pmd_free()
calls pud|pmd_free() for pages not allocated with pud|pmd_alloc_one().

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 5/6] arm64/mm: Enable page table page accounting for user space

[Anshuman Khandual]

On 02/25/2019 09:05 PM, Mark Rutland wrote:
> On Mon, Feb 25, 2019 at 08:19:53PM +0530, Anshuman Khandual wrote:
>>
>>
>> On 02/25/2019 04:41 PM, Mark Rutland wrote:
>>> On Mon, Feb 25, 2019 at 10:33:58AM +0530, Anshuman Khandual wrote:
>>>> Page table pages created for user space processes must be accounted against
>>>> their memory control groups if initialized. This just introduces a helper
>>>> function pgtable_gfp_flags() which returns PGALLOC_GFP for all kernel page
>>>> table page allocations but adds __GFP_ACCOUNT for user page table pages.
>>>
>>> Can't we have pte_alloc_one_kernel() and pte_alloc_one() explicitly pass
>>> the GFP flags down to a __pte_alloc_one() helper, and consistently use
>>> pte_alloc_one_kernel() for kernel mappings?
>>>
>>> That would seem less surprising than hiding that detail in another
>>> function.
>>
>> Yes that will work as well. But its better to have this mm_struct based GFP switch
>> which is used in other archs as well to create generic MM helper functions going
>> forward.
> 
> I don't follow -- if we can make pte_alloc_one() generic, then we can
> *also* make pte_alloc_one_kernel() generic.

Right but when we do mm_struct can be used to decide applicable GFP for
the allocation. The way your proposal defers is that it wants to have
the GFP flag unique for both pte_alloc_one and pte_alloc_one_kernel
without looking into mm_struct which will work as well.

> 
> Looking at x86, I see:
> 
> pte_t *pte_alloc_one_kernel(struct mm_struct *mm)
> {
>         return (pte_t *)__get_free_page(PGALLOC_GFP & ~__GFP_ACCOUNT);
> }
> 
> ... which arhitectures do you see which look at the mm struct to decide
> the flags?
> 

On powerpc its an explicit helper function

arch/powerpc/include/asm/pgalloc.h (pgtable_gfp_flags)

On X86 its not an explicit helper function but we can see

arch/x86/include/asm/pgalloc.h

static inline pmd_t *pmd_alloc_one(struct mm_struct *mm, unsigned long addr)
{
        struct page *page;
        gfp_t gfp = GFP_KERNEL_ACCOUNT | __GFP_ZERO;

        if (mm == &init_mm)
                gfp &= ~__GFP_ACCOUNT;
        page = alloc_pages(gfp, 0);
        if (!page)
                return NULL;
        if (!pgtable_pmd_page_ctor(page)) {
                __free_pages(page, 0);
                return NULL;
        }
        return (pmd_t *)page_address(page);
}

static inline pud_t *pud_alloc_one(struct mm_struct *mm, unsigned long addr)
{
        gfp_t gfp = GFP_KERNEL_ACCOUNT;

        if (mm == &init_mm)
                gfp &= ~__GFP_ACCOUNT;
        return (pud_t *)get_zeroed_page(gfp);
}

static inline p4d_t *p4d_alloc_one(struct mm_struct *mm, unsigned long addr)
{
        gfp_t gfp = GFP_KERNEL_ACCOUNT;

        if (mm == &init_mm)
                gfp &= ~__GFP_ACCOUNT;
        return (p4d_t *)get_zeroed_page(gfp);
}

There are some more places on powerpc and x86 where it does the switch. But you
are right, pte_alloc_one_kernel() does not go through ctor functions on other
archs. But it should and this series changes it. Do you see any concerns in
doing so ?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 5/6] arm64/mm: Enable page table page accounting for user space

[Anshuman Khandual]

On 02/25/2019 10:33 AM, Anshuman Khandual wrote:
> Page table pages created for user space processes must be accounted against
> their memory control groups if initialized. This just introduces a helper
> function pgtable_gfp_flags() which returns PGALLOC_GFP for all kernel page
> table page allocations but adds __GFP_ACCOUNT for user page table pages.
> 
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
>  arch/arm64/include/asm/pgalloc.h | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/pgalloc.h b/arch/arm64/include/asm/pgalloc.h
> index 076764f66fb0..a02a4d1d967d 100644
> --- a/arch/arm64/include/asm/pgalloc.h
> +++ b/arch/arm64/include/asm/pgalloc.h
> @@ -100,12 +100,24 @@ pte_alloc_one_kernel(struct mm_struct *mm)
>  	return (pte_t *)pte_alloc_one_virt(mm);
>  }
>  
> +extern struct mm_struct efi_mm;
> +
> +static inline gfp_t pgtable_gfp_flags(struct mm_struct *mm)
> +{
> +	if (unlikely((mm == &init_mm) || (mm == &efi_mm)))
> +		return PGALLOC_GFP;

Hello Will,

Checking for efi_mm is not necessary here as we just pass init_mm into
pte_alloc_one() from pgd_pgtable_alloc() which is used for all runtime
allocations for kernel mappings including init_mm and efi_mm. If we
really want to differentiate them, the helper pgtable_alloc() has to be
changed to include mm_struct. That might be some amount of code churn.
Wondering what will be preferred here.

- Anshuman  

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 1/6] KVM: ARM: Remove pgtable standard functions from stage-2 page tables

[Anshuman Khandual]

On 02/26/2019 09:45 AM, Anshuman Khandual wrote:
> 
> 
> On 02/25/2019 09:19 PM, Mark Rutland wrote:
>> On Mon, Feb 25, 2019 at 07:50:27PM +0530, Anshuman Khandual wrote:
>>>
>>>
>>> On 02/25/2019 04:30 PM, Mark Rutland wrote:
>>>> Hi Anshuman,
>>>>
>>>> On Mon, Feb 25, 2019 at 10:33:54AM +0530, Anshuman Khandual wrote:
>>>>> ARM64 standard pgtable functions are going to use pgtable_page_[ctor|dtor]
>>>>> constructs. Certain stage-2 page table allocations are multi order which
>>>>> cannot be allocated through a generic pgtable function as it does not exist
>>>>> right now. This prevents all pgtable allocations including multi order ones
>>>>> in stage-2 from moving into new ARM64 (pgtable_page_[ctor|dtor]) pgtable
>>>>> functions. Hence remove all generic pgtable allocation function dependency
>>>>> from stage-2 page tables till there is one multi-order allocation function
>>>>> available.
>>>>
>>>> I'm a bit confused by this. Which allocations are multi-order?
>>>>
>>>
>>> Stage-2 PGD.
>>>
>>> kvm_alloc_stage2_pgd -> alloc_pages_exact
>>> kvm_free_stage2_pgd -> free_pages_exact
>>>  
>>>> Why does that prevent other allcoations from using the regular routines?
>>>
>>> At present both stage-2 PGD (kvm_alloc_stage2_pgd -> alloc_pages_exact), PUD|PMD
>>> (mmu_memory_cache_alloc) allocates directly from buddy allocator but then while
>>> freeing back stage-2 PGD directly calls buddy allocator via free_pages_exact but
>>> PUD|PMD get freed with stage2_[pud|pmd]_free which calls pud|pmd_free instead
>>> of calling free_pages() directly.
>>
>> If we allocate/free the stage-2 PGD with {alloc,free}_pages_exact(),
>> then the PGD level is balanced today.
>>
>> I don't see what that has to do with the other levels of table.
> 
> The idea is either all page table levels pages go through ctor/dtor constructs
> or none of them do. Not only this makes sense logically but also prevents getting
> into bad page state errors when freeing without calling dtor. For other non-KVM
> use cases like kernel linear or vmmap it helps while tearing down the page table
> for memory hot-remove cases.
> 
>>
>>> All of these worked fine because pud|pmd_free called free_pages() directly with
>>> out going through pgtable_page_dtor(). But now we are changing pud|pmd_free to
>>> use pgtable_page_dtor() both for user and host kernel page tables. This will
>>> break stage2 page table (bad page state errors) because the new free path which
>>> would call pgtable_page_dtor() where as alloc patch never called pgtable_page_ctor().
>>
>> I'm lost as to how that relates to the alloc/free of the PGD. AFAICT,
>> that's unrelated to the problem at hand.
> 
> As mentioned above either all page table use ctor/dtor based allocation/free or
> none of them do.
> 
>>
>> What subtlety am I missing?
> 
> pmd|pud_free() will call dtor() after the series hence they cannot be used by
> stage2_pmd|pud_free because stage2_pud|pmd get allocated from pre-allocated
> mmu_memory_cache_alloc which builds with __get_free_page(PGALLOC_GFP). Is not
> that right ? 
> 
>>
>>> To fix this situation either we move all stage-2 page table to use pte_alloc_one()
>>> and pte_free() which goes through pgtable_page_ctor/dtor cycle or just keep the
>>> virtualization page tables out of it (stage2/hyp) and remove the only dependency
>>> which breaks because of these changes. This series went with the second option. 
>>
>> It sounds to me like this is just a mismatch between the alloc/free
>> paths for the PUD and PMD levels of table.
>>
>> IIUC, we allocate those with {pmd,pud}_alloc_one(), and free them with
>> stage2_{pud,pmd}_free(), which call {pud,pmd}_free().
> 
> AFAICS they get allocated from mmu_memory_cache_alloc instead. pud|pmd_alloc_one()
> get used only for the _hyp_ mappings not for the stage2 table.
> 
> kvm_handle_guest_abort
> 	user_mem_abort
> 		stage2_set_pud_huge -> stage2_get_pud -> mmu_memory_cache_alloc
> 		stage2_set_pmd_huge -> stage2_get_pmd -> mmu_memory_cache_alloc
> 		stage2_set_pte -> mmu_memory_cache_alloc
> 
> Am I missing something here.

This needs to be added into this patch here. Got skipped because my guest VM test setup
always created PMD level mappings. No impact on arm platform as it's pte_free_kernel()
always called free_page which remains unchanged.

diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
index 30251e288629..61ea76d786b9 100644
--- a/virt/kvm/arm/mmu.c
+++ b/virt/kvm/arm/mmu.c
@@ -191,7 +191,7 @@ static void clear_stage2_pmd_entry(struct kvm *kvm, pmd_t *pmd, phys_addr_t addr
        VM_BUG_ON(pmd_thp_or_huge(*pmd));
        pmd_clear(pmd);
        kvm_tlb_flush_vmid_ipa(kvm, addr);
-       pte_free_kernel(NULL, pte_table);
+       __free_page(virt_to_page(pte_table));
        put_page(virt_to_page(pmd));
 }


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel