Message-ID: <553FD3B6.3080909@ezchip.com>
Date: Tue, 28 Apr 2015 14:38:46 -0400
From: Chris Metcalf <cmetcalf@ezchip.com>
User-Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0
MIME-Version: 1.0
To: Peter Zijlstra <peterz@infradead.org>
CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>,
        Manfred Spraul <manfred@colorfullife.com>,
        Oleg Nesterov <oleg@redhat.com>, Kirill Tkhai <ktkhai@parallels.com>,
        <linux-kernel@vger.kernel.org>, Ingo Molnar <mingo@redhat.com>,
        Josh Poimboeuf <jpoimboe@redhat.com>
Subject: Re: [PATCH 2/2] [PATCH] sched: Add smp_rmb() in task rq locking cycles
References: <54E77CC0.5030401@colorfullife.com> <20150220184551.GQ2896@worktop.programming.kicks-ass.net> <20150425195602.GA26676@linux.vnet.ibm.com> <20150426105213.GA27191@linux.vnet.ibm.com> <20150428143357.GF23123@twins.programming.kicks-ass.net> <553FACF1.2020405@ezchip.com> <20150428164033.GJ5029@twins.programming.kicks-ass.net> <553FBC4F.3070104@ezchip.com> <20150428174323.GL5029@twins.programming.kicks-ass.net> <553FCAD0.9090403@ezchip.com> <20150428182410.GM5029@twins.programming.kicks-ass.net>
In-Reply-To: <20150428182410.GM5029@twins.programming.kicks-ass.net>
Content-Type: text/plain; charset="windows-1252"; format=flowed
Content-Transfer-Encoding: 7bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Apr 2015 18:38:58.7411 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB5PR02MB0773
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 1947
Lines: 55

On 04/28/2015 02:24 PM, Peter Zijlstra wrote:
> A few questions:
> On Tue, Apr 28, 2015 at 02:00:48PM -0400, Chris Metcalf wrote:
>
>>> static inline void arch_spin_lock(arch_spinlock_t *lock)
>>> {
>>> 	unsigned short head, tail;
>>>
>>> 	___tns_lock(&lock->lock); /* XXX does the TNS imply a ___sync?  */
> Does it? Something needs to provide the ACQUIRE semantics.

Yes, __insn_xxx() is a compiler barrier on tile.

Tile architectures do not need any hardware-level "acquire" semantics
since normally control dependency is sufficient for lock acquisition.
Loads and stores are issued in-order into the mesh network, but issued
loads don't block further instruction issue until a register read dependency
requires it.  There is no speculative execution.


>>> 	head = lock->head;
>>> 	lock->head++;
>>> 	___tns_unlock(&lock->lock);
>>>
>>> 	while (READ_ONCE(lock->tail) != head)
>>> 		cpu_relax();
>>> }
>>>
>>> static inline void arch_spin_unlock(arch_spinlock_t *lock)
>>> {
>>> 	/*
>>> 	 * can do with regular load/store because the lock owner
>>> 	 * is the only one going to do stores to the tail
>>> 	 */
>>> 	unsigned short tail = READ_ONCE(lock->tail);
>>> 	smp_mb(); /* MB is stronger than RELEASE */
> Note that your code uses wmb(), wmb is strictly speaking not correct,
> as its weaker than RELEASE.
>
> _However_ it doesn't make any practical difference since all three
> barriers end up emitting __sync() so its not a bug per se.

Yes, this code dates back to 2.6.18 or so; today I would use
smp_store_release().  I like the trend in the kernel to move more
towards the C11 memory order model; I think it will help a lot.

-- 
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.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/