linux-kernel - Re: [PATCH v4 12/16] locking/rwsem: Enable time-based spinning on reader-owned rwsem

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]

Date:   Thu, 18 Apr 2019 15:06:11 +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 12/16] locking/rwsem: Enable time-based spinning on
 reader-owned rwsem


So I really dislike time based spinning, and we've always rejected it
before.

On Sat, Apr 13, 2019 at 01:22:55PM -0400, Waiman Long wrote:

> +static inline u64 rwsem_rspin_threshold(struct rw_semaphore *sem)
> +{
> +	long count = atomic_long_read(&sem->count);
> +	int reader_cnt = atomic_long_read(&sem->count) >> RWSEM_READER_SHIFT;
> +
> +	if (reader_cnt > 30)
> +		reader_cnt = 30;
> +	return sched_clock() + ((count & RWSEM_FLAG_WAITERS)
> +		? 10 * NSEC_PER_USEC + reader_cnt * NSEC_PER_USEC/2
> +		: 25 * NSEC_PER_USEC);
> +}

Urgh, why do you _have_ to write unreadable code :-(

static inline u64 rwsem_rspin_threshold(struct rw_semaphore *sem)
{
	long count = atomic_long_read(&sem->count);
	u64 delta = 25 * NSEC_PER_USEC;

	if (count & RWSEM_FLAG_WAITERS) {
		int readers = count >> RWSEM_READER_SHIFT;

		if (readers > 30)
			readers = 30;

		delta = (20 + readers) * NSEC_PER_USEC / 2;
	}

	return sched_clock() + delta;
}

I don't get it though; the number of current read-owners is independent
of WAITERS, while the hold time does correspond to it.

So why do we have that WAITERS check in there?

> @@ -616,6 +678,35 @@ static bool rwsem_optimistic_spin(struct rw_semaphore *sem, bool wlock)
>  		if (taken)
>  			break;
>  
> +		/*
> +		 * Time-based reader-owned rwsem optimistic spinning
> +		 */

This relies on rwsem_spin_on_owner() not actually spinning for
read-owned.

> +		if (wlock && (owner_state == OWNER_READER)) {
> +			/*
> +			 * Initialize rspin_threshold when the owner
> +			 * state changes from non-reader to reader.
> +			 */
> +			if (prev_owner_state != OWNER_READER) {
> +				if (!is_rwsem_spinnable(sem))
> +					break;
> +				rspin_threshold = rwsem_rspin_threshold(sem);
> +				loop = 0;
> +			}

This seems fragile, why not to the rspin_threshold thing _once_ at the
start of this function?

This way it can be reset.

> +			/*
> +			 * Check time threshold every 16 iterations to
> +			 * avoid calling sched_clock() too frequently.
> +			 * This will make the actual spinning time a
> +			 * bit more than that specified in the threshold.
> +			 */
> +			else if (!(++loop & 0xf) &&
> +				 (sched_clock() > rspin_threshold)) {

Why is calling sched_clock() lots a problem?

> +				rwsem_set_nonspinnable(sem);
> +				lockevent_inc(rwsem_opt_nospin);
> +				break;
> +			}
> +		}
> +
>  		/*
>  		 * An RT task cannot do optimistic spinning if it cannot
>  		 * be sure the lock holder is running or live-lock may