Re: [RFC PATCH-tip v2 3/6] locking/rwsem: Enable count-based spinning on reader
From: Peter Zijlstra <peterz@infradead.org>
Date: 2016-06-15 17:38:47
Also in:
linux-alpha, linux-s390, lkml
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.
quoted hunk ↗ jump to hunk
+ 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 spinningfull 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();