linux-kernel - Re: [RFC PATCH-tip v2 3/6] locking/rwsem: Enable count-based spinning 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: <20160615173830.GR30921@twins.programming.kicks-ass.net>
Date:	Wed, 15 Jun 2016 19:38:30 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Waiman Long <Waiman.Long@....com>
Cc:	Ingo Molnar <mingo@...hat.com>, linux-kernel@...r.kernel.org,
	x86@...nel.org, linux-alpha@...r.kernel.org,
	linux-ia64@...r.kernel.org, linux-s390@...r.kernel.org,
	linux-arch@...r.kernel.org, Davidlohr Bueso <dave@...olabs.net>,
	Jason Low <jason.low2@...com>,
	Dave Chinner <david@...morbit.com>,
	Scott J Norton <scott.norton@....com>,
	Douglas Hatch <doug.hatch@....com>
Subject: Re: [RFC PATCH-tip v2 3/6] locking/rwsem: Enable count-based
 spinning on reader

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.

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

full 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();