Re: sem_lock() vs qspinlocks

[Manfred Spraul]

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