Re: [patch 1/3] mutex subsystem: trylock

[Nicolas Pitre <nico@cam.org>]

On Wed, 28 Dec 2005, Ingo Molnar wrote:

> 
> * Ingo Molnar <mingo@elte.hu> wrote:
> 
> > * Nicolas Pitre <nico@cam.org> wrote:
> > 
> > > > here we go to great trouble trying to avoid the 'slowpath', while we 
> > > > unconditionally force the next unlock into the slowpath! So we have 
> > > > not won anything. (on a cycle count basis it's probably even a net 
> > > > loss)
> > > 
> > > I disagree.  [...elaborate analysis of the code ...]
> > 
> > you are right, it should work fine, and should be optimal. I'll add 
> > your xchg variant to mutex-xchg.h.
> 
> the patch below adds it, and it boots fine on x86 with mutex.c hacked to 
> include asm-generic/mutex-xchg.h.

Here's an additional patch to fix some comments, and to add a small 
optimization.

Index: linux-2.6/include/asm-generic/mutex-xchg.h
===================================================================
--- linux-2.6.orig/include/asm-generic/mutex-xchg.h
+++ linux-2.6/include/asm-generic/mutex-xchg.h
@@ -75,9 +75,14 @@ do {									\
  *  @count: pointer of type atomic_t
  *  @fn: spinlock based trylock implementation
  *
- * We call the spinlock based generic variant, because atomic_xchg() is
- * too destructive to provide a simpler trylock implementation than the
- * spinlock based one.
+ * Change the count from 1 to a value lower than 1, and return 0 (failure)
+ * if it wasn't 1 originally, or return 1 (success) otherwise. This function
+ * MUST leave the value lower than 1 even when the "1" assertion wasn't true.
+ * Additionally, if the value was < 0 originally, this function must not leave
+ * it to 0 on failure.
+ *
+ * If the architecture has no effective trylock variant, it should call the
+ * <fn> spinlock-based trylock variant unconditionally.
  */
 static inline int
 __mutex_fastpath_trylock(atomic_t *count, int (*fn)(atomic_t *))
@@ -90,12 +95,13 @@ __mutex_fastpath_trylock(atomic_t *count
 		 * state. If while doing so we get back a prev value of 1
 		 * then we just own it.
 		 *
-		 * [ In the rare case of the mutex going to 1 and then to 0
-		 *   in this few-instructions window, this has the potential
-		 *   to trigger the slowpath for the owner's unlock path, but
-		 *   that's not a problem in practice. ]
+		 * [ In the rare case of the mutex going to 1, to 0, to -1
+		 *   and then back to 0 in this few-instructions window,
+		 *   this has the potential to trigger the slowpath for the
+		 *   owner's unlock path needlessly, but that's not a problem
+		 *   in practice. ]
 		 */
-		prev = atomic_xchg(count, -1);
+		prev = atomic_xchg(count, prev);
 		if (prev < 0)
 			prev = 0;
 	}