Messages in this thread Patch in this message | | | Subject | Re: [PATCH 2/2] locking/rwsem: Use a return variable in rwsem_spin_on_owner() | From | Jason Low <> | Date | Wed, 08 Apr 2015 23:40:11 -0700 |
| |
On Thu, 2015-04-09 at 07:37 +0200, Ingo Molnar wrote:
> The 'break' path does not seem to be equivalent, we used to do: > > > - rcu_read_unlock(); > > - return false; > > and now we'll do: > > > + ret = false; > ... > > + if (!READ_ONCE(sem->owner)) { > > + long count = READ_ONCE(sem->count); > > it's harmless (we do one more round of checking), but that's not an > equivalent transformation and slows down the preemption trigger a > (tiny) bit, because the chance that we actually catch the lock when > breaking out early is vanishingly small. (It might in fact do the > wrong thing in returning true if need_resched() is set and we've > switched owners in that small window.) > > Given how dissimilar the return path is in this case, I'm not sure > it's worth sharing it. This might be one of the few cases where > separate return statements is the better solution.
I also preferred the multiple returns for the rwsem variant to avoid needing to check sem->owner when it should go to slowpath, as you mentioned.
Now that I think of it though, if we want to have just one return path, we can still do that if we add an "out" label.
--- kernel/locking/rwsem-xadd.c | 25 +++++++++++++------------ 1 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c index 3417d01..e74240f 100644 --- a/kernel/locking/rwsem-xadd.c +++ b/kernel/locking/rwsem-xadd.c @@ -327,38 +327,39 @@ done: static noinline bool rwsem_spin_on_owner(struct rw_semaphore *sem, struct task_struct *owner) { - long count; + bool ret = true; rcu_read_lock(); while (sem->owner == owner) { /* * Ensure we emit the owner->on_cpu, dereference _after_ - * checking sem->owner still matches owner, if that fails, - * owner might point to free()d memory, if it still matches, + * checking sem->owner still matches owner. If that fails, + * owner might point to freed memory. If it still matches, * the rcu_read_lock() ensures the memory stays valid. */ barrier(); - /* abort spinning when need_resched or owner is not running */ + /* Abort spinning when need_resched or owner is not running. */ if (!owner->on_cpu || need_resched()) { rcu_read_unlock(); - return false; + ret = false; + goto out; } cpu_relax_lowlatency(); } 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. + * held by readers. Check the counter to verify the state. */ - count = READ_ONCE(sem->count); - return (count == 0 || count == RWSEM_WAITING_BIAS); + if (!READ_ONCE(sem->owner)) { + long count = READ_ONCE(sem->count); + ret = (count == 0 || count == RWSEM_WAITING_BIAS); + } +out: + return ret; } static bool rwsem_optimistic_spin(struct rw_semaphore *sem) -- 1.7.2.5
| |