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