Re: [PATCH -v4 5/8] locking/mutex: Add lock handoff to avoid starvation

[Peter Zijlstra]

On Thu, Oct 13, 2016 at 04:14:47PM +0100, Will Deacon wrote:

> > +		if (__owner_task(owner)) {
> > +			if (handoff && unlikely(__owner_task(owner) == current)) {
> > +				/*
> > +				 * Provide ACQUIRE semantics for the lock-handoff.
> > +				 *
> > +				 * We cannot easily use load-acquire here, since
> > +				 * the actual load is a failed cmpxchg, which
> > +				 * doesn't imply any barriers.
> > +				 *
> > +				 * Also, this is a fairly unlikely scenario, and
> > +				 * this contains the cost.
> > +				 */
> > +				smp_mb(); /* ACQUIRE */
> 
> As we discussed on another thread recently, a failed cmpxchg_acquire
> will always give you ACQUIRE semantics in practice. Maybe we should update
> the documentation to allow this? The only special case is the full-barrier
> version.

So on PPC we do:

static __always_inline unsigned long
__cmpxchg_u32_acquire(u32 *p, unsigned long old, unsigned long new)
{
        unsigned long prev;

        __asm__ __volatile__ (
"1:     lwarx   %0,0,%2         # __cmpxchg_u32_acquire\n"
"       cmpw    0,%0,%3\n"
"       bne-    2f\n"
        PPC405_ERR77(0, %2)
"       stwcx.  %4,0,%2\n"
"       bne-    1b\n"
        PPC_ACQUIRE_BARRIER
        "\n"
"2:"
        : "=&r" (prev), "+m" (*p)
        : "r" (p), "r" (old), "r" (new)
        : "cc", "memory");

        return prev;
}

which I read to skip over the ACQUIRE_BARRIER on fail.


Similarly, we _could_ make the generic version skip the barrier entirely
(we currently do not it seems).


And while I agree that it makes semantic sense, in that we always issue
the LOAD, and since we defined the ACQUIRE to apply to the LOADs only,
and we always issue the LOAD, we should also always provide ACQUIRE
semantics. I'm not entirely convinced we should go there just yet. It
would make failed cmpxchg_acquire()'s more expensive, and this really is
the only place we care about those.


So I would propose for now we keep these explicit barriers; both here
and the other place you mentioned, but keep this in mind.

Also, I don't feel we need more complexity in this patch set just now.