Re: [patch 00/21] mutex subsystem, -V14

[Olof Johansson <olof@lixom.net>]

On Thu, Jan 05, 2006 at 05:06:26PM -0600, Joel Schopp wrote:

> Here is a first pass at a powerpc file for the fast paths just as an 
> FYI/RFC. It is completely untested, but compiles.

You really should test it, it saves reviewers time. It's not that hard
to at least try booting it.

Besides the isync comments earlier, there's a bunch of whitespace issues
going on. Did you copy and paste the code from somewhere? If so, you
should move the original copyright over too.

All your macros use spaces instead of tabs up to the \, should be
changed.

All tmp variables should be ints, since the atomic_t counter is a 32-bit
variable. If you use longs, and lwarx (loads 32-bit without sign
extend), the comparison with < 0 will never be true.

> Index: 2.6.15-mutex14/include/asm-powerpc/mutex.h
> ===================================================================
> --- 2.6.15-mutex14.orig/include/asm-powerpc/mutex.h	2006-01-04 14:46:31.%N -0600
> +++ 2.6.15-mutex14/include/asm-powerpc/mutex.h	2006-01-05 16:25:41.%N -0600
> @@ -1,9 +1,83 @@
>  /*
> - * Pull in the generic implementation for the mutex fastpath.
> + * include/asm-powerpc/mutex.h

No need to keep filenames in files.

>   *
> - * TODO: implement optimized primitives instead, or leave the generic
> - * implementation in place, or pick the atomic_xchg() based generic
> - * implementation. (see asm-generic/mutex-xchg.h for details)
> + * PowerPC optimized mutex locking primitives
> + *
> + * Please look into asm-generic/mutex-xchg.h for a formal definition.
> + * Copyright (C) 2006 Joel Schopp <jschopp@austin.ibm.com>, IBM
>   */
> +#ifndef _ASM_MUTEX_H
> +#define _ASM_MUTEX_H
> +#define __mutex_fastpath_lock(count, fail_fn)\
> +do{                                     \
> +	long tmp;                       \
> +	__asm__ __volatile__(		\
> +"1:	lwarx		%0,0,%1\n"      \
> +"	addic	        %0,%0,-1\n"     \
> +"	stwcx.          %0,0,%1\n"      \
> +"	bne-            1b\n"           \
> +"	isync           \n"             \
> +	: "=&r" (tmp)                   \
> +	: "r" (&(count)->counter)       \
> +	: "cr0", "memory");             \
> +	if (unlikely(tmp < 0))          \
> +		fail_fn(count);         \
> +} while (0)                              

trailing whitespace

> +
> +#define __mutex_fastpath_unlock(count, fail_fn)\
> +do{                                         \
> +	long tmp;                           \
> +	__asm__ __volatile__(SYNC_ON_SMP    \
> +"1:	lwarx		%0,0,%1\n"          \
> +"	addic	        %0,%0,1\n"          \
> +"	stwcx.          %0,0,%1\n"          \
> +"       bne-            1b\n"               \

space vs tab

> +	: "=&r" (tmp)                       \
> +	: "r" (&(count)->counter)           \
> +	: "cr0", "memory");                 \
> +	if (unlikely(tmp <= 0))             \
> +		fail_fn(count);             \
> +} while (0)
> +
> +
> +static inline int 

trailing whitespace

> +__mutex_fastpath_trylock(atomic_t* count, int (*fail_fn)(atomic_t*))

atomic_t *count

> +{
> +	long tmp;
> +	__asm__ __volatile__(
> +"1:     lwarx		%0,0,%1\n"
> +"       cmpwi           0,%0,1\n"
> +"       bne-            2f\n"
> +"       stwcx.          %0,0,%1\n"

space vs tab on the above 4 lines

Shouldn't you decrement the counter before the store?

> +"	bne-		1b\n"
> +"	isync\n"
> +"2:"
> +	: "=&r" (tmp)
> +	: "r" (&(count)->counter)
> +	: "cr0", "memory");
> +
> +	return (int)tmp;
> +
> +}
> +
> +#define __mutex_slowpath_needs_to_unlock()		1
>  
> -#include <asm-generic/mutex-dec.h>
> +static inline int 

trailing whitespace

> +__mutex_fastpath_lock_retval(atomic_t* count, int (*fail_fn)(atomic_t *))

atomic_t *count

> +{
> +	long tmp;

counter is a 32-bit variable, so should tmp be otherwise the < 0
comparison can never be true.

> +	__asm__ __volatile__(
> +"1:	lwarx		%0,0,%1\n"
> +"	addic	        %0,%0,-1\n"
> +"	stwcx.          %0,0,%1\n"
> +"	bne-            1b\n"
> +"	isync           \n"
> +	: "=&r" (tmp)
> +	: "r" (&(count)->counter)
> +	: "cr0", "memory");
> +	if (unlikely(tmp < 0))
> +		return fail_fn(count);
> +	else
> +		return 0;
> +}
> +#endif