lkml.org 
[lkml]   [2019]   [Apr]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [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


\
 
 \ /
  Last update: 2019-04-18 15:06    [W:0.676 / U:5.448 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site