On 06/15/2016 01:38 PM, Peter Zijlstra wrote:

On Tue, Jun 14, 2016 at 06:48:06PM -0400, Waiman Long wrote:

quoted

  static bool rwsem_optimistic_spin(struct rw_semaphore *sem)
  {
-	bool taken = false;
+	bool taken = false, can_spin;

I would place the variables without assignment first.

Sure, easy change.

quoted

+	int loopcnt;

  	preempt_disable();

@@ -409,6 +412,8 @@ static bool rwsem_optimistic_spin(struct rw_semaphore *sem)
  	if (!osq_lock(&sem->osq))
  		goto done;

+	loopcnt = sem->rspin_enabled ? RWSEM_RSPIN_THRESHOLD : 0;
+
  	/*
  	 * Optimistically spin on the owner field and attempt to acquire the
  	 * lock whenever the owner changes. Spinning will be stopped when:

@@ -416,7 +421,7 @@ static bool rwsem_optimistic_spin(struct rw_semaphore *sem)
  	 *  2) readers own the lock as we can't determine if they are
  	 *     actively running or not.
  	 */
-	while (rwsem_spin_on_owner(sem)) {
+	while ((can_spin = rwsem_spin_on_owner(sem)) || loopcnt) {
  		/*
  		 * Try to acquire the lock
  		 */

@@ -425,13 +430,16 @@ static bool rwsem_optimistic_spin(struct rw_semaphore *sem)
  			break;
  		}

+		if (!can_spin&&  loopcnt)
+			loopcnt--;

This seems to suggest 'can_spin' is a bad name, because if we cannot
spin, we do in fact spin anyway?

Maybe call it write_spin or something, which makes it clear that if its
not a write spin we'll do a read spin?

I am fine with the write_spin name.

Also, isn't this the wrong level to do loopcnt at?
rwsem_spin_on_owner() can have spend any amount of cycles spinning. So
you're not counting loops of similar unit.

The loopcnt does not include time spinning on writer. It will be 
decremented only if the lock is owned by reader (can_spin == false). I 
will clarify that with additional comments.

quoted

+	/*
+	 * Was owner a reader?
+	 */
+	if (rwsem_owner_is_reader(sem->owner)) {
+		/*
+		 * Update rspin_enabled for reader spinning

full stop and newline?

Sure.

quoted

+		 * Increment by 1 if successfully&  decrement by 8 if
+		 * unsuccessful.

This is bloody obvious from the code, explain why, not what the code
does.

Will clarify the comment.

quoted

                                The decrement amount is kind of arbitrary
+		 * and can be adjusted if necessary.
+		 */
+		if (taken&&  (sem->rspin_enabled<  RWSEM_RSPIN_ENABLED_MAX))
+			sem->rspin_enabled++;
+		else if (!taken)
+			sem->rspin_enabled = (sem->rspin_enabled>= 8)
+					   ? sem->rspin_enabled - 8 : 0;

This is unreadable and against coding style.

I will fix the coding style problem.

Cheers,
Longman