Re: [PATCH] MIPS: Make set_pte() SMP safe.

[Joshua Kinard <kumba@gentoo.org>]

On 08/03/2015 20:48, David Daney wrote:
> From: David Daney <david.daney@cavium.com>
> 
> On MIPS the GLOBAL bit of the PTE must have the same value in any
> aligned pair of PTEs.  These pairs of PTEs are referred to as
> "buddies".  In a SMP system is is possible for two CPUs to be calling
> set_pte() on adjacent PTEs at the same time.  There is a race between
> setting the PTE and a different CPU setting the GLOBAL bit in its
> buddy PTE.
> 
> This race can be observed when multiple CPUs are executing
> vmap()/vfree() at the same time.

Curious, but what's the observed symptom when this race condition occurs?  I am
wondering if it may (or may not) explain the periodic hard lockups on IP27 SMP
that I see during heavy disk I/O.  On that platform, !CONFIG_SMP works fine.
It's only in SMP mode that I can reproduce the lockups, so I suspect it's a
race condition of some kind.


> Make setting the buddy PTE's GLOBAL bit an atomic operation to close
> the race condition.
> 
> The case of CONFIG_64BIT_PHYS_ADDR && CONFIG_CPU_MIPS32 is *not*
> handled.
> 
> Signed-off-by: David Daney <david.daney@cavium.com>
> Cc: <stable@vger.kernel.org>
> ---
>  arch/mips/include/asm/pgtable.h | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/arch/mips/include/asm/pgtable.h b/arch/mips/include/asm/pgtable.h
> index 9d81067..ae85694 100644
> --- a/arch/mips/include/asm/pgtable.h
> +++ b/arch/mips/include/asm/pgtable.h
> @@ -182,8 +182,39 @@ static inline void set_pte(pte_t *ptep, pte_t pteval)
>  		 * Make sure the buddy is global too (if it's !none,
>  		 * it better already be global)
>  		 */
> +#ifdef CONFIG_SMP
> +		/*
> +		 * For SMP, multiple CPUs can race, so we need to do
> +		 * this atomically.
> +		 */
> +#ifdef CONFIG_64BIT
> +#define LL_INSN "lld"
> +#define SC_INSN "scd"
> +#else /* CONFIG_32BIT */
> +#define LL_INSN "ll"
> +#define SC_INSN "sc"
> +#endif
> +		unsigned long page_global = _PAGE_GLOBAL;
> +		unsigned long tmp;
> +
> +		__asm__ __volatile__ (

If an R10000_LLSC_WAR case is needed here (see below), then shouldn't ".set
arch=r4000" go here, and '.set    "MIPS_ISA_ARCH_LEVEL"' go in the
!R10000_LLSC_WAR case?


> +			"	.set	push\n"
> +			"	.set	noreorder\n"
> +			"1:	" LL_INSN "	%[tmp], %[buddy]\n"
> +			"	bnez	%[tmp], 2f\n"
> +			"	 or	%[tmp], %[tmp], %[global]\n"
> +			"	" SC_INSN "	%[tmp], %[buddy]\n"

A quick look at asm/local.h and asm/bitops.h shows that they use "__LL" and
"__SC" instead of defining local variants.  Perhaps those macros are usable
here as well?


> +			"	beqz	%[tmp], 1b\n"

Does this 'beqz' insn need to have a case for R10000_LLSC_WAR, which uses
'beqzl', like several other areas in the kernel?



> +			"	 nop\n"
> +			"2:\n"
> +			"	.set pop"
> +			: [buddy] "+m" (buddy->pte),
> +			  [tmp] "=&r" (tmp)
> +			: [global] "r" (page_global));
> +#else /* !CONFIG_SMP */
>  		if (pte_none(*buddy))
>  			pte_val(*buddy) = pte_val(*buddy) | _PAGE_GLOBAL;
> +#endif /* CONFIG_SMP */
>  	}
>  #endif
>  }
> 

--J