lkml.org 
[lkml]   [2016]   [May]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2016-05-12 22:41    [W:0.146 / U:0.492 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site