Date:   Sun, 26 Feb 2023 17:15:27 +0100
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 5/6] locking/rwsem: Unify wait loop
Message-ID: <Y/uFn4WwZ8R3Dh4S@hirez.programming.kicks-ass.net>
References: <20230223122642.491637862@infradead.org>
 <20230223123319.608133045@infradead.org>
 <0be586e4-8c08-63ff-a78d-ffb5ae37adc3@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <0be586e4-8c08-63ff-a78d-ffb5ae37adc3@redhat.com>
Precedence: bulk

On Thu, Feb 23, 2023 at 05:45:56PM -0500, Waiman Long wrote:

> Unfortunately, lockevent_inc() doesn't work with waiter_type() like that as
> the compilation will fail if CONFIG_LOCK_EVENT_COUNTS is enabled.? Could you
> include the attached patch in your series and make the following changes?

Yeah, robot told me; fixed it like so:

--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -995,13 +995,16 @@ static inline void rwsem_cond_wake_waite
 	rwsem_mark_wake(sem, wake_type, wake_q);
 }
 
-#define waiter_type(_waiter, _r, _w)	\
-	((_waiter)->type == RWSEM_WAITING_FOR_READ ? (_r) : (_w))
+#define lockevent_rw_inc(rd, evr, evw)	do {	\
+	lockevent_cond_inc(evr, (rd));		\
+	lockevent_cond_inc(evw, !(rd));		\
+} while (0)
 
 static __always_inline struct rw_semaphore *
-rwsem_waiter_wait(struct rw_semaphore *sem, struct rwsem_waiter *waiter, int state)
+rwsem_waiter_wait(struct rw_semaphore *sem, struct rwsem_waiter *waiter,
+		  int state, bool reader)
 {
-	trace_contention_begin(sem, waiter_type(waiter, LCB_F_READ, LCB_F_WRITE));
+	trace_contention_begin(sem, reader ? LCB_F_READ : LCB_F_WRITE);
 
 	/* wait to be given the lock */
 	for (;;) {
@@ -1019,18 +1022,19 @@ rwsem_waiter_wait(struct rw_semaphore *s
 			break;
 		}
 		schedule_preempt_disabled();
-		lockevent_inc(waiter_type(waiter, rwsem_sleep_reader, rwsem_sleep_writer));
+		lockevent_rw_inc(reader, rwsem_sleep_reader, rwsem_sleep_writer);
 	}
 
 	__set_current_state(TASK_RUNNING);
-	lockevent_inc(waiter_type(waiter, rwsem_rlock, rwsem_wlock));
+
+	lockevent_rw_inc(reader, rwsem_rlock, rwsem_wlock);
 	trace_contention_end(sem, 0);
 	return sem;
 
 out_nolock:
 	rwsem_del_wake_waiter(sem, waiter);
 	__set_current_state(TASK_RUNNING);
-	lockevent_inc(waiter_type(waiter, rwsem_rlock_fail, rwsem_wlock_fail));
+	lockevent_rw_inc(reader, rwem_rlock_fail, rwsem_wlock_fail);
 	trace_contention_end(sem, -EINTR);
 	return ERR_PTR(-EINTR);
 }
@@ -1112,7 +1116,7 @@ rwsem_down_read_slowpath(struct rw_semap
 	if (!wake_q_empty(&wake_q))
 		wake_up_q(&wake_q);
 
-	return rwsem_waiter_wait(sem, &waiter, state);
+	return rwsem_waiter_wait(sem, &waiter, state, true);
 }
 
 /*
@@ -1162,7 +1166,7 @@ rwsem_down_write_slowpath(struct rw_sema
 	}
 	raw_spin_unlock_irq(&sem->wait_lock);
 
-	return rwsem_waiter_wait(sem, &waiter, state);
+	return rwsem_waiter_wait(sem, &waiter, state, false);
 }
 
 /*