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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20161017092226.GM3117@twins.programming.kicks-ass.net>
Date:   Mon, 17 Oct 2016 11:22:26 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Will Deacon <will.deacon@....com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Waiman Long <waiman.long@....com>,
        Jason Low <jason.low2@....com>,
        Ding Tianhong <dingtianhong@...wei.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        Imre Deak <imre.deak@...el.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Davidlohr Bueso <dave@...olabs.net>,
        Tim Chen <tim.c.chen@...ux.intel.com>,
        Terry Rudd <terry.rudd@....com>,
        "Paul E. McKenney" <paulmck@...ibm.com>,
        Jason Low <jason.low2@...com>,
        Chris Wilson <chris@...is-wilson.co.uk>,
        Daniel Vetter <daniel.vetter@...ll.ch>
Subject: Re: [PATCH -v4 5/8] locking/mutex: Add lock handoff to avoid
 starvation

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.