Re: sem_lock() vs qspinlocks

From: Manfred Spraul <manfred@colorfullife.com>
To: Peter Zijlstra <peterz@infradead.org>,
	Davidlohr Bueso <dave@stgolabs.net>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	Boqun Feng <boqun.feng@gmail.com>,
	Waiman Long <Waiman.Long@hpe.com>, Ingo Molnar <mingo@kernel.org>,
	ggherdovich@suse.com, Mel Gorman <mgorman@techsingularity.net>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Paul McKenney <paulmck@linux.vnet.ibm.com>,
	Will Deacon <will.deacon@arm.com>,
	1vier1@web.de
Subject: Re: sem_lock() vs qspinlocks
Date: Sat, 21 May 2016 15:49:20 +0200	[thread overview]
Message-ID: <fad29add-e019-9af2-0220-40b9f4205568@colorfullife.com> (raw)
In-Reply-To: <20160521073739.GB15728@worktop.ger.corp.intel.com>

On 05/21/2016 09:37 AM, Peter Zijlstra wrote:
> On Fri, May 20, 2016 at 05:48:39PM -0700, Davidlohr Bueso wrote:
>> As opposed to spin_is_locked(), spin_unlock_wait() is perhaps more tempting
>> to use for locking correctness. For example, taking a look at nf_conntrack_all_lock(),
>> it too likes to get smart with spin_unlock_wait() -- also for finer graining purposes.
>> While not identical to sems, it goes like:
>>
>> nf_conntrack_all_lock():	nf_conntrack_lock():
>> spin_lock(B);			spin_lock(A);
>>
>> 				if (bar) { // false
>> bar = 1;			   ...
>> 				}
>> [loop ctrl-barrier]				
>>   spin_unlock_wait(A);
>> foo();				foo();
>>
>> If the spin_unlock_wait() doesn't yet see the store that makes A visibly locked,
>> we could end up with both threads in foo(), no?. (Although I'm unsure about that
>> ctrl barrier and archs could fall into it. The point was to see in-tree examples
>> of creative thinking with locking).
> I'm tempted to put that trailing smp_rmb() in spin_unlock_wait() too;
> because I suspect the netfilter code is broken without it.
>
> And it seems intuitive to assume that if we return from unlock_wait() we
> can indeed observe the critical section we waited on.
Then !spin_is_locked() and spin_unlock_wait() would be different with 
regards to memory barriers.
Would that really help?

My old plan was to document the rules, and define a generic 
smp_acquire__after_spin_is_unlocked.
https://lkml.org/lkml/2015/3/1/153

Noone supported it, so it ended up as 
ipc_smp_acquire__after_spin_is_unlocked().
Should we move it to linux/spinlock.h?

Who needs it?
- ipc/sem.c (but please start from the version from linux-next as 
reference, it is far less convoluted compared to the current code)
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/ipc/sem.c

- nf_conntrack

- task_rq_lock() perhaps needs smp_acquire__after_ctrl_dep
(I didn't figure out yet what happened to the proposed patch)
https://lkml.org/lkml/2015/2/17/129

--
     Manfred