linux-kernel - Re: [PATCH 4/5] locking/rwsem: Avoid deceiving lock spinners

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: <1423071544.9530.107.camel@schen9-desk2.jf.intel.com>
Date:	Wed, 04 Feb 2015 09:39:04 -0800
From:	Tim Chen <tim.c.chen@...ux.intel.com>
To:	Peter Zijlstra <peterz@...radead.org>
Cc:	Jason Low <jason.low2@...com>, Davidlohr Bueso <dave@...olabs.net>,
	Ingo Molnar <mingo@...nel.org>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Michel Lespinasse <walken@...gle.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/5] locking/rwsem: Avoid deceiving lock spinners

On Wed, 2015-02-04 at 13:06 +0100, Peter Zijlstra wrote:
> On Tue, Feb 03, 2015 at 11:43:36AM -0800, Tim Chen wrote:
> > That's true.  We cannot have the lock grabbed by a new write 
> > contender as any new writer contender of the lock will be 
> > queued by the OSQ logic. Only the
> > thread doing the optimistic spin is attempting write lock.  
> > In other word, switching of write owner of the rwsem to a new
> > owner cannot happen.  Either write owner stay as the original one, or
> > we don't have a write owner.  So using test of write owner
> > switching as an indicator of congestion is incorrect.
> > 
> > If my reasoning above is sound, then the check 
> > 
> > +       if (READ_ONCE(sem->owner))
> > +               return true; /* new owner, continue spinning */
> > +
> > 
> > is unnecessary and can be removed, as we cannot have a 
> > new write owner of the rwsem, other than the thread
> > doing optimistic spinning.
> 
> I have read the rest of the thread; but the one thing that I didn't see
> is trylocks, trylocks can always come in an steal things regardless of
> the OSQ stuff.

Jason also pointed that out.  So the owner change check is needed
after all.  Now because of the OSQ logic, even if owner has changed,
the likelihood that the spinner at the head of OSQ will acquire the
lock is high.  So it should continue to spin.

That's because any new threads coming in will try lock only
once, and go to the OSQ. It is unlikely that they will trylock at
the precise moment when the owner release the lock as they do not
continue to spin on the lock.  The contention from new threads
are low.

So letting the thread at head of OSQ to continue to spin is probably
the right thing to do.

Tim

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/