On Tue, Jun 14, 2016 at 06:48:06PM -0400, Waiman Long wrote: > 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. > + 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? 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. > + /* > + * Was owner a reader? > + */ > + if (rwsem_owner_is_reader(sem->owner)) { > + /* > + * Update rspin_enabled for reader spinning full stop and newline? > + * Increment by 1 if successfully & decrement by 8 if > + * unsuccessful. This is bloody obvious from the code, explain why, not what the code does. > 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. > + } > osq_unlock(&sem->osq); > done: > preempt_enable(); -- To unsubscribe from this list: send the line "unsubscribe linux-arch" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html