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

[Peter Zijlstra]

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