Message-ID: <1430403123.2011.16.camel@stgolabs.net>
Subject: Re: [PATCH v3] locking/rwsem: reduce spinlock contention in wakeup
 after up_read/up_write
From: Davidlohr Bueso <dave@stgolabs.net>
To: Waiman Long <waiman.long@hp.com>
Cc: Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@kernel.org>,
        linux-kernel@vger.kernel.org, Jason Low <jason.low2@hp.com>,
        Scott J Norton <scott.norton@hp.com>,
        Douglas Hatch <doug.hatch@hp.com>
Date: Thu, 30 Apr 2015 07:12:03 -0700
In-Reply-To: <554137EC.4030700@hp.com>
References: <1429898069-28907-1-git-send-email-Waiman.Long@hp.com>
	 <1429907956.10273.65.camel@stgolabs.net> <553E9B25.4060802@hp.com>
	 <1430245038.2004.21.camel@stgolabs.net> <554137EC.4030700@hp.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1926
Lines: 42

On Wed, 2015-04-29 at 15:58 -0400, Waiman Long wrote:
> A write lock can also be acquired by a spinning writer in 
> rwsem_try_write_lock_unqueued() where wait_lock isn't used. With 
> multiple down_read's, it is possible that the first exiting reader wakes 
> up a writer who acquires the write lock while the other readers are 
> waiting for acquiring the wait_lock.

Except that readers that do the wakeup do not call __rwsem_do_wake() if
there is an active writer:

	/* If there are no active locks, wake the front queued process(es).
	 *
	 * If there are no writers and we are first in the queue,
	 * wake our own waiter to join the existing active readers !
	 */
	if (count == RWSEM_WAITING_BIAS ||
	    (count > RWSEM_WAITING_BIAS &&
	     adjustment != -RWSEM_ACTIVE_READ_BIAS))
		sem = __rwsem_do_wake(sem, RWSEM_WAKE_ANY);

And for the reader part, your rwsem_has_active_writer() check, while
avoiding the counter atomic update, would break current semantics in
that we still do the next reader grant -- also note the unlikely()
predictor ;) And this is done with the counter, so by using the owner,
you would have a race between the cmpxchg and rwsem_set_owner(). Yes,
its a small window (specially after commit 7a215f89), but there
nonetheless and could cause even more bogus wakeups. Again, I simply do
not like mixing these two -- we get away with it with the optimistic
spinning because we just iterate again, but this is not the case.

Ultimately, is this really an issue? Do you have numbers that could
justify such a change? I suspect all the benchmark results you posted in
the patch are from reducing the spinlock contention, not from this.

Thanks,
Davidlohr

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