Bug in ia64 specific down() function??

Bug in ia64 specific down() function??

[Chen, Kenneth W]

The memory order semantics for include/asm-ia64/semaphore.h:down()
doesn't look right.  It is using atomic_dec_return, which eventually
translate into ia64_fetch_and_add() that uses release semantics.
Shouldn't it use acquire semantics?

- Ken

Re: Bug in ia64 specific down() function??

[Zoltan Menyhart]

Chen, Kenneth W wrote:
> The memory order semantics for include/asm-ia64/semaphore.h:down()
> doesn't look right.  It is using atomic_dec_return, which eventually
> translate into ia64_fetch_and_add() that uses release semantics.
> Shouldn't it use acquire semantics?

What about this one:

--- linux-2.6.15-test/include/asm-ia64/semaphore.h	2006-01-10 13:54:31.000000000 +0100
+++ linux-2.6.15-test-down/include/asm-ia64/semaphore.h	2006-01-13 16:16:04.000000000 +0100
@@ -61,7 +61,7 @@ down (struct semaphore * sem)
 down (struct semaphore *sem)
 {
 	might_sleep();
-	if (atomic_dec_return(&sem->count) < 0)
+	if (ia64_fetchadd(-1, &sem->count.counter, acq) < 1)
 		__down(sem);
 }
 
@@ -75,7 +75,7 @@ down_interruptible (struct semaphore * sem)
 	int ret = 0;
 
 	might_sleep();
-	if (atomic_dec_return(&sem->count) < 0)
+	if (ia64_fetchadd(-1, &sem->count.counter, acq) < 1)
 		ret = __down_interruptible(sem);
 	return ret;
 }
@@ -85,7 +85,7 @@ down_trylock (struct semaphore *sem)
 {
 	int ret = 0;
 
-	if (atomic_dec_return(&sem->count) < 0)
+	if (ia64_fetchadd(-1, &sem->count.counter, acq) < 1)
 		ret = __down_trylock(sem);
 	return ret;
 }
@@ -93,7 +93,7 @@ up (struct semaphore *sem)
 static inline void
 up (struct semaphore * sem)
 {
-	if (atomic_inc_return(&sem->count) <= 0)
+	if (ia64_fetchadd(1, &sem->count.counter, rel) <= -1)
 		__up(sem);
 }

I do not like the too long chains of the macro definitions which --
as the example shows -- hide how the things work.
This is why I used "ia64_fetchadd()".
("IA64_FETCHADD()" is a bit nasty.)

"__down_interruptible()" and "__down()" could be cleaned up too, but
the atomic operation on the semaphore is followed by a wakeup, that
includes taking a lock, that provides the ".acq" semantics.

"__down_trylock()" is called on failure, the data area protected
by the semaphore will not be touched by the caller.

Zoltan Menyhart

RE: Bug in ia64 specific down() function??

[Chen, Kenneth W]

Zoltan Menyhart wrote on Friday, January 13, 2006 8:25 AM
> Chen, Kenneth W wrote:
> > The memory order semantics for include/asm-ia64/semaphore.h:down()
> > doesn't look right.  It is using atomic_dec_return, which eventually
> > translate into ia64_fetch_and_add() that uses release semantics.
> > Shouldn't it use acquire semantics?
> 
> What about this one:
> 
> --- linux-2.6.15-test/include/asm-ia64/semaphore.h	2006-01-10 13:54:31.000000000 +0100
> +++ linux-2.6.15-test-down/include/asm-ia64/semaphore.h	2006-01-13 16:16:04.000000000 +0100
> @@ -61,7 +61,7 @@ down (struct semaphore * sem)
>  down (struct semaphore *sem)
>  {
>  	might_sleep();
> -	if (atomic_dec_return(&sem->count) < 0)
> +	if (ia64_fetchadd(-1, &sem->count.counter, acq) < 1)
>  		__down(sem);
>  }
>  
> @@ -75,7 +75,7 @@ down_interruptible (struct semaphore * sem)
>  	int ret = 0;
>  
>  	might_sleep();
> -	if (atomic_dec_return(&sem->count) < 0)
> +	if (ia64_fetchadd(-1, &sem->count.counter, acq) < 1)
>  		ret = __down_interruptible(sem);
>  	return ret;
>  }
> @@ -85,7 +85,7 @@ down_trylock (struct semaphore *sem)
>  {
>  	int ret = 0;
>  
> -	if (atomic_dec_return(&sem->count) < 0)
> +	if (ia64_fetchadd(-1, &sem->count.counter, acq) < 1)
>  		ret = __down_trylock(sem);
>  	return ret;
>  }
> @@ -93,7 +93,7 @@ up (struct semaphore *sem)
>  static inline void
>  up (struct semaphore * sem)
>  {
> -	if (atomic_inc_return(&sem->count) <= 0)
> +	if (ia64_fetchadd(1, &sem->count.counter, rel) <= -1)
>  		__up(sem);
>  }

Yeah, looked OK to me.

- Ken