Am Mittwoch, 27. Januar 2021, 22:18:51 CET schrieb Helge Deller:
> On parisc a spinlock is stored in the next page behind the pgd which
> protects against parallel accesses to the pgd. That's why one additional
> page (PGD_ALLOC_ORDER) is allocated for the pgd.
>
> Matthew Wilcox suggested that we instead should use a pointer in the
> struct page table for this spinlock and noted, that the comments for the
> PGD_ORDER and PMD_ORDER defines were wrong.
>
> Both suggestions are addressed in this patch. The pgd spinlock
> (parisc_pgd_lock) is stored in the struct page table. In
> switch_mm_irqs_off() the physical address of this lock is loaded into
> cr28 (tr4) and the pgd into cr25, so that the fault handlers can
> directly access the lock.
>
> The currently implemened Hybrid L2/L3 page table scheme (where the pmd
> is adjacent to the pgd) is dropped now too.
>
> Suggested-by: Matthew Wilcox <willy@xxxxxxxxxxxxx>
> Fixes: b37d1c1898b2 ("parisc: Use per-pagetable spinlock")
> Signed-off-by: Helge Deller <deller@xxxxxx>
> Signed-off-by: John David Anglin <dave.anglin@xxxxxxxx>
>
> diff --git a/arch/parisc/include/asm/mmu_context.h
> b/arch/parisc/include/asm/mmu_context.h index 46f8c22c5977..3e02b1f75e54
> 100644
> --- a/arch/parisc/include/asm/mmu_context.h
> +++ b/arch/parisc/include/asm/mmu_context.h
> @@ -5,6 +5,7 @@
> #include <linux/mm.h>
> #include <linux/sched.h>
> #include <linux/atomic.h>
> +#include <linux/spinlock.h>
> #include <asm-generic/mm_hooks.h>
>
> /* on PA-RISC, we actually have enough contexts to justify an allocator
> @@ -50,6 +51,14 @@ static inline void switch_mm_irqs_off(struct mm_struct
> *prev, struct mm_struct *next, struct task_struct *tsk)
> {
> if (prev != next) {
> +#ifdef CONFIG_SMP
> + /* phys address of tlb lock in cr28 (tr4) for TLB faults
*/
> + struct page *page;
> +
> + page = virt_to_page((unsigned long) next->pgd);
This is one of the few cases you have a space after the cast.
Another one is in pgd_alloc():
>+ page = virt_to_page((unsigned long) pgd);
> diff --git a/arch/parisc/include/asm/pgalloc.h
> b/arch/parisc/include/asm/pgalloc.h index a6482b2ce0ea..9c1a54543c87 100644
> --- a/arch/parisc/include/asm/pgalloc.h
> +++ b/arch/parisc/include/asm/pgalloc.h
> @@ -68,43 +66,27 @@ static inline void pud_populate(struct mm_struct *mm,
> pud_t *pud, pmd_t *pmd) (__u32)(__pa((unsigned long)pmd) >>
> PxD_VALUE_SHIFT)));
> }
>
> -static inline pmd_t *pmd_alloc_one(struct mm_struct *mm, unsigned long
> address)
> +static inline pmd_t *pmd_alloc_one(struct mm_struct *mm, unsigned
> long addr)
Does that change add benefit?
> {
> - return (pmd_t *)__get_free_pages(GFP_PGTABLE_KERNEL, PMD_ORDER);
> + pmd_t *pmd;
> +
> + pmd = (pmd_t *)__get_free_pages(GFP_PGTABLE_KERNEL, PMD_ORDER);
> + if (pmd)
Maybe annotate that as likely() as it was in pgd_alloc() before?
> + memset ((void *)pmd, 0, PAGE_SIZE << PMD_ORDER);
> + return pmd;
> }
> diff --git a/arch/parisc/include/asm/pgtable.h
> b/arch/parisc/include/asm/pgtable.h index 75cf84070fc9..c08c7b37e5f4 100644
> --- a/arch/parisc/include/asm/pgtable.h
> +++ b/arch/parisc/include/asm/pgtable.h
> @@ -94,10 +96,12 @@ static inline void purge_tlb_entries(struct mm_struct
> *mm, unsigned long addr) #define set_pte_at(mm, addr, ptep, pteval)
\
> do {
\
> unsigned long flags;
\
> - spin_lock_irqsave(pgd_spinlock((mm)->pgd), flags);\
> + spinlock_t *pgd_lock;
\
> + pgd_lock = pgd_spinlock((mm)->pgd); \
These should just fit into a single line.
> + spin_lock_irqsave(pgd_lock, flags); \
> set_pte(ptep, pteval);
\
> purge_tlb_entries(mm, addr);
\
> - spin_unlock_irqrestore(pgd_spinlock((mm)->pgd), flags);\
> + spin_unlock_irqrestore(pgd_lock, flags); \
> } while (0)
>
> #endif /* !__ASSEMBLY__ */
> diff --git a/arch/parisc/mm/init.c b/arch/parisc/mm/init.c
> index 3ec633b11b54..4f3f180b0b20 100644
> --- a/arch/parisc/mm/init.c
> +++ b/arch/parisc/mm/init.c
> @@ -681,6 +681,24 @@ static void __init parisc_bootmem_free(void)
> free_area_init(max_zone_pfn);
> }
>
> +static void __init parisc_init_pgd_lock(void)
> +{
> + struct page *page;
> +
> + page = virt_to_page((unsigned long) &swapper_pg_dir);
Another space.
> + page->parisc_pgd_lock = &pa_swapper_pg_lock;
> +}
> +
> +#ifdef CONFIG_SMP
> +spinlock_t *pgd_spinlock(pgd_t *pgd)
> +{
> + struct page *page;
> +
> + page = virt_to_page((unsigned long) pgd);
> + return page->parisc_pgd_lock;
> +}
> +#endif
This is very simple, and I suspect it being called rather often. Wouldn't it
make sense to make it inline?
EikeAttachment:
signature.asc
Description: This is a digitally signed message part.
