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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20190417131841.GF4038@hirez.programming.kicks-ass.net>
Date:   Wed, 17 Apr 2019 15:18:41 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Waiman Long <longman@...hat.com>
Cc:     Ingo Molnar <mingo@...hat.com>, Will Deacon <will.deacon@....com>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-kernel@...r.kernel.org, x86@...nel.org,
        Davidlohr Bueso <dave@...olabs.net>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Tim Chen <tim.c.chen@...ux.intel.com>,
        huang ying <huang.ying.caritas@...il.com>
Subject: Re: [PATCH v4 09/16] locking/rwsem: Ensure an RT task will not spin
 on reader

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@...hat.com>
> ---
>  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.

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

If you want one extra trylock attempt, why make it conditional on RT,
why not something simple like this?

--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -556,7 +556,7 @@ static bool rwsem_optimistic_spin(struct
 		 */
 		if (rwsem_try_write_lock_unqueued(sem)) {
 			taken = true;
-			break;
+			goto unlock;
 		}
 
 		/*
@@ -576,6 +576,11 @@ static bool rwsem_optimistic_spin(struct
 		 */
 		cpu_relax();
 	}
+
+	if (rwsem_try_write_lock_unqueued(sem))
+		taken = true;
+
+unlock:
 	osq_unlock(&sem->osq);
 done:
 	preempt_enable();