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
