Messages in this thread | ![/](/images/icornerl.gif) | | Date | Thu, 11 Apr 2024 14:24:25 +0100 | From | Mark Rutland <> | Subject | Re: [PATCH v2 4/4] arm64: mm: Lazily clear pte table mappings from fixmap |
| |
On Thu, Apr 04, 2024 at 03:33:08PM +0100, Ryan Roberts wrote: > With the pgtable operations abstracted into `struct pgtable_ops`, the > early pgtable alloc, map and unmap operations are nicely centralized. So > let's enhance the implementation to speed up the clearing of pte table > mappings in the fixmap. > > Extend FIX_MAP so that we now have 16 slots in the fixmap dedicated for > pte tables. At alloc/map time, we select the next slot in the series and > map it. Or if we are at the end and no more slots are available, clear > down all of the slots and start at the beginning again. Batching the > clear like this means we can issue tlbis more efficiently. > > Due to the batching, there may still be some slots mapped at the end, so > address this by adding an optional cleanup() function to `struct > pgtable_ops` to handle this for us. > > Execution time of map_mem(), which creates the kernel linear map page > tables, was measured on different machines with different RAM configs: > > | Apple M2 VM | Ampere Altra| Ampere Altra| Ampere Altra > | VM, 16G | VM, 64G | VM, 256G | Metal, 512G > ---------------|-------------|-------------|-------------|------------- > | ms (%) | ms (%) | ms (%) | ms (%) > ---------------|-------------|-------------|-------------|------------- > before | 11 (0%) | 109 (0%) | 449 (0%) | 1257 (0%) > after | 6 (-46%) | 61 (-44%) | 257 (-43%) | 838 (-33%)
I'd prefer to leave this as-is for now, as compared to the baseline this is the last 2-3%, and (assuming my comments on patch 3 hold) that way we don't need the pgtable_ops indirection, which'll keep the code fairly simple to understand.
So unless Catalin or Will feel otherwise, I'd suggest that we take patches 1 and 2, drop 3 and 4 for now, and maybe try the alternative approach I've commented on patch 3.
Does that sound ok to you?
Mark.
> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com> > Tested-by: Itaru Kitayama <itaru.kitayama@fujitsu.com> > Tested-by: Eric Chanudet <echanude@redhat.com> > --- > arch/arm64/include/asm/fixmap.h | 5 +++- > arch/arm64/include/asm/pgtable.h | 4 --- > arch/arm64/mm/fixmap.c | 11 ++++++++ > arch/arm64/mm/mmu.c | 44 +++++++++++++++++++++++++++++--- > 4 files changed, 56 insertions(+), 8 deletions(-) > > diff --git a/arch/arm64/include/asm/fixmap.h b/arch/arm64/include/asm/fixmap.h > index 87e307804b99..91fcd7c5c513 100644 > --- a/arch/arm64/include/asm/fixmap.h > +++ b/arch/arm64/include/asm/fixmap.h > @@ -84,7 +84,9 @@ enum fixed_addresses { > * Used for kernel page table creation, so unmapped memory may be used > * for tables. > */ > - FIX_PTE, > +#define NR_PTE_SLOTS 16 > + FIX_PTE_END, > + FIX_PTE_BEGIN = FIX_PTE_END + NR_PTE_SLOTS - 1, > FIX_PMD, > FIX_PUD, > FIX_P4D, > @@ -108,6 +110,7 @@ void __init early_fixmap_init(void); > #define __late_clear_fixmap(idx) __set_fixmap((idx), 0, FIXMAP_PAGE_CLEAR) > > extern void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot); > +void __init clear_fixmap_nosync(enum fixed_addresses idx); > > #include <asm-generic/fixmap.h> > > diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h > index 92c9aed5e7af..4c7114d49697 100644 > --- a/arch/arm64/include/asm/pgtable.h > +++ b/arch/arm64/include/asm/pgtable.h > @@ -691,10 +691,6 @@ static inline unsigned long pmd_page_vaddr(pmd_t pmd) > /* Find an entry in the third-level page table. */ > #define pte_offset_phys(dir,addr) (pmd_page_paddr(READ_ONCE(*(dir))) + pte_index(addr) * sizeof(pte_t)) > > -#define pte_set_fixmap(addr) ((pte_t *)set_fixmap_offset(FIX_PTE, addr)) > -#define pte_set_fixmap_offset(pmd, addr) pte_set_fixmap(pte_offset_phys(pmd, addr)) > -#define pte_clear_fixmap() clear_fixmap(FIX_PTE) > - > #define pmd_page(pmd) phys_to_page(__pmd_to_phys(pmd)) > > /* use ONLY for statically allocated translation tables */ > diff --git a/arch/arm64/mm/fixmap.c b/arch/arm64/mm/fixmap.c > index de1e09d986ad..0cb09bedeeec 100644 > --- a/arch/arm64/mm/fixmap.c > +++ b/arch/arm64/mm/fixmap.c > @@ -131,6 +131,17 @@ void __set_fixmap(enum fixed_addresses idx, > } > } > > +void __init clear_fixmap_nosync(enum fixed_addresses idx) > +{ > + unsigned long addr = __fix_to_virt(idx); > + pte_t *ptep; > + > + BUG_ON(idx <= FIX_HOLE || idx >= __end_of_fixed_addresses); > + > + ptep = fixmap_pte(addr); > + __pte_clear(&init_mm, addr, ptep); > +} > + > void *__init fixmap_remap_fdt(phys_addr_t dt_phys, int *size, pgprot_t prot) > { > const u64 dt_virt_base = __fix_to_virt(FIX_FDT); > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c > index 90bf822859b8..2e3b594aa23c 100644 > --- a/arch/arm64/mm/mmu.c > +++ b/arch/arm64/mm/mmu.c > @@ -66,11 +66,14 @@ enum pgtable_type { > * mapped either as a result of a previous call to alloc() or > * map(). The page's virtual address must be considered invalid > * after this call returns. > + * @cleanup: (Optional) Called at the end of a set of operations to cleanup > + * any lazy state. > */ > struct pgtable_ops { > void *(*alloc)(int type, phys_addr_t *pa); > void *(*map)(int type, void *parent, unsigned long addr); > void (*unmap)(int type); > + void (*cleanup)(void); > }; > > #define NO_BLOCK_MAPPINGS BIT(0) > @@ -139,9 +142,33 @@ pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn, > } > EXPORT_SYMBOL(phys_mem_access_prot); > > +static int pte_slot_next __initdata = FIX_PTE_BEGIN; > + > +static void __init clear_pte_fixmap_slots(void) > +{ > + unsigned long start = __fix_to_virt(FIX_PTE_BEGIN); > + unsigned long end = __fix_to_virt(pte_slot_next); > + int i; > + > + for (i = FIX_PTE_BEGIN; i > pte_slot_next; i--) > + clear_fixmap_nosync(i); > + > + flush_tlb_kernel_range(start, end); > + pte_slot_next = FIX_PTE_BEGIN; > +} > + > +static int __init pte_fixmap_slot(void) > +{ > + if (pte_slot_next < FIX_PTE_END) > + clear_pte_fixmap_slots(); > + > + return pte_slot_next--; > +} > + > static void *__init early_pgtable_alloc(int type, phys_addr_t *pa) > { > void *va; > + int slot; > > *pa = memblock_phys_alloc_range(PAGE_SIZE, PAGE_SIZE, 0, > MEMBLOCK_ALLOC_NOLEAKTRACE); > @@ -159,7 +186,9 @@ static void *__init early_pgtable_alloc(int type, phys_addr_t *pa) > va = pmd_set_fixmap(*pa); > break; > case TYPE_PTE: > - va = pte_set_fixmap(*pa); > + slot = pte_fixmap_slot(); > + set_fixmap(slot, *pa); > + va = (pte_t *)__fix_to_virt(slot); > break; > default: > BUG(); > @@ -174,7 +203,9 @@ static void *__init early_pgtable_alloc(int type, phys_addr_t *pa) > > static void *__init early_pgtable_map(int type, void *parent, unsigned long addr) > { > + phys_addr_t pa; > void *entry; > + int slot; > > switch (type) { > case TYPE_P4D: > @@ -187,7 +218,10 @@ static void *__init early_pgtable_map(int type, void *parent, unsigned long addr > entry = pmd_set_fixmap_offset((pud_t *)parent, addr); > break; > case TYPE_PTE: > - entry = pte_set_fixmap_offset((pmd_t *)parent, addr); > + slot = pte_fixmap_slot(); > + pa = pte_offset_phys((pmd_t *)parent, addr); > + set_fixmap(slot, pa); > + entry = (pte_t *)(__fix_to_virt(slot) + (pa & (PAGE_SIZE - 1))); > break; > default: > BUG(); > @@ -209,7 +243,7 @@ static void __init early_pgtable_unmap(int type) > pmd_clear_fixmap(); > break; > case TYPE_PTE: > - pte_clear_fixmap(); > + // Unmap lazily: see clear_pte_fixmap_slots(). > break; > default: > BUG(); > @@ -220,6 +254,7 @@ static struct pgtable_ops early_pgtable_ops __initdata = { > .alloc = early_pgtable_alloc, > .map = early_pgtable_map, > .unmap = early_pgtable_unmap, > + .cleanup = clear_pte_fixmap_slots, > }; > > bool pgattr_change_is_safe(u64 old, u64 new) > @@ -538,6 +573,9 @@ static void __create_pgd_mapping_locked(pgd_t *pgdir, phys_addr_t phys, > alloc_init_p4d(pgdp, addr, next, phys, prot, ops, flags); > phys += next - addr; > } while (pgdp++, addr = next, addr != end); > + > + if (ops->cleanup) > + ops->cleanup(); > } > > static void __create_pgd_mapping(pgd_t *pgdir, phys_addr_t phys, > -- > 2.25.1 >
| ![\](/images/icornerr.gif) |