Re: [RFC PATCH-tip v2 3/6] locking/rwsem: Enable count-based spinning on reader

Waiman Long
2016-06-15T19:28:03Z

On 06/15/2016 01:38 PM, Peter Zijlstra wrote:
> 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.

Sure, easy change.

>
>> + 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.

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

Sure.

>
>> + * 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.

>> 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