Re: [PATCH 3/6] locking/rwsem: Rework writer wakeup

From: Peter Zijlstra <peterz@infradead.org>
To: Waiman Long <longman@redhat.com>
Cc: mingo@redhat.com, will@kernel.org, linux-kernel@vger.kernel.org,
	boqun.feng@gmail.com
Subject: Re: [PATCH 3/6] locking/rwsem: Rework writer wakeup
Date: Sun, 26 Feb 2023 12:58:29 +0100	[thread overview]
Message-ID: <Y/tJZXrDYpvdJKMh@hirez.programming.kicks-ass.net> (raw)
In-Reply-To: <bf2948c4-dd6a-1cf6-16b5-39e5e17ef72a@redhat.com>

On Thu, Feb 23, 2023 at 04:38:08PM -0500, Waiman Long wrote:

> > +static void rwsem_writer_wake(struct rw_semaphore *sem,
> > +			      struct rwsem_waiter *waiter,
> > +			      struct wake_q_head *wake_q)
> > +{
> > +	struct rwsem_waiter *first = rwsem_first_waiter(sem);
> > +	long count, new;
> > +
> > +	lockdep_assert_held(&sem->wait_lock);
> > +
> > +	count = atomic_long_read(&sem->count);
> > +	do {
> > +		bool has_handoff = !!(count & RWSEM_FLAG_HANDOFF);
> > +
> > +		if (has_handoff) {
> > +			/*
> > +			 * Honor handoff bit and yield only when the first
> > +			 * waiter is the one that set it. Otherwisee, we
> > +			 * still try to acquire the rwsem.
> > +			 */
> > +			if (first->handoff_set && (waiter != first))
> > +				return;
> > +		}
> This "if" statement if for a non-first waiter that somehow got woken up to
> have a chance to steal the lock. Now the handoff is done in the wake side
> for the first waiter, this "if" statement is not applicable and can be
> removed.

Yeah, that can be cleaned up, something like the below. But that doesn't
appear to be the cause of issues.

--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -427,25 +427,12 @@ static void rwsem_writer_wake(struct rw_
 			      struct rwsem_waiter *waiter,
 			      struct wake_q_head *wake_q)
 {
-	struct rwsem_waiter *first = rwsem_first_waiter(sem);
 	long count, new;
 
 	lockdep_assert_held(&sem->wait_lock);
 
 	count = atomic_long_read(&sem->count);
 	do {
-		bool has_handoff = !!(count & RWSEM_FLAG_HANDOFF);
-
-		if (has_handoff) {
-			/*
-			 * Honor handoff bit and yield only when the first
-			 * waiter is the one that set it. Otherwisee, we
-			 * still try to acquire the rwsem.
-			 */
-			if (first->handoff_set && (waiter != first))
-				return;
-		}
-
 		new = count;
 
 		if (count & RWSEM_LOCK_MASK) {
@@ -454,8 +441,9 @@ static void rwsem_writer_wake(struct rw_
 			 * if it is an RT task or wait in the wait queue
 			 * for too long.
 			 */
-			if (has_handoff || (!rt_task(waiter->task) &&
-					    !time_after(jiffies, waiter->timeout)))
+			if ((count & RWSEM_FLAG_HANDOFF) ||
+			    (!rt_task(waiter->task) &&
+			     !time_after(jiffies, waiter->timeout)))
 				return;
 
 			new |= RWSEM_FLAG_HANDOFF;
@@ -474,7 +462,7 @@ static void rwsem_writer_wake(struct rw_
 	 * set here to enable optimistic spinning in slowpath loop.
 	 */
 	if (new & RWSEM_FLAG_HANDOFF) {
-		first->handoff_set = true;
+		waiter->handoff_set = true;
 		lockevent_inc(rwsem_wlock_handoff);
 		return;
 	}
