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
