On 10/17/2016 02:45 PM, Waiman Long wrote:
On 10/07/2016 10:52 AM, Peter Zijlstra wrote:
   * Actual trylock that will work on any unlocked state.
+ *
+ * When setting the owner field, we must preserve the low flag bits.
+ *
+ * Be careful with @handoff, only set that in a wait-loop (where you set
+ * HANDOFF) to avoid recursive lock attempts.
-static inline bool __mutex_trylock(struct mutex *lock)
+static inline bool __mutex_trylock(struct mutex *lock, const bool handoff)
      unsigned long owner, curr = (unsigned long)current;

      owner = atomic_long_read(&lock->owner);
      for (;;) { /* must loop, can race against a flag */
-        unsigned long old;
+        unsigned long old, flags = __owner_flags(owner);
+        if (__owner_task(owner)) {
+            if (handoff&&  unlikely(__owner_task(owner) == current)) {
+                /*
+                 * Provide ACQUIRE semantics for the lock-handoff.
+                 *
+                 * We cannot easily use load-acquire here, since
+                 * the actual load is a failed cmpxchg, which
+                 * doesn't imply any barriers.
+                 *
+                 * Also, this is a fairly unlikely scenario, and
+                 * this contains the cost.
+                 */

I am not so sure about your comment here. I guess you are referring to the atomic_long_cmpxchg_acquire below for the failed cmpxchg. However, it is also possible that the path can be triggered on the first round without cmpxchg. Maybe we can do a load_acquire on the owner again to satisfy this requirement without a smp_mb().

+                smp_mb(); /* ACQUIRE */
+                return true;
+            }

-        if (__owner_task(owner))
              return false;
+        }

-        old = atomic_long_cmpxchg_acquire(&lock->owner, owner,
-                          curr | __owner_flags(owner));
+        /*
+         * We set the HANDOFF bit, we must make sure it doesn't live
+         * past the point where we acquire it. This would be possible
+         * if we (accidentally) set the bit on an unlocked mutex.
+         */
+        if (handoff)
+            flags&= ~MUTEX_FLAG_HANDOFF;
+ old = atomic_long_cmpxchg_acquire(&lock->owner, owner, curr | flags);
          if (old == owner)
              return true;

Other than that, the code is fine.

Reviewed-by: Waiman Long <waiman.l...@hpe.com>

One more thing, I think it may be worthwhile to add another comment about what happens when the HANDOFF bit was set while we take the error path (goto err). As the actual handoff is serialized by the wait_lock, the code will still do the right thing. Either the next one in the queue will be handed off or it will be unlocked if the queue is empty.


Reply via email to