Date: Thu, 12 Nov 2015 06:59:58 -0800
From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
To: =?iso-8859-1?Q?M=E5ns_Rullg=E5rd?= <mans@mansr.com>
Cc: Peter Zijlstra <peterz@infradead.org>, ralf@linux-mips.org,
        ddaney@caviumnetworks.com, linux-kernel@vger.kernel.org,
        Will Deacon <will.deacon@arm.com>, torvalds@linux-foundation.org,
        boqun.feng@gmail.com
Subject: Re: [RFC][PATCH] mips: Fix arch_spin_unlock()
Message-ID: <20151112145958.GX3972@linux.vnet.ibm.com>
Reply-To: paulmck@linux.vnet.ibm.com
References: <20151112123123.GZ17308@twins.programming.kicks-ass.net>
 <20151112143231.GS3972@linux.vnet.ibm.com>
 <yw1xbnazmfvr.fsf@unicorn.mansr.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <yw1xbnazmfvr.fsf@unicorn.mansr.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1962
Lines: 58

On Thu, Nov 12, 2015 at 02:50:00PM +0000, M?ns Rullg?rd wrote:
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> writes:
> 
> > On Thu, Nov 12, 2015 at 01:31:23PM +0100, Peter Zijlstra wrote:
> >> Hi
> >> 
> >> I think the MIPS arch_spin_unlock() is borken.
> >> 
> >> spin_unlock() must have RELEASE semantics, these require that no LOADs
> >> nor STOREs leak out from the critical section.
> >> 
> >> >From what I know MIPS has a relaxed memory model which allows reads to
> >> pass stores, and as implemented arch_spin_unlock() only issues a wmb
> >> which doesn't order prior reads vs later stores.
> >> 
> >> Therefore upgrade the wmb() to smp_mb().
> >> 
> >> (Also, why the unconditional wmb, as opposed to smp_wmb() ?)
> >
> > One guess is that they want to order I/O accesses within the critical
> > section?
> 
> Isn't that what mmiowb() is for?

Indeed it is.  Perhaps they didn't trust the device drivers that they
care about to use it?  Anyway, just my guess.  Just out of curiosity,
what is your guess?

							Thanx, Paul

> >> Maybe-Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> >> ---
> >> diff --git a/arch/mips/include/asm/spinlock.h b/arch/mips/include/asm/spinlock.h
> >> index 40196bebe849..b2ca13f06152 100644
> >> --- a/arch/mips/include/asm/spinlock.h
> >> +++ b/arch/mips/include/asm/spinlock.h
> >> @@ -140,7 +140,7 @@ static inline void arch_spin_lock(arch_spinlock_t *lock)
> >>  static inline void arch_spin_unlock(arch_spinlock_t *lock)
> >>  {
> >>  	unsigned int serving_now = lock->h.serving_now + 1;
> >> -	wmb();
> >> +	smp_mb();
> >>  	lock->h.serving_now = (u16)serving_now;
> >>  	nudge_writes();
> >>  }
> >> 
> >
> 
> -- 
> M?ns Rullg?rd
> mans@mansr.com
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/