On 08/03/2015 20:48, David Daney wrote: > From: David Daney <david.daney@xxxxxxxxxx> > > 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@xxxxxxxxxx> > Cc: <stable@xxxxxxxxxxxxxxx> > --- > 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
