From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from resqmta-ch2-06v.sys.comcast.net ([69.252.207.38]:55707 "EHLO resqmta-ch2-06v.sys.comcast.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753854AbbHXD3A (ORCPT ); Sun, 23 Aug 2015 23:29:00 -0400 Message-ID: <55DA8F47.10002@gentoo.org> Date: Sun, 23 Aug 2015 23:28:07 -0400 From: Joshua Kinard MIME-Version: 1.0 To: David Daney , linux-mips@linux-mips.org, ralf@linux-mips.org CC: David Daney , stable@vger.kernel.org Subject: Re: [PATCH] MIPS: Make set_pte() SMP safe. References: <1438649323-1082-1-git-send-email-ddaney.cavm@gmail.com> In-Reply-To: <1438649323-1082-1-git-send-email-ddaney.cavm@gmail.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: stable-owner@vger.kernel.org List-ID: On 08/03/2015 20:48, David Daney wrote: > From: David Daney > > 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 > Cc: > --- > 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