[PATCH v2 0/2] Make reader optimistic spinning optional

[PATCH v2 0/2] Make reader optimistic spinning optional

[Bongkyu Kim]

Hi,

This is rework of the following discussed patch.
https://lore.kernel.org/all/20230613043308.GA1027@KORCO045595.samsungds.net/

Changes from the previous patch
- Split to revert and modify patches
- Change according to Waiman Long's review
    More wording to documentation part
    Change module_param to early_param
    Code change by Waiman Long's suggestion

In mobile environment, reader optimistic spinning is still useful
because there're not many readers. In my test result at android device,
it improves application startup time about 3.8%
App startup time is most important factor for android user expriences.
So, re-enable reader optimistic spinning by this commit. And,
make it optional feature by cmdline.

Test result:
This is 15 application startup performance in our exynos soc.
- Cortex A78*2 + Cortex A55*6
- unit: ms (lower is better)

Application             base  opt_rspin  Diff  Diff(%)
--------------------  ------  ---------  ----  -------
* Total(geomean)         343        330   -13    +3.8%
--------------------  ------  ---------  ----  -------
helloworld               110        108    -2    +1.8%
Amazon_Seller            397        388    -9    +2.3%
Whatsapp                 311        304    -7    +2.3%
Simple_PDF_Reader        500        463   -37    +7.4%
FaceApp                  330        317   -13    +3.9%
Timestamp_Camera_Free    451        443    -8    +1.8%
Kindle                   629        597   -32    +5.1%
Coinbase                 243        233   -10    +4.1%
Firefox                  425        399   -26    +6.1%
Candy_Crush_Soda         552        538   -14    +2.5%
Hill_Climb_Racing        245        230   -15    +6.1%
Call_Recorder            437        426   -11    +2.5%
Color_Fill_3D            190        180   -10    +5.3%
eToro                    512        505    -7    +1.4%
GroupMe                  281        266   -15    +5.3%

Changes v1 -> v2:
- Move the rwsem_opt_rspin definition out of the "ifdef
  CONFIG_RWSEM_SPIN_ON_OWNER" and add _ro_after_init to rwsem_opt_rspin.

Bongkyu Kim (2):
  Revert "locking/rwsem: Remove reader optimistic spinning"
  locking/rwsem: Make reader optimistic spinning optional

 .../admin-guide/kernel-parameters.txt         |   9 +
 kernel/locking/lock_events_list.h             |   5 +-
 kernel/locking/rwsem.c                        | 297 +++++++++++++++---
 3 files changed, 263 insertions(+), 48 deletions(-)

-- 
2.36.1

[PATCH v2 1/2] Revert "locking/rwsem: Remove reader optimistic spinning"

[Bongkyu Kim]

This reverts commit 617f3ef95177840c77f59c2aec1029d27d5547d6.

In mobile environment, reader optimistic spinning is still useful
because there're not many readers. In my test result at android device,
it improves application startup time about 3.8%
App startup time is most important factor for android user expriences.
So, re-enable reader optimistic spinning by this commit. And,
the later patch will make it optional feature by cmdline.

Test result:
This is 15 application startup performance in our exynos soc.
- Cortex A78*2 + Cortex A55*6
- unit: ms (lower is better)

Application             base  opt_rspin  Diff  Diff(%)
--------------------  ------  ---------  ----  -------
* Total(geomean)         343        330   -13    +3.8%
--------------------  ------  ---------  ----  -------
helloworld               110        108    -2    +1.8%
Amazon_Seller            397        388    -9    +2.3%
Whatsapp                 311        304    -7    +2.3%
Simple_PDF_Reader        500        463   -37    +7.4%
FaceApp                  330        317   -13    +3.9%
Timestamp_Camera_Free    451        443    -8    +1.8%
Kindle                   629        597   -32    +5.1%
Coinbase                 243        233   -10    +4.1%
Firefox                  425        399   -26    +6.1%
Candy_Crush_Soda         552        538   -14    +2.5%
Hill_Climb_Racing        245        230   -15    +6.1%
Call_Recorder            437        426   -11    +2.5%
Color_Fill_3D            190        180   -10    +5.3%
eToro                    512        505    -7    +1.4%
GroupMe                  281        266   -15    +5.3%

Signed-off-by: Bongkyu Kim <bongkyu7.kim@samsung.com>
---
 kernel/locking/lock_events_list.h |   5 +-
 kernel/locking/rwsem.c            | 283 +++++++++++++++++++++++++-----
 2 files changed, 240 insertions(+), 48 deletions(-)

diff --git a/kernel/locking/lock_events_list.h b/kernel/locking/lock_events_list.h
index 97fb6f3f840a..270a0d351932 100644
--- a/kernel/locking/lock_events_list.h
+++ b/kernel/locking/lock_events_list.h
@@ -56,9 +56,12 @@ LOCK_EVENT(rwsem_sleep_reader)	/* # of reader sleeps			*/
 LOCK_EVENT(rwsem_sleep_writer)	/* # of writer sleeps			*/
 LOCK_EVENT(rwsem_wake_reader)	/* # of reader wakeups			*/
 LOCK_EVENT(rwsem_wake_writer)	/* # of writer wakeups			*/
-LOCK_EVENT(rwsem_opt_lock)	/* # of opt-acquired write locks	*/
+LOCK_EVENT(rwsem_opt_rlock)	/* # of opt-acquired read locks		*/
+LOCK_EVENT(rwsem_opt_wlock)	/* # of opt-acquired write locks	*/
 LOCK_EVENT(rwsem_opt_fail)	/* # of failed optspins			*/
 LOCK_EVENT(rwsem_opt_nospin)	/* # of disabled optspins		*/
+LOCK_EVENT(rwsem_opt_norspin)	/* # of disabled reader-only optspins	*/
+LOCK_EVENT(rwsem_opt_rlock2)	/* # of opt-acquired 2ndary read locks	*/
 LOCK_EVENT(rwsem_rlock)		/* # of read locks acquired		*/
 LOCK_EVENT(rwsem_rlock_steal)	/* # of read locks by lock stealing	*/
 LOCK_EVENT(rwsem_rlock_fast)	/* # of fast read locks acquired	*/
diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
index 9eabd585ce7a..9c0462d515c1 100644
--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -33,13 +33,19 @@
 #include "lock_events.h"
 
 /*
- * The least significant 2 bits of the owner value has the following
+ * The least significant 3 bits of the owner value has the following
  * meanings when set.
  *  - Bit 0: RWSEM_READER_OWNED - The rwsem is owned by readers
- *  - Bit 1: RWSEM_NONSPINNABLE - Cannot spin on a reader-owned lock
+ *  - Bit 1: RWSEM_RD_NONSPINNABLE - Readers cannot spin on this lock.
+ *  - Bit 2: RWSEM_WR_NONSPINNABLE - Writers cannot spin on this lock.
  *
- * When the rwsem is reader-owned and a spinning writer has timed out,
- * the nonspinnable bit will be set to disable optimistic spinning.
+ * When the rwsem is either owned by an anonymous writer, or it is
+ * reader-owned, but a spinning writer has timed out, both nonspinnable
+ * bits will be set to disable optimistic spinning by readers and writers.
+ * In the later case, the last unlocking reader should then check the
+ * writer nonspinnable bit and clear it only to give writers preference
+ * to acquire the lock via optimistic spinning, but not readers. Similar
+ * action is also done in the reader slowpath.
 
  * When a writer acquires a rwsem, it puts its task_struct pointer
  * into the owner field. It is cleared after an unlock.
@@ -55,13 +61,46 @@
  * is involved. Ideally we would like to track all the readers that own
  * a rwsem, but the overhead is simply too big.
  *
- * A fast path reader optimistic lock stealing is supported when the rwsem
- * is previously owned by a writer and the following conditions are met:
- *  - rwsem is not currently writer owned
- *  - the handoff isn't set.
+ * Reader optimistic spinning is helpful when the reader critical section
+ * is short and there aren't that many readers around. It makes readers
+ * relatively more preferred than writers. When a writer times out spinning
+ * on a reader-owned lock and set the nospinnable bits, there are two main
+ * reasons for that.
+ *
+ *  1) The reader critical section is long, perhaps the task sleeps after
+ *     acquiring the read lock.
+ *  2) There are just too many readers contending the lock causing it to
+ *     take a while to service all of them.
+ *
+ * In the former case, long reader critical section will impede the progress
+ * of writers which is usually more important for system performance. In
+ * the later case, reader optimistic spinning tends to make the reader
+ * groups that contain readers that acquire the lock together smaller
+ * leading to more of them. That may hurt performance in some cases. In
+ * other words, the setting of nonspinnable bits indicates that reader
+ * optimistic spinning may not be helpful for those workloads that cause
+ * it.
+ *
+ * Therefore, any writers that had observed the setting of the writer
+ * nonspinnable bit for a given rwsem after they fail to acquire the lock
+ * via optimistic spinning will set the reader nonspinnable bit once they
+ * acquire the write lock. Similarly, readers that observe the setting
+ * of reader nonspinnable bit at slowpath entry will set the reader
+ * nonspinnable bits when they acquire the read lock via the wakeup path.
+ *
+ * Once the reader nonspinnable bit is on, it will only be reset when
+ * a writer is able to acquire the rwsem in the fast path or somehow a
+ * reader or writer in the slowpath doesn't observe the nonspinable bit.
+ *
+ * This is to discourage reader optmistic spinning on that particular
+ * rwsem and make writers more preferred. This adaptive disabling of reader
+ * optimistic spinning will alleviate the negative side effect of this
+ * feature.
  */
 #define RWSEM_READER_OWNED	(1UL << 0)
-#define RWSEM_NONSPINNABLE	(1UL << 1)
+#define RWSEM_RD_NONSPINNABLE	(1UL << 1)
+#define RWSEM_WR_NONSPINNABLE	(1UL << 2)
+#define RWSEM_NONSPINNABLE	(RWSEM_RD_NONSPINNABLE | RWSEM_WR_NONSPINNABLE)
 #define RWSEM_OWNER_FLAGS_MASK	(RWSEM_READER_OWNED | RWSEM_NONSPINNABLE)
 
 #ifdef CONFIG_DEBUG_RWSEMS
@@ -171,7 +210,7 @@ static inline void __rwsem_set_reader_owned(struct rw_semaphore *sem,
 					    struct task_struct *owner)
 {
 	unsigned long val = (unsigned long)owner | RWSEM_READER_OWNED |
-		(atomic_long_read(&sem->owner) & RWSEM_NONSPINNABLE);
+		(atomic_long_read(&sem->owner) & RWSEM_RD_NONSPINNABLE);
 
 	atomic_long_set(&sem->owner, val);
 }
@@ -341,6 +380,7 @@ struct rwsem_waiter {
 	enum rwsem_waiter_type type;
 	unsigned long timeout;
 	bool handoff_set;
+	unsigned long last_rowner;
 };
 #define rwsem_first_waiter(sem) \
 	list_first_entry(&sem->wait_list, struct rwsem_waiter, list)
@@ -480,6 +520,10 @@ static void rwsem_mark_wake(struct rw_semaphore *sem,
 		 * the reader is copied over.
 		 */
 		owner = waiter->task;
+		if (waiter->last_rowner & RWSEM_RD_NONSPINNABLE) {
+			owner = (void *)((unsigned long)owner | RWSEM_RD_NONSPINNABLE);
+			lockevent_inc(rwsem_opt_norspin);
+		}
 		__rwsem_set_reader_owned(sem, owner);
 	}
 
@@ -684,6 +728,30 @@ enum owner_state {
 };
 
 #ifdef CONFIG_RWSEM_SPIN_ON_OWNER
+/*
+ * Try to acquire read lock before the reader is put on wait queue.
+ * Lock acquisition isn't allowed if the rwsem is locked or a writer handoff
+ * is ongoing.
+ */
+static inline bool rwsem_try_read_lock_unqueued(struct rw_semaphore *sem)
+{
+	long count = atomic_long_read(&sem->count);
+
+	if (count & (RWSEM_WRITER_MASK | RWSEM_FLAG_HANDOFF))
+		return false;
+
+	count = atomic_long_fetch_add_acquire(RWSEM_READER_BIAS, &sem->count);
+	if (!(count & (RWSEM_WRITER_MASK | RWSEM_FLAG_HANDOFF))) {
+		rwsem_set_reader_owned(sem);
+		lockevent_inc(rwsem_opt_rlock);
+		return true;
+	}
+
+	/* Back out the change */
+	atomic_long_add(-RWSEM_READER_BIAS, &sem->count);
+	return false;
+}
+
 /*
  * Try to acquire write lock before the writer has been put on wait queue.
  */
@@ -695,14 +763,15 @@ static inline bool rwsem_try_write_lock_unqueued(struct rw_semaphore *sem)
 		if (atomic_long_try_cmpxchg_acquire(&sem->count, &count,
 					count | RWSEM_WRITER_LOCKED)) {
 			rwsem_set_owner(sem);
-			lockevent_inc(rwsem_opt_lock);
+			lockevent_inc(rwsem_opt_wlock);
 			return true;
 		}
 	}
 	return false;
 }
 
-static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
+static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem,
+					   unsigned long nonspinnable)
 {
 	struct task_struct *owner;
 	unsigned long flags;
@@ -721,7 +790,7 @@ static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
 	/*
 	 * Don't check the read-owner as the entry may be stale.
 	 */
-	if ((flags & RWSEM_NONSPINNABLE) ||
+	if ((flags & nonspinnable) ||
 	    (owner && !(flags & RWSEM_READER_OWNED) && !owner_on_cpu(owner)))
 		ret = false;
 
@@ -732,9 +801,9 @@ static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
 #define OWNER_SPINNABLE		(OWNER_NULL | OWNER_WRITER | OWNER_READER)
 
 static inline enum owner_state
-rwsem_owner_state(struct task_struct *owner, unsigned long flags)
+rwsem_owner_state(struct task_struct *owner, unsigned long flags, unsigned long nonspinnable)
 {
-	if (flags & RWSEM_NONSPINNABLE)
+	if (flags & nonspinnable)
 		return OWNER_NONSPINNABLE;
 
 	if (flags & RWSEM_READER_OWNED)
@@ -744,7 +813,7 @@ rwsem_owner_state(struct task_struct *owner, unsigned long flags)
 }
 
 static noinline enum owner_state
-rwsem_spin_on_owner(struct rw_semaphore *sem)
+rwsem_spin_on_owner(struct rw_semaphore *sem, unsigned long nonspinnable)
 {
 	struct task_struct *new, *owner;
 	unsigned long flags, new_flags;
@@ -753,7 +822,7 @@ rwsem_spin_on_owner(struct rw_semaphore *sem)
 	lockdep_assert_preemption_disabled();
 
 	owner = rwsem_owner_flags(sem, &flags);
-	state = rwsem_owner_state(owner, flags);
+	state = rwsem_owner_state(owner, flags, nonspinnable);
 	if (state != OWNER_WRITER)
 		return state;
 
@@ -766,7 +835,7 @@ rwsem_spin_on_owner(struct rw_semaphore *sem)
 		 */
 		new = rwsem_owner_flags(sem, &new_flags);
 		if ((new != owner) || (new_flags != flags)) {
-			state = rwsem_owner_state(new, new_flags);
+			state = rwsem_owner_state(new, new_flags, nonspinnable);
 			break;
 		}
 
@@ -816,12 +885,14 @@ static inline u64 rwsem_rspin_threshold(struct rw_semaphore *sem)
 	return sched_clock() + delta;
 }
 
-static bool rwsem_optimistic_spin(struct rw_semaphore *sem)
+static bool rwsem_optimistic_spin(struct rw_semaphore *sem, bool wlock)
 {
 	bool taken = false;
 	int prev_owner_state = OWNER_NULL;
 	int loop = 0;
 	u64 rspin_threshold = 0;
+	unsigned long nonspinnable = wlock ? RWSEM_WR_NONSPINNABLE
+					   : RWSEM_RD_NONSPINNABLE;
 
 	/* sem->wait_lock should not be held when doing optimistic spinning */
 	if (!osq_lock(&sem->osq))
@@ -836,14 +907,15 @@ static bool rwsem_optimistic_spin(struct rw_semaphore *sem)
 	for (;;) {
 		enum owner_state owner_state;
 
-		owner_state = rwsem_spin_on_owner(sem);
+		owner_state = rwsem_spin_on_owner(sem, nonspinnable);
 		if (!(owner_state & OWNER_SPINNABLE))
 			break;
 
 		/*
 		 * Try to acquire the lock
 		 */
-		taken = rwsem_try_write_lock_unqueued(sem);
+		taken = wlock ? rwsem_try_write_lock_unqueued(sem)
+			      : rwsem_try_read_lock_unqueued(sem);
 
 		if (taken)
 			break;
@@ -851,7 +923,7 @@ static bool rwsem_optimistic_spin(struct rw_semaphore *sem)
 		/*
 		 * Time-based reader-owned rwsem optimistic spinning
 		 */
-		if (owner_state == OWNER_READER) {
+		if (wlock && (owner_state == OWNER_READER)) {
 			/*
 			 * Re-initialize rspin_threshold every time when
 			 * the owner state changes from non-reader to reader.
@@ -860,7 +932,7 @@ static bool rwsem_optimistic_spin(struct rw_semaphore *sem)
 			 * the beginning of the 2nd reader phase.
 			 */
 			if (prev_owner_state != OWNER_READER) {
-				if (rwsem_test_oflags(sem, RWSEM_NONSPINNABLE))
+				if (rwsem_test_oflags(sem, nonspinnable))
 					break;
 				rspin_threshold = rwsem_rspin_threshold(sem);
 				loop = 0;
@@ -935,30 +1007,89 @@ static bool rwsem_optimistic_spin(struct rw_semaphore *sem)
 }
 
 /*
- * Clear the owner's RWSEM_NONSPINNABLE bit if it is set. This should
+ * Clear the owner's RWSEM_WR_NONSPINNABLE bit if it is set. This should
  * only be called when the reader count reaches 0.
+ *
+ * This give writers better chance to acquire the rwsem first before
+ * readers when the rwsem was being held by readers for a relatively long
+ * period of time. Race can happen that an optimistic spinner may have
+ * just stolen the rwsem and set the owner, but just clearing the
+ * RWSEM_WR_NONSPINNABLE bit will do no harm anyway.
  */
-static inline void clear_nonspinnable(struct rw_semaphore *sem)
+static inline void clear_wr_nonspinnable(struct rw_semaphore *sem)
 {
-	if (unlikely(rwsem_test_oflags(sem, RWSEM_NONSPINNABLE)))
-		atomic_long_andnot(RWSEM_NONSPINNABLE, &sem->owner);
+	if (unlikely(rwsem_test_oflags(sem, RWSEM_WR_NONSPINNABLE)))
+		atomic_long_andnot(RWSEM_WR_NONSPINNABLE, &sem->owner);
+}
+
+/*
+ * This function is called when the reader fails to acquire the lock via
+ * optimistic spinning. In this case we will still attempt to do a trylock
+ * when comparing the rwsem state right now with the state when entering
+ * the slowpath indicates that the reader is still in a valid reader phase.
+ * This happens when the following conditions are true:
+ *
+ * 1) The lock is currently reader owned, and
+ * 2) The lock is previously not reader-owned or the last read owner changes.
+ *
+ * In the former case, we have transitioned from a writer phase to a
+ * reader-phase while spinning. In the latter case, it means the reader
+ * phase hasn't ended when we entered the optimistic spinning loop. In
+ * both cases, the reader is eligible to acquire the lock. This is the
+ * secondary path where a read lock is acquired optimistically.
+ *
+ * The reader non-spinnable bit wasn't set at time of entry or it will
+ * not be here at all.
+ */
+static inline bool rwsem_reader_phase_trylock(struct rw_semaphore *sem,
+					      unsigned long last_rowner)
+{
+	unsigned long owner = atomic_long_read(&sem->owner);
+
+	if (!(owner & RWSEM_READER_OWNED))
+		return false;
+
+	if (((owner ^ last_rowner) & ~RWSEM_OWNER_FLAGS_MASK) &&
+	    rwsem_try_read_lock_unqueued(sem)) {
+		lockevent_inc(rwsem_opt_rlock2);
+		lockevent_add(rwsem_opt_fail, -1);
+		return true;
+	}
+	return false;
+}
+
+static inline bool rwsem_no_spinners(struct rw_semaphore *sem)
+{
+	return !osq_is_locked(&sem->osq);
 }
 
 #else
-static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
+static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem,
+					   unsigned long nonspinnable)
 {
 	return false;
 }
 
-static inline bool rwsem_optimistic_spin(struct rw_semaphore *sem)
+static inline bool rwsem_optimistic_spin(struct rw_semaphore *sem, bool wlock)
 {
 	return false;
 }
 
-static inline void clear_nonspinnable(struct rw_semaphore *sem) { }
+static inline void clear_wr_nonspinnable(struct rw_semaphore *sem) { }
+
+static inline bool rwsem_reader_phase_trylock(struct rw_semaphore *sem,
+					      unsigned long last_rowner)
+{
+	return false;
+}
+
+static inline bool rwsem_no_spinners(sem)
+{
+	return false;
+}
 
 static inline enum owner_state
-rwsem_spin_on_owner(struct rw_semaphore *sem)
+rwsem_spin_on_owner(struct rw_semaphore *sem, unsigned long nonspinnable)
 {
 	return OWNER_NONSPINNABLE;
 }
@@ -984,7 +1115,7 @@ static inline void rwsem_cond_wake_waiter(struct rw_semaphore *sem, long count,
 		wake_type = RWSEM_WAKE_READERS;
 	} else {
 		wake_type = RWSEM_WAKE_ANY;
-		clear_nonspinnable(sem);
+		clear_wr_nonspinnable(sem);
 	}
 	rwsem_mark_wake(sem, wake_type, wake_q);
 }
@@ -995,32 +1126,61 @@ static inline void rwsem_cond_wake_waiter(struct rw_semaphore *sem, long count,
 static struct rw_semaphore __sched *
 rwsem_down_read_slowpath(struct rw_semaphore *sem, long count, unsigned int state)
 {
-	long adjustment = -RWSEM_READER_BIAS;
+	long owner, adjustment = -RWSEM_READER_BIAS;
 	long rcnt = (count >> RWSEM_READER_SHIFT);
 	struct rwsem_waiter waiter;
 	DEFINE_WAKE_Q(wake_q);
 
 	/*
 	 * To prevent a constant stream of readers from starving a sleeping
-	 * waiter, don't attempt optimistic lock stealing if the lock is
-	 * currently owned by readers.
+	 * waiter, don't attempt optimistic spinning if the lock is currently
+	 * owned by readers.
 	 */
-	if ((atomic_long_read(&sem->owner) & RWSEM_READER_OWNED) &&
-	    (rcnt > 1) && !(count & RWSEM_WRITER_LOCKED))
+	owner = atomic_long_read(&sem->owner);
+	if ((owner & RWSEM_READER_OWNED) && (rcnt > 1) &&
+	   !(count & RWSEM_WRITER_LOCKED))
 		goto queue;
 
 	/*
-	 * Reader optimistic lock stealing.
+	 * Reader optimistic lock stealing
+	 *
+	 * We can take the read lock directly without doing
+	 * rwsem_optimistic_spin() if the conditions are right.
+	 * Also wake up other readers if it is the first reader.
 	 */
-	if (!(count & (RWSEM_WRITER_LOCKED | RWSEM_FLAG_HANDOFF))) {
+	if (!(count & (RWSEM_WRITER_LOCKED | RWSEM_FLAG_HANDOFF)) &&
+	    rwsem_no_spinners(sem)) {
 		rwsem_set_reader_owned(sem);
 		lockevent_inc(rwsem_rlock_steal);
+		if (rcnt == 1)
+			goto wake_readers;
+		return sem;
+	}
+
+	/*
+	 * Save the current read-owner of rwsem, if available, and the
+	 * reader nonspinnable bit.
+	 */
+	waiter.last_rowner = owner;
+	if (!(waiter.last_rowner & RWSEM_READER_OWNED))
+		waiter.last_rowner &= RWSEM_RD_NONSPINNABLE;
+
+	if (!rwsem_can_spin_on_owner(sem, RWSEM_RD_NONSPINNABLE))
+		goto queue;
 
+	/*
+	 * Undo read bias from down_read() and do optimistic spinning.
+	 */
+	atomic_long_add(-RWSEM_READER_BIAS, &sem->count);
+	adjustment = 0;
+	if (rwsem_optimistic_spin(sem, false)) {
+		/* rwsem_optimistic_spin() implies ACQUIRE on success */
 		/*
-		 * Wake up other readers in the wait queue if it is
-		 * the first reader.
+		 * Wake up other readers in the wait list if the front
+		 * waiter is a reader.
 		 */
-		if ((rcnt == 1) && (count & RWSEM_FLAG_WAITERS)) {
+wake_readers:
+		if ((atomic_long_read(&sem->count) & RWSEM_FLAG_WAITERS)) {
 			raw_spin_lock_irq(&sem->wait_lock);
 			if (!list_empty(&sem->wait_list))
 				rwsem_mark_wake(sem, RWSEM_WAKE_READ_OWNED,
@@ -1029,6 +1189,9 @@ rwsem_down_read_slowpath(struct rw_semaphore *sem, long count, unsigned int stat
 			wake_up_q(&wake_q);
 		}
 		return sem;
+	} else if (rwsem_reader_phase_trylock(sem, waiter.last_rowner)) {
+		/* rwsem_reader_phase_trylock() implies ACQUIRE on success */
+		return sem;
 	}
 
 queue:
@@ -1045,7 +1208,7 @@ rwsem_down_read_slowpath(struct rw_semaphore *sem, long count, unsigned int stat
 		 * immediately as its RWSEM_READER_BIAS has already been set
 		 * in the count.
 		 */
-		if (!(atomic_long_read(&sem->count) & RWSEM_WRITER_MASK)) {
+		if (adjustment && !(atomic_long_read(&sem->count) & RWSEM_WRITER_MASK)) {
 			/* Provide lock ACQUIRE */
 			smp_acquire__after_ctrl_dep();
 			raw_spin_unlock_irq(&sem->wait_lock);
@@ -1058,7 +1221,10 @@ rwsem_down_read_slowpath(struct rw_semaphore *sem, long count, unsigned int stat
 	rwsem_add_waiter(sem, &waiter);
 
 	/* we're now waiting on the lock, but no longer actively locking */
-	count = atomic_long_add_return(adjustment, &sem->count);
+	if (adjustment)
+		count = atomic_long_add_return(adjustment, &sem->count);
+	else
+		count = atomic_long_read(&sem->count);
 
 	rwsem_cond_wake_waiter(sem, count, &wake_q);
 	raw_spin_unlock_irq(&sem->wait_lock);
@@ -1100,21 +1266,43 @@ rwsem_down_read_slowpath(struct rw_semaphore *sem, long count, unsigned int stat
 	return ERR_PTR(-EINTR);
 }
 
+/*
+ * This function is called by the a write lock owner. So the owner value
+ * won't get changed by others.
+ */
+static inline void rwsem_disable_reader_optspin(struct rw_semaphore *sem,
+						bool disable)
+{
+	if (unlikely(disable)) {
+		atomic_long_or(RWSEM_RD_NONSPINNABLE, &sem->owner);
+		lockevent_inc(rwsem_opt_norspin);
+	}
+}
+
 /*
  * Wait until we successfully acquire the write lock
  */
 static struct rw_semaphore __sched *
 rwsem_down_write_slowpath(struct rw_semaphore *sem, int state)
 {
+	bool disable_rspin;
 	struct rwsem_waiter waiter;
 	DEFINE_WAKE_Q(wake_q);
 
 	/* do optimistic spinning and steal lock if possible */
-	if (rwsem_can_spin_on_owner(sem) && rwsem_optimistic_spin(sem)) {
+	if (rwsem_can_spin_on_owner(sem, RWSEM_WR_NONSPINNABLE) &&
+	    rwsem_optimistic_spin(sem, true)) {
 		/* rwsem_optimistic_spin() implies ACQUIRE on success */
 		return sem;
 	}
 
+	/*
+	 * Disable reader optimistic spinning for this rwsem after
+	 * acquiring the write lock when the setting of the nonspinnable
+	 * bits are observed.
+	 */
+	disable_rspin = atomic_long_read(&sem->owner) & RWSEM_NONSPINNABLE;
+
 	/*
 	 * Optimistic spinning failed, proceed to the slowpath
 	 * and block until we can acquire the sem.
@@ -1170,7 +1358,7 @@ rwsem_down_write_slowpath(struct rw_semaphore *sem, int state)
 		if (waiter.handoff_set) {
 			enum owner_state owner_state;
 
-			owner_state = rwsem_spin_on_owner(sem);
+			owner_state = rwsem_spin_on_owner(sem, RWSEM_NONSPINNABLE);
 			if (owner_state == OWNER_NULL)
 				goto trylock_again;
 		}
@@ -1182,6 +1370,7 @@ rwsem_down_write_slowpath(struct rw_semaphore *sem, int state)
 		raw_spin_lock_irq(&sem->wait_lock);
 	}
 	__set_current_state(TASK_RUNNING);
+	rwsem_disable_reader_optspin(sem, disable_rspin);
 	raw_spin_unlock_irq(&sem->wait_lock);
 	lockevent_inc(rwsem_wlock);
 	trace_contention_end(sem, 0);
@@ -1348,7 +1537,7 @@ static inline void __up_read(struct rw_semaphore *sem)
 	DEBUG_RWSEMS_WARN_ON(tmp < 0, sem);
 	if (unlikely((tmp & (RWSEM_LOCK_MASK|RWSEM_FLAG_WAITERS)) ==
 		      RWSEM_FLAG_WAITERS)) {
-		clear_nonspinnable(sem);
+		clear_wr_nonspinnable(sem);
 		rwsem_wake(sem);
 	}
 	preempt_enable();
-- 
2.36.1

Re: [PATCH v2 1/2] Revert "locking/rwsem: Remove reader optimistic spinning"

[Peter Zijlstra]

On Fri, Sep 01, 2023 at 10:07:03AM +0900, Bongkyu Kim wrote:
> This reverts commit 617f3ef95177840c77f59c2aec1029d27d5547d6.
> 
> In mobile environment, reader optimistic spinning is still useful
> because there're not many readers. In my test result at android device,
> it improves application startup time about 3.8%
> App startup time is most important factor for android user expriences.
> So, re-enable reader optimistic spinning by this commit. And,
> the later patch will make it optional feature by cmdline.

I'm not seeing any mention on how this interacts with all the rwsem work
that has been done since that commit, like the handoff rework.

Why is a straight revert a sane thing at this point?

Re: [PATCH v2 1/2] Revert "locking/rwsem: Remove reader optimistic spinning"

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Fri, Sep 01, 2023 at 10:07:03AM +0900, Bongkyu Kim wrote:
> > This reverts commit 617f3ef95177840c77f59c2aec1029d27d5547d6.
> > 
> > In mobile environment, reader optimistic spinning is still useful
> > because there're not many readers. In my test result at android device,
> > it improves application startup time about 3.8%
> > App startup time is most important factor for android user expriences.
> > So, re-enable reader optimistic spinning by this commit. And,
> > the later patch will make it optional feature by cmdline.
> 
> I'm not seeing any mention on how this interacts with all the rwsem work
> that has been done since that commit, like the handoff rework.
> 
> Why is a straight revert a sane thing at this point?

Yeah, so this should probably be titled:

   locking/rwsem: Reintroduce reader optimistic spinning

... instead of the double-negative 'remove removal' thing that is indeed 
confusing.

Thanks,

	Ingo

Re: [PATCH v2 1/2] Revert "locking/rwsem: Remove reader optimistic spinning"

[Waiman Long]

On 9/4/23 11:10, Peter Zijlstra wrote:
> On Fri, Sep 01, 2023 at 10:07:03AM +0900, Bongkyu Kim wrote:
>> This reverts commit 617f3ef95177840c77f59c2aec1029d27d5547d6.
>>
>> In mobile environment, reader optimistic spinning is still useful
>> because there're not many readers. In my test result at android device,
>> it improves application startup time about 3.8%
>> App startup time is most important factor for android user expriences.
>> So, re-enable reader optimistic spinning by this commit. And,
>> the later patch will make it optional feature by cmdline.
> I'm not seeing any mention on how this interacts with all the rwsem work
> that has been done since that commit, like the handoff rework.
>
> Why is a straight revert a sane thing at this point?

I also agree that a revert is not the best way to reintroduce the 
feature. It should document the reason why reader optimistic spinning is 
not the default as discussed in commit 617f3ef9517 ("locking/rwsem: 
Remove reader optimistic spinning") and under what condition should 
reader optimistic spinning can be turned back on.

Besides, I now think we may not really need 2 separate nonspinnable 
bits. We can go with one that is set by writer timing out when spinning 
on reader.

Cheers,
Longman

Re: [PATCH v2 1/2] Revert "locking/rwsem: Remove reader optimistic spinning"

[Bongkyu Kim]

[-- Attachment #1: Type: text/plain, Size: 1598 bytes --]

On Mon, Sep 04, 2023 at 03:56:56PM -0400, Waiman Long wrote:
> On 9/4/23 11:10, Peter Zijlstra wrote:
> > On Fri, Sep 01, 2023 at 10:07:03AM +0900, Bongkyu Kim wrote:
> > > This reverts commit 617f3ef95177840c77f59c2aec1029d27d5547d6.
> > > 
> > > In mobile environment, reader optimistic spinning is still useful
> > > because there're not many readers. In my test result at android device,
> > > it improves application startup time about 3.8%
> > > App startup time is most important factor for android user expriences.
> > > So, re-enable reader optimistic spinning by this commit. And,
> > > the later patch will make it optional feature by cmdline.
> > I'm not seeing any mention on how this interacts with all the rwsem work
> > that has been done since that commit, like the handoff rework.
> > 
> > Why is a straight revert a sane thing at this point?
> 
> I also agree that a revert is not the best way to reintroduce the feature.
> It should document the reason why reader optimistic spinning is not the
> default as discussed in commit 617f3ef9517 ("locking/rwsem: Remove reader
> optimistic spinning") and under what condition should reader optimistic
> spinning can be turned back on.
> 
> Besides, I now think we may not really need 2 separate nonspinnable bits. We
> can go with one that is set by writer timing out when spinning on reader.
> 
> Cheers,
> Longman

Should I modify like the below?
- Title to "locking/rwsem: Reintroduce reader optimistic spinning"
- Add more document like Longman's comment
- Reconsidering about 2 separate nonspinnable bits to one

Thanks,
Bongkyu

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 1/2] Revert "locking/rwsem: Remove reader optimistic spinning"

[Waiman Long]

On 9/6/23 07:27, Bongkyu Kim wrote:
> On Mon, Sep 04, 2023 at 03:56:56PM -0400, Waiman Long wrote:
>> On 9/4/23 11:10, Peter Zijlstra wrote:
>>> On Fri, Sep 01, 2023 at 10:07:03AM +0900, Bongkyu Kim wrote:
>>>> This reverts commit 617f3ef95177840c77f59c2aec1029d27d5547d6.
>>>>
>>>> In mobile environment, reader optimistic spinning is still useful
>>>> because there're not many readers. In my test result at android device,
>>>> it improves application startup time about 3.8%
>>>> App startup time is most important factor for android user expriences.
>>>> So, re-enable reader optimistic spinning by this commit. And,
>>>> the later patch will make it optional feature by cmdline.
>>> I'm not seeing any mention on how this interacts with all the rwsem work
>>> that has been done since that commit, like the handoff rework.
>>>
>>> Why is a straight revert a sane thing at this point?
>> I also agree that a revert is not the best way to reintroduce the feature.
>> It should document the reason why reader optimistic spinning is not the
>> default as discussed in commit 617f3ef9517 ("locking/rwsem: Remove reader
>> optimistic spinning") and under what condition should reader optimistic
>> spinning can be turned back on.
>>
>> Besides, I now think we may not really need 2 separate nonspinnable bits. We
>> can go with one that is set by writer timing out when spinning on reader.
>>
>> Cheers,
>> Longman
> Should I modify like the below?
> - Title to "locking/rwsem: Reintroduce reader optimistic spinning"
> - Add more document like Longman's comment
> - Reconsidering about 2 separate nonspinnable bits to one

Besides the above, Peter also ask to verify that it won't affect handoff 
handling which requires that an unlocker see the lock will be free and 
wake up the head of the wait queue. Given the fact that the simple 
heuristic of skipping optimistic spinning if the lock is reader owned is 
kept, that shouldn't be a problem, but you still need to document that.

Cheers,
Longman

Re: [PATCH v2 1/2] Revert "locking/rwsem: Remove reader optimistic spinning"

[Bongkyu Kim]

[-- Attachment #1: Type: text/plain, Size: 2415 bytes --]

On Wed, Sep 06, 2023 at 09:32:45AM -0400, Waiman Long wrote:
> 
> On 9/6/23 07:27, Bongkyu Kim wrote:
> > On Mon, Sep 04, 2023 at 03:56:56PM -0400, Waiman Long wrote:
> > > On 9/4/23 11:10, Peter Zijlstra wrote:
> > > > On Fri, Sep 01, 2023 at 10:07:03AM +0900, Bongkyu Kim wrote:
> > > > > This reverts commit 617f3ef95177840c77f59c2aec1029d27d5547d6.
> > > > > 
> > > > > In mobile environment, reader optimistic spinning is still useful
> > > > > because there're not many readers. In my test result at android device,
> > > > > it improves application startup time about 3.8%
> > > > > App startup time is most important factor for android user expriences.
> > > > > So, re-enable reader optimistic spinning by this commit. And,
> > > > > the later patch will make it optional feature by cmdline.
> > > > I'm not seeing any mention on how this interacts with all the rwsem work
> > > > that has been done since that commit, like the handoff rework.
> > > > 
> > > > Why is a straight revert a sane thing at this point?
> > > I also agree that a revert is not the best way to reintroduce the feature.
> > > It should document the reason why reader optimistic spinning is not the
> > > default as discussed in commit 617f3ef9517 ("locking/rwsem: Remove reader
> > > optimistic spinning") and under what condition should reader optimistic
> > > spinning can be turned back on.
> > > 
> > > Besides, I now think we may not really need 2 separate nonspinnable bits. We
> > > can go with one that is set by writer timing out when spinning on reader.
> > > 
> > > Cheers,
> > > Longman
> > Should I modify like the below?
> > - Title to "locking/rwsem: Reintroduce reader optimistic spinning"
> > - Add more document like Longman's comment
> > - Reconsidering about 2 separate nonspinnable bits to one
> 
> Besides the above, Peter also ask to verify that it won't affect handoff
> handling which requires that an unlocker see the lock will be free and wake
> up the head of the wait queue. Given the fact that the simple heuristic of
> skipping optimistic spinning if the lock is reader owned is kept, that
> shouldn't be a problem, but you still need to document that.
> 
> Cheers,
> Longman
 
I've been reviewing nonspinnable bits for several days, but I can't find the
way for one spinnable bit. In this patch, how about modify only already mentioned
document part? About one spinnable bit, we will discuss later.

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH v2 2/2] locking/rwsem: Make reader optimistic spinning optional

[Bongkyu Kim]

Disable reader optimistic spinning by default. And, can enable it
by "rwsem.opt_rspin" cmdline.

Also, fix compile error without CONFIG_RWSEM_SPIN_ON_OWNER (reported by
kernel test robot)

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202306010043.VJHcuCnb-lkp@intel.com/
Signed-off-by: Bongkyu Kim <bongkyu7.kim@samsung.com>
---
 Documentation/admin-guide/kernel-parameters.txt |  9 +++++++++
 kernel/locking/rwsem.c                          | 16 +++++++++++++++-
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 772b54df084b..adf16a07fe4d 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -5605,6 +5605,15 @@
 
 	rw		[KNL] Mount root device read-write on boot
 
+	rwsem.opt_rspin [KNL]
+			Use rwsem reader optimistic spinning. Reader optimistic
+			spinning is helpful when the reader critical section is
+			short and there aren't that many readers around.
+			For example, enable this option may improve performance
+			in mobile workload that there're not many readers, but
+			may reduce performance in server workload that there're
+			many readers.
+
 	S		[KNL] Run init in single mode
 
 	s390_iommu=	[HW,S390]
diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
index 9c0462d515c1..47c467880af5 100644
--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -117,6 +117,17 @@
 # define DEBUG_RWSEMS_WARN_ON(c, sem)
 #endif
 
+static bool __ro_after_init rwsem_opt_rspin;
+
+static int __init opt_rspin(char *str)
+{
+        rwsem_opt_rspin = true;
+
+        return 0;
+}
+
+early_param("rwsem.opt_rspin", opt_rspin);
+
 /*
  * On 64-bit architectures, the bit definitions of the count are:
  *
@@ -1083,7 +1094,7 @@ static inline bool rwsem_reader_phase_trylock(struct rw_semaphore *sem,
 	return false;
 }
 
-static inline bool rwsem_no_spinners(sem)
+static inline bool rwsem_no_spinners(struct rw_semaphore *sem)
 {
 	return false;
 }
@@ -1157,6 +1168,9 @@ rwsem_down_read_slowpath(struct rw_semaphore *sem, long count, unsigned int stat
 		return sem;
 	}
 
+	if (!IS_ENABLED(CONFIG_RWSEM_SPIN_ON_OWNER) || !rwsem_opt_rspin)
+		goto queue;
+
 	/*
 	 * Save the current read-owner of rwsem, if available, and the
 	 * reader nonspinnable bit.
-- 
2.36.1

Re: [PATCH v2 2/2] locking/rwsem: Make reader optimistic spinning optional

[Peter Zijlstra]

On Fri, Sep 01, 2023 at 10:07:04AM +0900, Bongkyu Kim wrote:

> diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
> index 9c0462d515c1..47c467880af5 100644
> --- a/kernel/locking/rwsem.c
> +++ b/kernel/locking/rwsem.c
> @@ -117,6 +117,17 @@
>  # define DEBUG_RWSEMS_WARN_ON(c, sem)
>  #endif
>  
> +static bool __ro_after_init rwsem_opt_rspin;
> +
> +static int __init opt_rspin(char *str)
> +{
> +        rwsem_opt_rspin = true;
> +
> +        return 0;
> +}
> +
> +early_param("rwsem.opt_rspin", opt_rspin);
> +
>  /*
>   * On 64-bit architectures, the bit definitions of the count are:
>   *
> @@ -1083,7 +1094,7 @@ static inline bool rwsem_reader_phase_trylock(struct rw_semaphore *sem,
>  	return false;
>  }
>  
> -static inline bool rwsem_no_spinners(sem)
> +static inline bool rwsem_no_spinners(struct rw_semaphore *sem)
>  {
>  	return false;
>  }
> @@ -1157,6 +1168,9 @@ rwsem_down_read_slowpath(struct rw_semaphore *sem, long count, unsigned int stat
>  		return sem;
>  	}
>  
> +	if (!IS_ENABLED(CONFIG_RWSEM_SPIN_ON_OWNER) || !rwsem_opt_rspin)
> +		goto queue;
> +

At the very least this should be a static_branch(), but I still very
much want an answer on how all this interacts with the handoff stuff.

Re: [PATCH v2 0/2] Make reader optimistic spinning optional

[John Stultz]

On Thu, Aug 31, 2023 at 6:07 PM Bongkyu Kim <bongkyu7.kim@samsung.com> wrote:
>
> This is rework of the following discussed patch.
> https://lore.kernel.org/all/20230613043308.GA1027@KORCO045595.samsungds.net/
>
> Changes from the previous patch
> - Split to revert and modify patches
> - Change according to Waiman Long's review
>     More wording to documentation part
>     Change module_param to early_param
>     Code change by Waiman Long's suggestion
>
> In mobile environment, reader optimistic spinning is still useful
> because there're not many readers. In my test result at android device,
> it improves application startup time about 3.8%
> App startup time is most important factor for android user expriences.
> So, re-enable reader optimistic spinning by this commit. And,
> make it optional feature by cmdline.
>
> Test result:
> This is 15 application startup performance in our exynos soc.
> - Cortex A78*2 + Cortex A55*6
> - unit: ms (lower is better)
>
> Application             base  opt_rspin  Diff  Diff(%)
> --------------------  ------  ---------  ----  -------
> * Total(geomean)         343        330   -13    +3.8%
> --------------------  ------  ---------  ----  -------
> helloworld               110        108    -2    +1.8%
> Amazon_Seller            397        388    -9    +2.3%
> Whatsapp                 311        304    -7    +2.3%
> Simple_PDF_Reader        500        463   -37    +7.4%
> FaceApp                  330        317   -13    +3.9%
> Timestamp_Camera_Free    451        443    -8    +1.8%
> Kindle                   629        597   -32    +5.1%
> Coinbase                 243        233   -10    +4.1%
> Firefox                  425        399   -26    +6.1%
> Candy_Crush_Soda         552        538   -14    +2.5%
> Hill_Climb_Racing        245        230   -15    +6.1%
> Call_Recorder            437        426   -11    +2.5%
> Color_Fill_3D            190        180   -10    +5.3%
> eToro                    512        505    -7    +1.4%
> GroupMe                  281        266   -15    +5.3%
>

Hey Bongkyu,
  I wanted to reach out to see what the current status of this patch
set? I'm seeing other parties trying to work around the loss of the
optimistic spinning functionality since commit 617f3ef95177
("locking/rwsem: Remove reader optimistic spinning") as well, with
their own custom variants (providing some substantial gains), and
would really like to have a common solution.

thanks
-john

Re: [PATCH v2 0/2] Make reader optimistic spinning optional

[Bongkyu Kim]

[-- Attachment #1: Type: text/plain, Size: 2837 bytes --]

On Tue, Apr 02, 2024 at 04:46:06PM -0700, John Stultz wrote:
> On Thu, Aug 31, 2023 at 6:07 PM Bongkyu Kim <bongkyu7.kim@samsung.com> wrote:
> >
> > This is rework of the following discussed patch.
> > https://lore.kernel.org/all/20230613043308.GA1027@KORCO045595.samsungds.net/
> >
> > Changes from the previous patch
> > - Split to revert and modify patches
> > - Change according to Waiman Long's review
> >     More wording to documentation part
> >     Change module_param to early_param
> >     Code change by Waiman Long's suggestion
> >
> > In mobile environment, reader optimistic spinning is still useful
> > because there're not many readers. In my test result at android device,
> > it improves application startup time about 3.8%
> > App startup time is most important factor for android user expriences.
> > So, re-enable reader optimistic spinning by this commit. And,
> > make it optional feature by cmdline.
> >
> > Test result:
> > This is 15 application startup performance in our exynos soc.
> > - Cortex A78*2 + Cortex A55*6
> > - unit: ms (lower is better)
> >
> > Application             base  opt_rspin  Diff  Diff(%)
> > --------------------  ------  ---------  ----  -------
> > * Total(geomean)         343        330   -13    +3.8%
> > --------------------  ------  ---------  ----  -------
> > helloworld               110        108    -2    +1.8%
> > Amazon_Seller            397        388    -9    +2.3%
> > Whatsapp                 311        304    -7    +2.3%
> > Simple_PDF_Reader        500        463   -37    +7.4%
> > FaceApp                  330        317   -13    +3.9%
> > Timestamp_Camera_Free    451        443    -8    +1.8%
> > Kindle                   629        597   -32    +5.1%
> > Coinbase                 243        233   -10    +4.1%
> > Firefox                  425        399   -26    +6.1%
> > Candy_Crush_Soda         552        538   -14    +2.5%
> > Hill_Climb_Racing        245        230   -15    +6.1%
> > Call_Recorder            437        426   -11    +2.5%
> > Color_Fill_3D            190        180   -10    +5.3%
> > eToro                    512        505    -7    +1.4%
> > GroupMe                  281        266   -15    +5.3%
> >
> 
> Hey Bongkyu,
>   I wanted to reach out to see what the current status of this patch
> set? I'm seeing other parties trying to work around the loss of the
> optimistic spinning functionality since commit 617f3ef95177
> ("locking/rwsem: Remove reader optimistic spinning") as well, with
> their own custom variants (providing some substantial gains), and
> would really like to have a common solution.
> 
> thanks
> -john
> 

Hi John,

I didn't get an reply, so I've been waiting.
Could you let me know about their patch?
I will try to comparing the app startup performance with reader optimistic spinning.

Thanks,
Bongkyu

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 0/2] Make reader optimistic spinning optional

[John Stultz]

On Tue, Apr 2, 2024 at 6:21 PM Bongkyu Kim <bongkyu7.kim@samsung.com> wrote:
> On Tue, Apr 02, 2024 at 04:46:06PM -0700, John Stultz wrote:
> > On Thu, Aug 31, 2023 at 6:07 PM Bongkyu Kim <bongkyu7.kim@samsung.com> wrote:
> > >
> > > This is rework of the following discussed patch.
> > > https://lore.kernel.org/all/20230613043308.GA1027@KORCO045595.samsungds.net/
> > >
> > > Changes from the previous patch
> > > - Split to revert and modify patches
> > > - Change according to Waiman Long's review
> > >     More wording to documentation part
> > >     Change module_param to early_param
> > >     Code change by Waiman Long's suggestion
> > >
> > > In mobile environment, reader optimistic spinning is still useful
> > > because there're not many readers. In my test result at android device,
> > > it improves application startup time about 3.8%
> > > App startup time is most important factor for android user expriences.
> > > So, re-enable reader optimistic spinning by this commit. And,
> > > make it optional feature by cmdline.
> > >
> > > Test result:
> > > This is 15 application startup performance in our exynos soc.
> > > - Cortex A78*2 + Cortex A55*6
> > > - unit: ms (lower is better)
> > >
> > > Application             base  opt_rspin  Diff  Diff(%)
> > > --------------------  ------  ---------  ----  -------
> > > * Total(geomean)         343        330   -13    +3.8%
> > > --------------------  ------  ---------  ----  -------
> > > helloworld               110        108    -2    +1.8%
> > > Amazon_Seller            397        388    -9    +2.3%
> > > Whatsapp                 311        304    -7    +2.3%
> > > Simple_PDF_Reader        500        463   -37    +7.4%
> > > FaceApp                  330        317   -13    +3.9%
> > > Timestamp_Camera_Free    451        443    -8    +1.8%
> > > Kindle                   629        597   -32    +5.1%
> > > Coinbase                 243        233   -10    +4.1%
> > > Firefox                  425        399   -26    +6.1%
> > > Candy_Crush_Soda         552        538   -14    +2.5%
> > > Hill_Climb_Racing        245        230   -15    +6.1%
> > > Call_Recorder            437        426   -11    +2.5%
> > > Color_Fill_3D            190        180   -10    +5.3%
> > > eToro                    512        505    -7    +1.4%
> > > GroupMe                  281        266   -15    +5.3%
> > >
> >
> > Hey Bongkyu,
> >   I wanted to reach out to see what the current status of this patch
> > set? I'm seeing other parties trying to work around the loss of the
> > optimistic spinning functionality since commit 617f3ef95177
> > ("locking/rwsem: Remove reader optimistic spinning") as well, with
> > their own custom variants (providing some substantial gains), and
> > would really like to have a common solution.
> >
>
> I didn't get an reply, so I've been waiting.
> Could you let me know about their patch?

I don't have insight/access to any other implementations, but I have
nudged folks to test your patch and chime in here.

Mostly I just wanted to share that others are also seeing performance
trouble from the loss of optimistic spinning, so it would be good to
get some sort of shared solution upstream.

thanks
-john

Re: [PATCH v2 0/2] Make reader optimistic spinning optional

[Bongkyu Kim]

[-- Attachment #1: Type: text/plain, Size: 3448 bytes --]

On Tue, Apr 02, 2024 at 06:27:40PM -0700, John Stultz wrote:
> On Tue, Apr 2, 2024 at 6:21 PM Bongkyu Kim <bongkyu7.kim@samsung.com> wrote:
> > On Tue, Apr 02, 2024 at 04:46:06PM -0700, John Stultz wrote:
> > > On Thu, Aug 31, 2023 at 6:07 PM Bongkyu Kim <bongkyu7.kim@samsung.com> wrote:
> > > >
> > > > This is rework of the following discussed patch.
> > > > https://lore.kernel.org/all/20230613043308.GA1027@KORCO045595.samsungds.net/
> > > >
> > > > Changes from the previous patch
> > > > - Split to revert and modify patches
> > > > - Change according to Waiman Long's review
> > > >     More wording to documentation part
> > > >     Change module_param to early_param
> > > >     Code change by Waiman Long's suggestion
> > > >
> > > > In mobile environment, reader optimistic spinning is still useful
> > > > because there're not many readers. In my test result at android device,
> > > > it improves application startup time about 3.8%
> > > > App startup time is most important factor for android user expriences.
> > > > So, re-enable reader optimistic spinning by this commit. And,
> > > > make it optional feature by cmdline.
> > > >
> > > > Test result:
> > > > This is 15 application startup performance in our exynos soc.
> > > > - Cortex A78*2 + Cortex A55*6
> > > > - unit: ms (lower is better)
> > > >
> > > > Application             base  opt_rspin  Diff  Diff(%)
> > > > --------------------  ------  ---------  ----  -------
> > > > * Total(geomean)         343        330   -13    +3.8%
> > > > --------------------  ------  ---------  ----  -------
> > > > helloworld               110        108    -2    +1.8%
> > > > Amazon_Seller            397        388    -9    +2.3%
> > > > Whatsapp                 311        304    -7    +2.3%
> > > > Simple_PDF_Reader        500        463   -37    +7.4%
> > > > FaceApp                  330        317   -13    +3.9%
> > > > Timestamp_Camera_Free    451        443    -8    +1.8%
> > > > Kindle                   629        597   -32    +5.1%
> > > > Coinbase                 243        233   -10    +4.1%
> > > > Firefox                  425        399   -26    +6.1%
> > > > Candy_Crush_Soda         552        538   -14    +2.5%
> > > > Hill_Climb_Racing        245        230   -15    +6.1%
> > > > Call_Recorder            437        426   -11    +2.5%
> > > > Color_Fill_3D            190        180   -10    +5.3%
> > > > eToro                    512        505    -7    +1.4%
> > > > GroupMe                  281        266   -15    +5.3%
> > > >
> > >
> > > Hey Bongkyu,
> > >   I wanted to reach out to see what the current status of this patch
> > > set? I'm seeing other parties trying to work around the loss of the
> > > optimistic spinning functionality since commit 617f3ef95177
> > > ("locking/rwsem: Remove reader optimistic spinning") as well, with
> > > their own custom variants (providing some substantial gains), and
> > > would really like to have a common solution.
> > >
> >
> > I didn't get an reply, so I've been waiting.
> > Could you let me know about their patch?
> 
> I don't have insight/access to any other implementations, but I have
> nudged folks to test your patch and chime in here.
> 
> Mostly I just wanted to share that others are also seeing performance
> trouble from the loss of optimistic spinning, so it would be good to
> get some sort of shared solution upstream.
> 
> thanks
> -john
> 

Thanks for your sharing!
Bongkyu.

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 0/2] Make reader optimistic spinning optional

[Waiman Long]

On 4/2/24 21:42, Bongkyu Kim wrote:
> On Tue, Apr 02, 2024 at 06:27:40PM -0700, John Stultz wrote:
>> On Tue, Apr 2, 2024 at 6:21 PM Bongkyu Kim <bongkyu7.kim@samsung.com> wrote:
>>> On Tue, Apr 02, 2024 at 04:46:06PM -0700, John Stultz wrote:
>>>> On Thu, Aug 31, 2023 at 6:07 PM Bongkyu Kim <bongkyu7.kim@samsung.com> wrote:
>>>>> This is rework of the following discussed patch.
>>>>> https://lore.kernel.org/all/20230613043308.GA1027@KORCO045595.samsungds.net/
>>>>>
>>>>> Changes from the previous patch
>>>>> - Split to revert and modify patches
>>>>> - Change according to Waiman Long's review
>>>>>      More wording to documentation part
>>>>>      Change module_param to early_param
>>>>>      Code change by Waiman Long's suggestion
>>>>>
>>>>> In mobile environment, reader optimistic spinning is still useful
>>>>> because there're not many readers. In my test result at android device,
>>>>> it improves application startup time about 3.8%
>>>>> App startup time is most important factor for android user expriences.
>>>>> So, re-enable reader optimistic spinning by this commit. And,
>>>>> make it optional feature by cmdline.
>>>>>
>>>>> Test result:
>>>>> This is 15 application startup performance in our exynos soc.
>>>>> - Cortex A78*2 + Cortex A55*6
>>>>> - unit: ms (lower is better)
>>>>>
>>>>> Application             base  opt_rspin  Diff  Diff(%)
>>>>> --------------------  ------  ---------  ----  -------
>>>>> * Total(geomean)         343        330   -13    +3.8%
>>>>> --------------------  ------  ---------  ----  -------
>>>>> helloworld               110        108    -2    +1.8%
>>>>> Amazon_Seller            397        388    -9    +2.3%
>>>>> Whatsapp                 311        304    -7    +2.3%
>>>>> Simple_PDF_Reader        500        463   -37    +7.4%
>>>>> FaceApp                  330        317   -13    +3.9%
>>>>> Timestamp_Camera_Free    451        443    -8    +1.8%
>>>>> Kindle                   629        597   -32    +5.1%
>>>>> Coinbase                 243        233   -10    +4.1%
>>>>> Firefox                  425        399   -26    +6.1%
>>>>> Candy_Crush_Soda         552        538   -14    +2.5%
>>>>> Hill_Climb_Racing        245        230   -15    +6.1%
>>>>> Call_Recorder            437        426   -11    +2.5%
>>>>> Color_Fill_3D            190        180   -10    +5.3%
>>>>> eToro                    512        505    -7    +1.4%
>>>>> GroupMe                  281        266   -15    +5.3%
>>>>>
>>>> Hey Bongkyu,
>>>>    I wanted to reach out to see what the current status of this patch
>>>> set? I'm seeing other parties trying to work around the loss of the
>>>> optimistic spinning functionality since commit 617f3ef95177
>>>> ("locking/rwsem: Remove reader optimistic spinning") as well, with
>>>> their own custom variants (providing some substantial gains), and
>>>> would really like to have a common solution.
>>>>
>>> I didn't get an reply, so I've been waiting.
>>> Could you let me know about their patch?
>> I don't have insight/access to any other implementations, but I have
>> nudged folks to test your patch and chime in here.
>>
>> Mostly I just wanted to share that others are also seeing performance
>> trouble from the loss of optimistic spinning, so it would be good to
>> get some sort of shared solution upstream.
>>
>> thanks
>> -john
>>
When this patch series was originally posted last year, we gave some 
comments and suggestion on how to improve it as well as request for more 
information on certain area. We were expecting a v2 with the suggested 
changes, but we never got one and so it just fell off the cliff.

Please send a v2 with the requested change and we can continue our 
discussion.

Thanks,
Longman

Re: [PATCH v2 0/2] Make reader optimistic spinning optional

[Waiman Long]

On 4/4/24 13:44, Waiman Long wrote:
>
> On 4/2/24 21:42, Bongkyu Kim wrote:
>> On Tue, Apr 02, 2024 at 06:27:40PM -0700, John Stultz wrote:
>>> On Tue, Apr 2, 2024 at 6:21 PM Bongkyu Kim 
>>> <bongkyu7.kim@samsung.com> wrote:
>>>> On Tue, Apr 02, 2024 at 04:46:06PM -0700, John Stultz wrote:
>>>>> On Thu, Aug 31, 2023 at 6:07 PM Bongkyu Kim 
>>>>> <bongkyu7.kim@samsung.com> wrote:
>>>>>> This is rework of the following discussed patch.
>>>>>> https://lore.kernel.org/all/20230613043308.GA1027@KORCO045595.samsungds.net/ 
>>>>>>
>>>>>>
>>>>>> Changes from the previous patch
>>>>>> - Split to revert and modify patches
>>>>>> - Change according to Waiman Long's review
>>>>>>      More wording to documentation part
>>>>>>      Change module_param to early_param
>>>>>>      Code change by Waiman Long's suggestion
>>>>>>
>>>>>> In mobile environment, reader optimistic spinning is still useful
>>>>>> because there're not many readers. In my test result at android 
>>>>>> device,
>>>>>> it improves application startup time about 3.8%
>>>>>> App startup time is most important factor for android user 
>>>>>> expriences.
>>>>>> So, re-enable reader optimistic spinning by this commit. And,
>>>>>> make it optional feature by cmdline.
>>>>>>
>>>>>> Test result:
>>>>>> This is 15 application startup performance in our exynos soc.
>>>>>> - Cortex A78*2 + Cortex A55*6
>>>>>> - unit: ms (lower is better)
>>>>>>
>>>>>> Application             base  opt_rspin  Diff  Diff(%)
>>>>>> --------------------  ------  ---------  ----  -------
>>>>>> * Total(geomean)         343        330   -13    +3.8%
>>>>>> --------------------  ------  ---------  ----  -------
>>>>>> helloworld               110        108    -2    +1.8%
>>>>>> Amazon_Seller            397        388    -9    +2.3%
>>>>>> Whatsapp                 311        304    -7    +2.3%
>>>>>> Simple_PDF_Reader        500        463   -37    +7.4%
>>>>>> FaceApp                  330        317   -13    +3.9%
>>>>>> Timestamp_Camera_Free    451        443    -8    +1.8%
>>>>>> Kindle                   629        597   -32    +5.1%
>>>>>> Coinbase                 243        233   -10    +4.1%
>>>>>> Firefox                  425        399   -26    +6.1%
>>>>>> Candy_Crush_Soda         552        538   -14    +2.5%
>>>>>> Hill_Climb_Racing        245        230   -15    +6.1%
>>>>>> Call_Recorder            437        426   -11    +2.5%
>>>>>> Color_Fill_3D            190        180   -10    +5.3%
>>>>>> eToro                    512        505    -7    +1.4%
>>>>>> GroupMe                  281        266   -15    +5.3%
>>>>>>
>>>>> Hey Bongkyu,
>>>>>    I wanted to reach out to see what the current status of this patch
>>>>> set? I'm seeing other parties trying to work around the loss of the
>>>>> optimistic spinning functionality since commit 617f3ef95177
>>>>> ("locking/rwsem: Remove reader optimistic spinning") as well, with
>>>>> their own custom variants (providing some substantial gains), and
>>>>> would really like to have a common solution.
>>>>>
>>>> I didn't get an reply, so I've been waiting.
>>>> Could you let me know about their patch?
>>> I don't have insight/access to any other implementations, but I have
>>> nudged folks to test your patch and chime in here.
>>>
>>> Mostly I just wanted to share that others are also seeing performance
>>> trouble from the loss of optimistic spinning, so it would be good to
>>> get some sort of shared solution upstream.
>>>
>>> thanks
>>> -john
>>>
> When this patch series was originally posted last year, we gave some 
> comments and suggestion on how to improve it as well as request for 
> more information on certain area. We were expecting a v2 with the 
> suggested changes, but we never got one and so it just fell off the 
> cliff.
>
> Please send a v2 with the requested change and we can continue our 
> discussion.

The major reason that reader optimistic spinning was taken out is 
because of reader fragmentation especially now that we essentially wake 
up all the readers all at once when it is reader's turn to take the read 
lock. I do admit I am a bit biased toward systems with large number of 
CPU cores. On smaller systems with just a few CPU cores, reader 
optimistic spinning may help performance. So one idea that I have is 
that one of the command line option values is an auto mode (beside on 
and off) that reader optimistic spinning is enabled for, say, <= 8 CPUs, 
but disabled with more CPUs.

Anyway, this is just one of my ideas.

Cheers,
Longman

Re: [PATCH v2 0/2] Make reader optimistic spinning optional

[Bongkyu Kim]

On 4/5/24 12:06, Waiman Long wrote:
> On 4/4/24 13:44, Waiman Long wrote:
>>
>> On 4/2/24 21:42, Bongkyu Kim wrote:
>>> On Tue, Apr 02, 2024 at 06:27:40PM -0700, John Stultz wrote:
>>>> On Tue, Apr 2, 2024 at 6:21 PM Bongkyu Kim
>>>> <bongkyu7.kim@samsung.com> wrote:
>>>>> On Tue, Apr 02, 2024 at 04:46:06PM -0700, John Stultz wrote:
>>>>>> On Thu, Aug 31, 2023 at 6:07 PM Bongkyu Kim
>>>>>> <bongkyu7.kim@samsung.com> wrote:
>>>>>>> This is rework of the following discussed patch.
>>>>>>> https://lore.kernel.org/all/20230613043308.GA1027@KORCO045595.samsungds.net/
>>>>>>>
>>>>>>> Changes from the previous patch
>>>>>>> - Split to revert and modify patches
>>>>>>> - Change according to Waiman Long's review
>>>>>>>      More wording to documentation part
>>>>>>>      Change module_param to early_param
>>>>>>>      Code change by Waiman Long's suggestion
>>>>>>>
>>>>>>> In mobile environment, reader optimistic spinning is still useful
>>>>>>> because there're not many readers. In my test result at android
>>>>>>> device,
>>>>>>> it improves application startup time about 3.8%
>>>>>>> App startup time is most important factor for android user
>>>>>>> expriences.
>>>>>>> So, re-enable reader optimistic spinning by this commit. And,
>>>>>>> make it optional feature by cmdline.
>>>>>>>
>>>>>>> Test result:
>>>>>>> This is 15 application startup performance in our exynos soc.
>>>>>>> - Cortex A78*2 + Cortex A55*6
>>>>>>> - unit: ms (lower is better)
>>>>>>>
>>>>>>> Application             base  opt_rspin  Diff  Diff(%)
>>>>>>> --------------------  ------  ---------  ----  -------
>>>>>>> * Total(geomean)         343        330   -13    +3.8%
>>>>>>> --------------------  ------  ---------  ----  -------
>>>>>>> helloworld               110        108    -2    +1.8%
>>>>>>> Amazon_Seller            397        388    -9    +2.3%
>>>>>>> Whatsapp                 311        304    -7    +2.3%
>>>>>>> Simple_PDF_Reader        500        463   -37    +7.4%
>>>>>>> FaceApp                  330        317   -13    +3.9%
>>>>>>> Timestamp_Camera_Free    451        443    -8    +1.8%
>>>>>>> Kindle                   629        597   -32    +5.1%
>>>>>>> Coinbase                 243        233   -10    +4.1%
>>>>>>> Firefox                  425        399   -26    +6.1%
>>>>>>> Candy_Crush_Soda         552        538   -14    +2.5%
>>>>>>> Hill_Climb_Racing        245        230   -15    +6.1%
>>>>>>> Call_Recorder            437        426   -11    +2.5%
>>>>>>> Color_Fill_3D            190        180   -10    +5.3%
>>>>>>> eToro                    512        505    -7    +1.4%
>>>>>>> GroupMe                  281        266   -15    +5.3%
>>>>>>>
>>>>>> Hey Bongkyu,
>>>>>>    I wanted to reach out to see what the current status of this patch
>>>>>> set? I'm seeing other parties trying to work around the loss of the
>>>>>> optimistic spinning functionality since commit 617f3ef95177
>>>>>> ("locking/rwsem: Remove reader optimistic spinning") as well, with
>>>>>> their own custom variants (providing some substantial gains), and
>>>>>> would really like to have a common solution.
>>>>>>
>>>>> I didn't get an reply, so I've been waiting.
>>>>> Could you let me know about their patch?
>>>> I don't have insight/access to any other implementations, but I have
>>>> nudged folks to test your patch and chime in here.
>>>>
>>>> Mostly I just wanted to share that others are also seeing performance
>>>> trouble from the loss of optimistic spinning, so it would be good to
>>>> get some sort of shared solution upstream.
>>>>
>>>> thanks
>>>> -john
>>>>
>> When this patch series was originally posted last year, we gave some
>> comments and suggestion on how to improve it as well as request for
>> more information on certain area. We were expecting a v2 with the
>> suggested changes, but we never got one and so it just fell off the
>> cliff.
>>
>> Please send a v2 with the requested change and we can continue our
>> discussion.
> 
> The major reason that reader optimistic spinning was taken out is
> because of reader fragmentation especially now that we essentially wake
> up all the readers all at once when it is reader's turn to take the read
> lock. I do admit I am a bit biased toward systems with large number of
> CPU cores. On smaller systems with just a few CPU cores, reader
> optimistic spinning may help performance. So one idea that I have is
> that one of the command line option values is an auto mode (beside on
> and off) that reader optimistic spinning is enabled for, say, <= 8 CPUs,
> but disabled with more CPUs.
> 
> Anyway, this is just one of my ideas.
> 
> Cheers,
> Longman
> 
> 

Hi Longman,

Including your idea, I will reconsider and resend patch.

Thanks,
Bongkyu

Re: [PATCH v2 0/2] Make reader optimistic spinning optional

[Bongkyu Kim]

[-- Attachment #1: Type: text/plain, Size: 5588 bytes --]

On Thu, Apr 04, 2024 at 11:06:12PM -0400, Waiman Long wrote:
> On 4/4/24 13:44, Waiman Long wrote:
> > 
> > On 4/2/24 21:42, Bongkyu Kim wrote:
> > > On Tue, Apr 02, 2024 at 06:27:40PM -0700, John Stultz wrote:
> > > > On Tue, Apr 2, 2024 at 6:21 PM Bongkyu Kim
> > > > <bongkyu7.kim@samsung.com> wrote:
> > > > > On Tue, Apr 02, 2024 at 04:46:06PM -0700, John Stultz wrote:
> > > > > > On Thu, Aug 31, 2023 at 6:07 PM Bongkyu Kim
> > > > > > <bongkyu7.kim@samsung.com> wrote:
> > > > > > > This is rework of the following discussed patch.
> > > > > > > https://lore.kernel.org/all/20230613043308.GA1027@KORCO045595.samsungds.net/
> > > > > > > 
> > > > > > > 
> > > > > > > Changes from the previous patch
> > > > > > > - Split to revert and modify patches
> > > > > > > - Change according to Waiman Long's review
> > > > > > >      More wording to documentation part
> > > > > > >      Change module_param to early_param
> > > > > > >      Code change by Waiman Long's suggestion
> > > > > > > 
> > > > > > > In mobile environment, reader optimistic spinning is still useful
> > > > > > > because there're not many readers. In my test result
> > > > > > > at android device,
> > > > > > > it improves application startup time about 3.8%
> > > > > > > App startup time is most important factor for
> > > > > > > android user expriences.
> > > > > > > So, re-enable reader optimistic spinning by this commit. And,
> > > > > > > make it optional feature by cmdline.
> > > > > > > 
> > > > > > > Test result:
> > > > > > > This is 15 application startup performance in our exynos soc.
> > > > > > > - Cortex A78*2 + Cortex A55*6
> > > > > > > - unit: ms (lower is better)
> > > > > > > 
> > > > > > > Application             base  opt_rspin  Diff  Diff(%)
> > > > > > > --------------------  ------  ---------  ----  -------
> > > > > > > * Total(geomean)         343        330   -13    +3.8%
> > > > > > > --------------------  ------  ---------  ----  -------
> > > > > > > helloworld               110        108    -2    +1.8%
> > > > > > > Amazon_Seller            397        388    -9    +2.3%
> > > > > > > Whatsapp                 311        304    -7    +2.3%
> > > > > > > Simple_PDF_Reader        500        463   -37    +7.4%
> > > > > > > FaceApp                  330        317   -13    +3.9%
> > > > > > > Timestamp_Camera_Free    451        443    -8    +1.8%
> > > > > > > Kindle                   629        597   -32    +5.1%
> > > > > > > Coinbase                 243        233   -10    +4.1%
> > > > > > > Firefox                  425        399   -26    +6.1%
> > > > > > > Candy_Crush_Soda         552        538   -14    +2.5%
> > > > > > > Hill_Climb_Racing        245        230   -15    +6.1%
> > > > > > > Call_Recorder            437        426   -11    +2.5%
> > > > > > > Color_Fill_3D            190        180   -10    +5.3%
> > > > > > > eToro                    512        505    -7    +1.4%
> > > > > > > GroupMe                  281        266   -15    +5.3%
> > > > > > > 
> > > > > > Hey Bongkyu,
> > > > > >    I wanted to reach out to see what the current status of this patch
> > > > > > set? I'm seeing other parties trying to work around the loss of the
> > > > > > optimistic spinning functionality since commit 617f3ef95177
> > > > > > ("locking/rwsem: Remove reader optimistic spinning") as well, with
> > > > > > their own custom variants (providing some substantial gains), and
> > > > > > would really like to have a common solution.
> > > > > > 
> > > > > I didn't get an reply, so I've been waiting.
> > > > > Could you let me know about their patch?
> > > > I don't have insight/access to any other implementations, but I have
> > > > nudged folks to test your patch and chime in here.
> > > > 
> > > > Mostly I just wanted to share that others are also seeing performance
> > > > trouble from the loss of optimistic spinning, so it would be good to
> > > > get some sort of shared solution upstream.
> > > > 
> > > > thanks
> > > > -john
> > > > 
> > When this patch series was originally posted last year, we gave some
> > comments and suggestion on how to improve it as well as request for more
> > information on certain area. We were expecting a v2 with the suggested
> > changes, but we never got one and so it just fell off the cliff.
> > 
> > Please send a v2 with the requested change and we can continue our
> > discussion.
> 
> The major reason that reader optimistic spinning was taken out is because of
> reader fragmentation especially now that we essentially wake up all the
> readers all at once when it is reader's turn to take the read lock. I do
> admit I am a bit biased toward systems with large number of CPU cores. On
> smaller systems with just a few CPU cores, reader optimistic spinning may
> help performance. So one idea that I have is that one of the command line
> option values is an auto mode (beside on and off) that reader optimistic
> spinning is enabled for, say, <= 8 CPUs, but disabled with more CPUs.
> 
> Anyway, this is just one of my ideas.
> 
> Cheers,
> Longman
> 
Hi Longman,

Including your idea, I will reconsider and resend patch.

Thanks,
Bongkyu


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 0/2] Make reader optimistic spinning optional

[xieliujie]

On 2024/4/5 14:51, Bongkyu Kim wrote:
> On Thu, Apr 04, 2024 at 11:06:12PM -0400, Waiman Long wrote:
>> On 4/4/24 13:44, Waiman Long wrote:
>>> On 4/2/24 21:42, Bongkyu Kim wrote:
>>>> On Tue, Apr 02, 2024 at 06:27:40PM -0700, John Stultz wrote:
>>>>> On Tue, Apr 2, 2024 at 6:21 PM Bongkyu Kim
>>>>> <bongkyu7.kim@samsung.com> wrote:
>>>>>> On Tue, Apr 02, 2024 at 04:46:06PM -0700, John Stultz wrote:
>>>>>>> On Thu, Aug 31, 2023 at 6:07 PM Bongkyu Kim
>>>>>>> <bongkyu7.kim@samsung.com> wrote:
>>>>>>>> This is rework of the following discussed patch.
>>>>>>>> https://lore.kernel.org/all/20230613043308.GA1027@KORCO045595.samsungds.net/
>>>>>>>>
>>>>>>>>
>>>>>>>> Changes from the previous patch
>>>>>>>> - Split to revert and modify patches
>>>>>>>> - Change according to Waiman Long's review
>>>>>>>>       More wording to documentation part
>>>>>>>>       Change module_param to early_param
>>>>>>>>       Code change by Waiman Long's suggestion
>>>>>>>>
>>>>>>>> In mobile environment, reader optimistic spinning is still useful
>>>>>>>> because there're not many readers. In my test result
>>>>>>>> at android device,
>>>>>>>> it improves application startup time about 3.8%
>>>>>>>> App startup time is most important factor for
>>>>>>>> android user expriences.
>>>>>>>> So, re-enable reader optimistic spinning by this commit. And,
>>>>>>>> make it optional feature by cmdline.
>>>>>>>>
>>>>>>>> Test result:
>>>>>>>> This is 15 application startup performance in our exynos soc.
>>>>>>>> - Cortex A78*2 + Cortex A55*6
>>>>>>>> - unit: ms (lower is better)
>>>>>>>>
>>>>>>>> Application             base  opt_rspin  Diff  Diff(%)
>>>>>>>> --------------------  ------  ---------  ----  -------
>>>>>>>> * Total(geomean)         343        330   -13    +3.8%
>>>>>>>> --------------------  ------  ---------  ----  -------
>>>>>>>> helloworld               110        108    -2    +1.8%
>>>>>>>> Amazon_Seller            397        388    -9    +2.3%
>>>>>>>> Whatsapp                 311        304    -7    +2.3%
>>>>>>>> Simple_PDF_Reader        500        463   -37    +7.4%
>>>>>>>> FaceApp                  330        317   -13    +3.9%
>>>>>>>> Timestamp_Camera_Free    451        443    -8    +1.8%
>>>>>>>> Kindle                   629        597   -32    +5.1%
>>>>>>>> Coinbase                 243        233   -10    +4.1%
>>>>>>>> Firefox                  425        399   -26    +6.1%
>>>>>>>> Candy_Crush_Soda         552        538   -14    +2.5%
>>>>>>>> Hill_Climb_Racing        245        230   -15    +6.1%
>>>>>>>> Call_Recorder            437        426   -11    +2.5%
>>>>>>>> Color_Fill_3D            190        180   -10    +5.3%
>>>>>>>> eToro                    512        505    -7    +1.4%
>>>>>>>> GroupMe                  281        266   -15    +5.3%
>>>>>>>>
>>>>>>> Hey Bongkyu,
>>>>>>>     I wanted to reach out to see what the current status of this patch
>>>>>>> set? I'm seeing other parties trying to work around the loss of the
>>>>>>> optimistic spinning functionality since commit 617f3ef95177
>>>>>>> ("locking/rwsem: Remove reader optimistic spinning") as well, with
>>>>>>> their own custom variants (providing some substantial gains), and
>>>>>>> would really like to have a common solution.
>>>>>>>
>>>>>> I didn't get an reply, so I've been waiting.
>>>>>> Could you let me know about their patch?
>>>>> I don't have insight/access to any other implementations, but I have
>>>>> nudged folks to test your patch and chime in here.
>>>>>
>>>>> Mostly I just wanted to share that others are also seeing performance
>>>>> trouble from the loss of optimistic spinning, so it would be good to
>>>>> get some sort of shared solution upstream.
>>>>>
>>>>> thanks
>>>>> -john
>>>>>
>>> When this patch series was originally posted last year, we gave some
>>> comments and suggestion on how to improve it as well as request for more
>>> information on certain area. We were expecting a v2 with the suggested
>>> changes, but we never got one and so it just fell off the cliff.
>>>
>>> Please send a v2 with the requested change and we can continue our
>>> discussion.
>> The major reason that reader optimistic spinning was taken out is because of
>> reader fragmentation especially now that we essentially wake up all the
>> readers all at once when it is reader's turn to take the read lock. I do
>> admit I am a bit biased toward systems with large number of CPU cores. On
>> smaller systems with just a few CPU cores, reader optimistic spinning may
>> help performance. So one idea that I have is that one of the command line
>> option values is an auto mode (beside on and off) that reader optimistic
>> spinning is enabled for, say, <= 8 CPUs, but disabled with more CPUs.
>>
>> Anyway, this is just one of my ideas.
>>
>> Cheers,
>> Longman
>>
> Hi Longman,
>
> Including your idea, I will reconsider and resend patch.
>
> Thanks,
> Bongkyu
Hi Bongkyu,

I have met the same problem as you. After rwsem reader optimistic spin
disabled, DouYin(or TikTok as you know, is one of the top applicaitons),
it's uninterruptible sleep (non-io) time increased during cold launch.
With the patch set you provide, the blocked time can be reduce from
122.6ms to 59.3ms(we test it in our mobile device with SM8650 core). Now
I'm trying to use vendor hook in GKI to restore the reader spin, and
john said I can share my test result to you here.

Thanks,
Liujie
>
>
________________________________
OPPO

本电子邮件及其附件含有OPPO公司的保密信息，仅限于邮件指明的收件人（包含个人及群组）使用。禁止任何人在未经授权的情况下以任何形式使用。如果您错收了本邮件，切勿传播、分发、复制、印刷或使用本邮件之任何部分或其所载之任何内容，并请立即以电子邮件通知发件人并删除本邮件及其附件。
网络通讯固有缺陷可能导致邮件被截留、修改、丢失、破坏或包含计算机病毒等不安全情况，OPPO对此类错误或遗漏而引致之任何损失概不承担责任并保留与本邮件相关之一切权利。
除非明确说明，本邮件及其附件无意作为在任何国家或地区之要约、招揽或承诺，亦无意作为任何交易或合同之正式确认。 发件人、其所属机构或所属机构之关联机构或任何上述机构之股东、董事、高级管理人员、员工或其他任何人（以下称“发件人”或“OPPO”）不因本邮件之误送而放弃其所享之任何权利，亦不对因故意或过失使用该等信息而引发或可能引发的损失承担任何责任。
文化差异披露：因全球文化差异影响，单纯以YES\OK或其他简单词汇的回复并不构成发件人对任何交易或合同之正式确认或接受，请与发件人再次确认以获得明确书面意见。发件人不对任何受文化差异影响而导致故意或错误使用该等信息所造成的任何直接或间接损害承担责任。
This e-mail and its attachments contain confidential information from OPPO, which is intended only for the person or entity whose address is listed above. Any use of the information contained herein in any way (including, but not limited to, total or partial disclosure, reproduction, or dissemination) by persons other than the intended recipient(s) is prohibited. If you are not the intended recipient, please do not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.
Electronic communications may contain computer viruses or other defects inherently, may not be accurately and/or timely transmitted to other systems, or may be intercepted, modified ,delayed, deleted or interfered. OPPO shall not be liable for any damages that arise or may arise from such matter and reserves all rights in connection with the email.
Unless expressly stated, this e-mail and its attachments are provided without any warranty, acceptance or promise of any kind in any country or region, nor constitute a formal confirmation or acceptance of any transaction or contract. The sender, together with its affiliates or any shareholder, director, officer, employee or any other person of any such institution (hereinafter referred to as "sender" or "OPPO") does not waive any rights and shall not be liable for any damages that arise or may arise from the intentional or negligent use of such information.
Cultural Differences Disclosure: Due to global cultural differences, any reply with only YES\OK or other simple words does not constitute any confirmation or acceptance of any transaction or contract, please confirm with the sender again to ensure clear opinion in written form. The sender shall not be responsible for any direct or indirect damages resulting from the intentional or misuse of such information.

Re: [PATCH v2 0/2] Make reader optimistic spinning optional

[Bongkyu Kim]

[-- Attachment #1: Type: text/plain, Size: 9866 bytes --]

On Mon, Apr 08, 2024 at 04:15:04PM +0800, xieliujie wrote:
> 
> On 2024/4/5 14:51, Bongkyu Kim wrote:
> > On Thu, Apr 04, 2024 at 11:06:12PM -0400, Waiman Long wrote:
> > > On 4/4/24 13:44, Waiman Long wrote:
> > > > On 4/2/24 21:42, Bongkyu Kim wrote:
> > > > > On Tue, Apr 02, 2024 at 06:27:40PM -0700, John Stultz wrote:
> > > > > > On Tue, Apr 2, 2024 at 6:21 PM Bongkyu Kim
> > > > > > <bongkyu7.kim@samsung.com> wrote:
> > > > > > > On Tue, Apr 02, 2024 at 04:46:06PM -0700, John Stultz wrote:
> > > > > > > > On Thu, Aug 31, 2023 at 6:07 PM Bongkyu Kim
> > > > > > > > <bongkyu7.kim@samsung.com> wrote:
> > > > > > > > > This is rework of the following discussed patch.
> > > > > > > > > https://lore.kernel.org/all/20230613043308.GA1027@KORCO045595.samsungds.net/
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Changes from the previous patch
> > > > > > > > > - Split to revert and modify patches
> > > > > > > > > - Change according to Waiman Long's review
> > > > > > > > >       More wording to documentation part
> > > > > > > > >       Change module_param to early_param
> > > > > > > > >       Code change by Waiman Long's suggestion
> > > > > > > > > 
> > > > > > > > > In mobile environment, reader optimistic spinning is still useful
> > > > > > > > > because there're not many readers. In my test result
> > > > > > > > > at android device,
> > > > > > > > > it improves application startup time about 3.8%
> > > > > > > > > App startup time is most important factor for
> > > > > > > > > android user expriences.
> > > > > > > > > So, re-enable reader optimistic spinning by this commit. And,
> > > > > > > > > make it optional feature by cmdline.
> > > > > > > > > 
> > > > > > > > > Test result:
> > > > > > > > > This is 15 application startup performance in our exynos soc.
> > > > > > > > > - Cortex A78*2 + Cortex A55*6
> > > > > > > > > - unit: ms (lower is better)
> > > > > > > > > 
> > > > > > > > > Application             base  opt_rspin  Diff  Diff(%)
> > > > > > > > > --------------------  ------  ---------  ----  -------
> > > > > > > > > * Total(geomean)         343        330   -13    +3.8%
> > > > > > > > > --------------------  ------  ---------  ----  -------
> > > > > > > > > helloworld               110        108    -2    +1.8%
> > > > > > > > > Amazon_Seller            397        388    -9    +2.3%
> > > > > > > > > Whatsapp                 311        304    -7    +2.3%
> > > > > > > > > Simple_PDF_Reader        500        463   -37    +7.4%
> > > > > > > > > FaceApp                  330        317   -13    +3.9%
> > > > > > > > > Timestamp_Camera_Free    451        443    -8    +1.8%
> > > > > > > > > Kindle                   629        597   -32    +5.1%
> > > > > > > > > Coinbase                 243        233   -10    +4.1%
> > > > > > > > > Firefox                  425        399   -26    +6.1%
> > > > > > > > > Candy_Crush_Soda         552        538   -14    +2.5%
> > > > > > > > > Hill_Climb_Racing        245        230   -15    +6.1%
> > > > > > > > > Call_Recorder            437        426   -11    +2.5%
> > > > > > > > > Color_Fill_3D            190        180   -10    +5.3%
> > > > > > > > > eToro                    512        505    -7    +1.4%
> > > > > > > > > GroupMe                  281        266   -15    +5.3%
> > > > > > > > > 
> > > > > > > > Hey Bongkyu,
> > > > > > > >     I wanted to reach out to see what the current status of this patch
> > > > > > > > set? I'm seeing other parties trying to work around the loss of the
> > > > > > > > optimistic spinning functionality since commit 617f3ef95177
> > > > > > > > ("locking/rwsem: Remove reader optimistic spinning") as well, with
> > > > > > > > their own custom variants (providing some substantial gains), and
> > > > > > > > would really like to have a common solution.
> > > > > > > > 
> > > > > > > I didn't get an reply, so I've been waiting.
> > > > > > > Could you let me know about their patch?
> > > > > > I don't have insight/access to any other implementations, but I have
> > > > > > nudged folks to test your patch and chime in here.
> > > > > > 
> > > > > > Mostly I just wanted to share that others are also seeing performance
> > > > > > trouble from the loss of optimistic spinning, so it would be good to
> > > > > > get some sort of shared solution upstream.
> > > > > > 
> > > > > > thanks
> > > > > > -john
> > > > > > 
> > > > When this patch series was originally posted last year, we gave some
> > > > comments and suggestion on how to improve it as well as request for more
> > > > information on certain area. We were expecting a v2 with the suggested
> > > > changes, but we never got one and so it just fell off the cliff.
> > > > 
> > > > Please send a v2 with the requested change and we can continue our
> > > > discussion.
> > > The major reason that reader optimistic spinning was taken out is because of
> > > reader fragmentation especially now that we essentially wake up all the
> > > readers all at once when it is reader's turn to take the read lock. I do
> > > admit I am a bit biased toward systems with large number of CPU cores. On
> > > smaller systems with just a few CPU cores, reader optimistic spinning may
> > > help performance. So one idea that I have is that one of the command line
> > > option values is an auto mode (beside on and off) that reader optimistic
> > > spinning is enabled for, say, <= 8 CPUs, but disabled with more CPUs.
> > > 
> > > Anyway, this is just one of my ideas.
> > > 
> > > Cheers,
> > > Longman
> > > 
> > Hi Longman,
> > 
> > Including your idea, I will reconsider and resend patch.
> > 
> > Thanks,
> > Bongkyu
> Hi Bongkyu,
> 
> I have met the same problem as you. After rwsem reader optimistic spin
> disabled, DouYin(or TikTok as you know, is one of the top applicaitons),
> it's uninterruptible sleep (non-io) time increased during cold launch.
> With the patch set you provide, the blocked time can be reduce from
> 122.6ms to 59.3ms(we test it in our mobile device with SM8650 core). Now
> I'm trying to use vendor hook in GKI to restore the reader spin, and
> john said I can share my test result to you here.
> 
> Thanks,
> Liujie

Hi Liujie,

Thanks for your sharing. I'm trying to send v3 patch like below.
https://lore.kernel.org/all/20240406081126.8030-1-bongkyu7.kim@samsung.com/

Bongkyu

> > 
> > 
> ________________________________
> OPPO
> 
> 本电子邮件及其附件含有OPPO公司的保密信息，仅限于邮件指明的收件人（包含个人及群组）使用。禁止任何人在未经授权的情况下以任何形式使用。如果您错收了本邮件，切勿传播、分发、复制、印刷或使用本邮件之任何部分或其所载之任何内容，并请立即以电子邮件通知发件人并删除本邮件及其附件。
> 网络通讯固有缺陷可能导致邮件被截留、修改、丢失、破坏或包含计算机病毒等不安全情况，OPPO对此类错误或遗漏而引致之任何损失概不承担责任并保留与本邮件相关之一切权利。
> 除非明确说明，本邮件及其附件无意作为在任何国家或地区之要约、招揽或承诺，亦无意作为任何交易或合同之正式确认。 发件人、其所属机构或所属机构之关联机构或任何上述机构之股东、董事、高级管理人员、员工或其他任何人（以下称“发件人”或“OPPO”）不因本邮件之误送而放弃其所享之任何权利，亦不对因故意或过失使用该等信息而引发或可能引发的损失承担任何责任。
> 文化差异披露：因全球文化差异影响，单纯以YES\OK或其他简单词汇的回复并不构成发件人对任何交易或合同之正式确认或接受，请与发件人再次确认以获得明确书面意见。发件人不对任何受文化差异影响而导致故意或错误使用该等信息所造成的任何直接或间接损害承担责任。
> This e-mail and its attachments contain confidential information from OPPO, which is intended only for the person or entity whose address is listed above. Any use of the information contained herein in any way (including, but not limited to, total or partial disclosure, reproduction, or dissemination) by persons other than the intended recipient(s) is prohibited. If you are not the intended recipient, please do not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.
> Electronic communications may contain computer viruses or other defects inherently, may not be accurately and/or timely transmitted to other systems, or may be intercepted, modified ,delayed, deleted or interfered. OPPO shall not be liable for any damages that arise or may arise from such matter and reserves all rights in connection with the email.
> Unless expressly stated, this e-mail and its attachments are provided without any warranty, acceptance or promise of any kind in any country or region, nor constitute a formal confirmation or acceptance of any transaction or contract. The sender, together with its affiliates or any shareholder, director, officer, employee or any other person of any such institution (hereinafter referred to as "sender" or "OPPO") does not waive any rights and shall not be liable for any damages that arise or may arise from the intentional or negligent use of such information.
> Cultural Differences Disclosure: Due to global cultural differences, any reply with only YES\OK or other simple words does not constitute any confirmation or acceptance of any transaction or contract, please confirm with the sender again to ensure clear opinion in written form. The sender shall not be responsible for any direct or indirect damages resulting from the intentional or misuse of such information.
> 

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]