Messages in this thread | | | Date | Mon, 9 May 2016 22:24:16 -0400 | From | Waiman Long <> | Subject | Re: [PATCH v2] locking/rwsem: Add reader-owned state to the owner field |
| |
On 05/09/2016 04:27 AM, Peter Zijlstra wrote: > On Fri, May 06, 2016 at 08:20:24PM -0400, Waiman Long wrote: >> @@ -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. We also quit if the lock is owned by >> + * readers. > Maybe also note why we quit on readers.
Sure. Will do so.
>> */ >> + if (rwsem_is_reader_owned(owner) || >> + (!owner&& (need_resched() || rt_task(current)))) >> break; >> >> /* > >> diff --git a/kernel/locking/rwsem.h b/kernel/locking/rwsem.h >> index 870ed9a..d7fea18 100644 >> --- a/kernel/locking/rwsem.h >> +++ b/kernel/locking/rwsem.h >> @@ -1,3 +1,20 @@ >> +/* >> + * The owner field of the rw_semaphore structure will be set to >> + * RWSEM_READ_OWNED when a reader grabs the lock. A writer will clear >> + * the owner field when it unlocks. A reader, on the other hand, will >> + * not touch the owner field when it unlocks. >> + * >> + * In essence, the owner field now has the following 3 states: >> + * 1) 0 >> + * - lock is free or the owner hasn't set the field yet >> + * 2) RWSEM_READER_OWNED >> + * - lock is currently or previously owned by readers (lock is free >> + * or not set by owner yet) >> + * 3) Other non-zero value >> + * - a writer owns the lock >> + */ >> +#define RWSEM_READER_OWNED 1UL > #define RWSEM_READER_OWNED ((struct task_struct *)1UL)
Will make the change.
>> + >> #ifdef CONFIG_RWSEM_SPIN_ON_OWNER >> static inline void rwsem_set_owner(struct rw_semaphore *sem) >> { >> @@ -9,6 +26,26 @@ static inline void rwsem_clear_owner(struct rw_semaphore *sem) >> sem->owner = NULL; >> } >> >> +static inline void rwsem_set_reader_owned(struct rw_semaphore *sem) >> +{ >> + /* >> + * We check the owner value first to make sure that we will only >> + * do a write to the rwsem cacheline when it is really necessary >> + * to minimize cacheline contention. >> + */ >> + if (sem->owner != (struct task_struct *)RWSEM_READER_OWNED) >> + sem->owner = (struct task_struct *)RWSEM_READER_OWNED; > How much if anything did this optimization matter?
I hadn't run any performance test to verify the effective of this change. For a reader-heavy rwsem, this change should be able to save quite a lot of needless write to the rwsem cacheline.
>> +} >> + >> +static inline bool rwsem_is_writer_owned(struct task_struct *owner) >> +{ >> + return (unsigned long)owner> RWSEM_READER_OWNED; >> +} > Tad too clever that; what does GCC generate if you write the obvious: > > return owner&& owner != RWSEM_READER_OWNER;
You are right. GCC is intelligent enough to make the necessary optimization. I will revert it to this form which is more obvious.
>> + >> +static inline bool rwsem_is_reader_owned(struct task_struct *owner) >> +{ >> + return owner == (struct task_struct *)RWSEM_READER_OWNED; >> +} > So I don't particularly like these names; they read like they take a > rwsem as argument, but they don't. > > Would something like: rwsem_owner_is_{reader,writer}() make more sense?
Yes, these names look good to me.
Cheers, Longman
| |