Re: [PATCH v4 09/16] locking/rwsem: Ensure an RT task will not spin on reader

[Waiman Long]

On 04/17/2019 09:18 AM, Peter Zijlstra wrote:
> On Sat, Apr 13, 2019 at 01:22:52PM -0400, Waiman Long wrote:
>> An RT task can do optimistic spinning only if the lock holder is
>> actually running. If the state of the lock holder isn't known, there
>> is a possibility that high priority of the RT task may block forward
>> progress of the lock holder if it happens to reside on the same CPU.
>> This will lead to deadlock. So we have to make sure that an RT task
>> will not spin on a reader-owned rwsem.
>>
>> When the owner is temporarily set to NULL, it is more tricky to decide
>> if an RT task should stop spinning as it may be a temporary state
>> where another writer may have just stolen the lock which then failed
>> the task's trylock attempt. So one more retry is allowed to make sure
>> that the lock is not spinnable by an RT task.
>>
>> When testing on a 8-socket IvyBridge-EX system, the one additional retry
>> seems to improve locking performance of RT write locking threads under
>> heavy contentions. The table below shows the locking rates (in kops/s)
>> with various write locking threads before and after the patch.
>>
>>     Locking threads     Pre-patch     Post-patch
>>     ---------------     ---------     -----------
>>             4             2,753          2,608
>>             8             2,529          2,520
>>            16             1,727          1,918
>>            32             1,263          1,956
>>            64               889          1,343
>>
>> Signed-off-by: Waiman Long <longman@xxxxxxxxxx>
>> ---
>>  kernel/locking/rwsem.c | 36 +++++++++++++++++++++++++++++-------
>>  1 file changed, 29 insertions(+), 7 deletions(-)
>>
>> diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
>> index 2d6850c3e77b..8e19b5141595 100644
>> --- a/kernel/locking/rwsem.c
>> +++ b/kernel/locking/rwsem.c
>> @@ -539,6 +539,8 @@ static noinline enum owner_state rwsem_spin_on_owner(struct rw_semaphore *sem)
>>  static bool rwsem_optimistic_spin(struct rw_semaphore *sem)
>>  {
>>  	bool taken = false;
>> +	bool is_rt_task = rt_task(current);
> Arguably this is wrong; a remote CPU could change the scheduling
> atributes of this task while it is spinning. In practise I don't think
> we do that without forcing a reschedule, but in theory we could if we
> find the task is current anyway.

Will move the check back to the main loop.

>> +	int prev_owner_state = OWNER_NULL;
>>  
>>  	preempt_disable();
>>  
>> @@ -556,7 +558,12 @@ 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) & OWNER_SPINNABLE) {
>> +	for (;;) {
>> +		enum owner_state owner_state = rwsem_spin_on_owner(sem);
>> +
>> +		if (!(owner_state & OWNER_SPINNABLE))
>> +			break;
>> +
>>  		/*
>>  		 * Try to acquire the lock
>>  		 */
>> @@ -566,13 +573,28 @@ static bool rwsem_optimistic_spin(struct rw_semaphore *sem)
>>  		}
>>  
>>  		/*
>> -		 * When there's no owner, we might have preempted between the
>> -		 * owner acquiring the lock and setting the owner field. If
>> -		 * we're an RT task that will live-lock because we won't let
>> -		 * the owner complete.
>> +		 * An RT task cannot do optimistic spinning if it cannot
>> +		 * be sure the lock holder is running or live-lock may
>> +		 * happen if the current task and the lock holder happen
>> +		 * to run in the same CPU.
>> +		 *
>> +		 * When there's no owner or is reader-owned, an RT task
>> +		 * will stop spinning if the owner state is not a writer
>> +		 * at the previous iteration of the loop. This allows the
>> +		 * RT task to recheck if the task that steals the lock is
>> +		 * a spinnable writer. If so, it can keeps on spinning.
>> +		 *
>> +		 * If the owner is a writer, the need_resched() check is
>> +		 * done inside rwsem_spin_on_owner(). If the owner is not
>> +		 * a writer, need_resched() check needs to be done here.
>>  		 */
>> -		if (!sem->owner && (need_resched() || rt_task(current)))
>> -			break;
>> +		if (owner_state != OWNER_WRITER) {
>> +			if (need_resched())
>> +				break;
>> +			if (is_rt_task && (prev_owner_state != OWNER_WRITER))
>> +				break;
>> +		}
>> +		prev_owner_state = owner_state;
>>  
>>  		/*
>>  		 * The cpu_relax() call is a compiler barrier which forces
> This patch confuses me mightily. I mean, I see what it does, but I can't
> figure out why. The Changelog is just one big source of confusion.

Sorry for confusing you. If count and owner are separate, there is a
time lag where the owner is NULL, but the lock is not free yet.
Similarly, the lock could be free but another task may have stolen the
lock if the waiter bit isn't set. In the former case, an extra iteration
gives it more time for the lock holder to release the lock. In the
latter case, if the new lock owner is a writer and set owner in time,
the RT task can keep on spinning. Will clarify that in the commit log
and the comment.

Cheers,
Longman