Re: [PATCH] parisc: Optimize per-pagetable spinlocks (v11)

[Rolf Eike Beer <eike-kernel@sf-tec.de>]

[-- Attachment #1: Type: text/plain, Size: 4603 bytes --]

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@infradead.org>
> Fixes: b37d1c1898b2 ("parisc: Use per-pagetable spinlock")
> Signed-off-by: Helge Deller <deller@gmx.de>
> Signed-off-by: John David Anglin <dave.anglin@bell.net>
> 
> 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?

Eike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 195 bytes --]