Messages in this thread | | | Date | Thu, 12 May 2016 16:15:22 -0400 | From | Waiman Long <> | Subject | Re: [PATCH v2] locking/rwsem: Add reader-owned state to the owner field |
| |
On 05/11/2016 06:04 PM, Peter Hurley wrote: > /* Grant an infinite number of read locks to the readers at the front > @@ -306,16 +312,11 @@ static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem) > > rcu_read_lock(); > owner = READ_ONCE(sem->owner); > - if (!owner) { > - long count = READ_ONCE(sem->count); > + if (!rwsem_is_writer_owned(owner)) { > /* > - * If sem->owner is not set, yet we have just recently entered the > - * slowpath with the lock being active, then there is a possibility > - * reader(s) may have the lock. To be safe, bail spinning in these > - * situations. > + * Don't spin if the rwsem is readers owned. > */ > - if (count& RWSEM_ACTIVE_MASK) > - ret = false; > + ret = !rwsem_is_reader_owned(owner); > goto done; > } > I'm not a big fan of all the helpers; istm like they're obfuscating the more > important requirements of rwsem. For example, this reduces to > > rcu_read_lock(); > owner = READ_ONCE(sem->owner); > ret = !owner || (rwsem_is_writer_owned(owner)&& owner->on_cpu); > rcu_read_unlock(); > return ret; >
Using helper functions usually make the code easier to read. This is helpful for the rwsem code which can be hard to understand especially how the different count values interact.
> >> @@ -328,8 +329,6 @@ done: >> static noinline >> bool rwsem_spin_on_owner(struct rw_semaphore *sem, struct task_struct *owner) >> { >> - long count; >> - >> rcu_read_lock(); >> while (sem->owner == owner) { >> /* >> @@ -350,16 +349,11 @@ bool rwsem_spin_on_owner(struct rw_semaphore *sem, struct task_struct *owner) >> } >> rcu_read_unlock(); >> >> - if (READ_ONCE(sem->owner)) >> - return true; /* new owner, continue spinning */ >> - >> /* >> - * When the owner is not set, the lock could be free or >> - * held by readers. Check the counter to verify the >> - * state. >> + * If there is a new owner or the owner is not set, we continue >> + * spinning. >> */ >> - count = READ_ONCE(sem->count); >> - return (count == 0 || count == RWSEM_WAITING_BIAS); >> + return !rwsem_is_reader_owned(READ_ONCE(sem->owner)); > It doesn't make sense to force reload sem->owner here; if sem->owner > is not being reloaded then the loop above will execute forever.
I agree that we don't actually need to use READ_ONCE() here for sem->owner as the barrier() call will force the reloading. It is more like a habit to use it for public variable or we will have to think a bit harder to make sure that we are doing the right thing.
> Arguably, this check should be bumped out to the optimistic spin and > reload/check the owner there? > > Or better yet; don't pass the owner in as a parameter at all, but > instead snapshot the owner and check its ownership on entry.
That will make the main optimistic spinning loop more complex.
> > Because see below... > >> } >> >> static bool rwsem_optimistic_spin(struct rw_semaphore *sem) >> @@ -378,7 +372,8 @@ static bool rwsem_optimistic_spin(struct rw_semaphore *sem) >> >> while (true) { >> owner = READ_ONCE(sem->owner); >> - if (owner&& !rwsem_spin_on_owner(sem, owner)) >> + if (rwsem_is_writer_owned(owner)&& >> + !rwsem_spin_on_owner(sem, owner)) >> break; >> >> /* wait_lock will be acquired if write_lock is obtained */ >> @@ -391,9 +386,11 @@ static bool rwsem_optimistic_spin(struct rw_semaphore *sem) >> * When there's no owner, we might have preempted between the >> * owner acquiring the lock and setting the owner field. If >> * we're an RT task that will live-lock because we won't let >> - * the owner complete. >> + * the owner complete. We also quit if the lock is owned by >> + * readers. >> */ >> - if (!owner&& (need_resched() || rt_task(current))) >> + if (rwsem_is_reader_owned(owner) || >> + (!owner&& (need_resched() || rt_task(current)))) > This is using the stale owner value that was cached before spinning on the owner; > That can't be right.
The code is going to loop back and reload the new owner value anyway. It is just a bit of additional latency. I will move the is_reader check up after loading sem->owner to clear any confusion.
Cheers, Longman
| |