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
