Re: [PATCH 2/2] locking/qspinlock: Limit # of spins in _Q_PENDING_VAL wait loop

[Waiman Long <longman@redhat.com>]

On 04/10/2018 02:26 PM, Will Deacon wrote:
> Hi Waiman,
>
> On Mon, Apr 09, 2018 at 02:08:52PM -0400, Waiman Long wrote:
>> A locker in the pending code path is doing an infinite number of spins
>> when waiting for the _Q_PENDING_VAL to _Q_LOCK_VAL transition. There
>> is a concern that lock starvation can happen concurrent lockers are
>> able to take the lock in the cmpxchg loop without queuing and pass it
>> around amongst themselves.
>>
>> To ensure forward progress while still taking advantage of using
>> the pending code path without queuing, the code is now modified
>> to do a limited number of spins before aborting the effort and
>> going to queue itself.
>>
>> Ideally, the spinning times should be at least a few times the typical
>> cacheline load time from memory which I think can be down to 100ns or
>> so for each cacheline load with the newest systems or up to several
>> hundreds ns for older systems.
>>
>> Signed-off-by: Waiman Long <longman@redhat.com>
>> ---
>>  kernel/locking/qspinlock.c | 19 +++++++++++++++++--
>>  1 file changed, 17 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c
>> index 634a49b..35367cc 100644
>> --- a/kernel/locking/qspinlock.c
>> +++ b/kernel/locking/qspinlock.c
>> @@ -82,6 +82,15 @@
>>  #endif
>>  
>>  /*
>> + * The pending bit spinning loop count.
>> + * This parameter can be overridden by another architecture specific
>> + * constant. Default is 512.
>> + */
>> +#ifndef _Q_PENDING_LOOP
>> +#define _Q_PENDING_LOOP	(1 << 9)
>> +#endif
> I really dislike heuristics like this because there's never a good number
> to choose and it almost certainly varies between systems and workloads
> rather than just by architecture. However, I've also not managed to come
> up with something better.

I share your concern about heuristic like this, but I can't think of
another easy way out.

> If I rewrite your code slightly to look like:
>
> 	if (val == _Q_PENDING_VAL) {
> 		int cnt = _Q_PENDING_LOOP;
> 		val = atomic_cond_read_relaxed(&lock->val, (VAL != _Q_PENDING_VAL) || !cnt--);
> 	}
>
> then architectures that implement atomic_cond_read_relaxed as something
> more interesting than a spinning loop will probably be happy with
> _Q_PENDING_LOOP == 1;

Right. That is why I state that _Q_PENDING_LOOP is an architecture
specific constant.

Cheers,
Longman