Re: [PATCH] 2.4.18-pre9, trylock for read/write semaphores

[David Howells <dhowells@redhat.com>]

Hi Brian,

Having examined your code again, I've got some comments on it:

I think the following would be more elegant:

	static inline int __down_read_trylock(struct rw_semaphore *sem)
	{
		signed long old, new;

		do {
			old = (volatile signed long)sem->count;
			if (old < RWSEM_UNLOCKED_VALUE)
				return 0;
			new = old + RWSEM_ACTIVE_READ_BIAS;
		} while (cmpxchg(&sem->count, old, new) != old);

		return 1;
	}

I'm also not sure that the cast has any effect in the following excerpt from
the above:

	old = (volatile signed long)sem->count;

What you may actually want is:

	static inline int __down_read_trylock(struct rw_semaphore *sem)
	{
		volatile signed long *pcount;
		signed long old, new;

		pcount = &sem->count;
		do {
			old = *pcount;
			if (old < RWSEM_UNLOCKED_VALUE)
				return 0;
			new = old + RWSEM_ACTIVE_READ_BIAS;
		} while (cmpxchg(pcount, old, new) != old);

		return 1;
	}

Or maybe even:

	static inline int __down_read_trylock(struct rw_semaphore *sem)
	{
		__s32 result, tmp;
		__asm__ __volatile__(
			"# beginning __down_read_trylock\n\t"
			"  movl      %0,%1\n\t"
			"1:\n\t"
			"  movl	     %1,%2\n\t"
			"  addl      %3,%2\n\t"
			"  jle	     2f\n\t"
	LOCK_PREFIX	"  cmpxchgl  %2,%0\n\t"
			"  jnz	     1b\n\t"
			"# ending __down_read_trylock\n\t"
			: "+m"(sem->count), "=&a"(result), "+&r"(tmp)
			: "i"(RWSEM_ACTIVE_READ_BIAS)
			: "memory", "cc");
		return result>=0 ? 1 : 0;
	}

Using this inline assembly has three advantages over mixing lots of C into it:

 (1) If the sign/zero flags are set as the result of calculating the new
     value, then the old value was negative, and so the trylock has failed.

 (2) CMPXCHG sets the zero flag on success and clears it on failure (something
     that C can't test with the current inline asm setup).

 (3) CMPXCHG fetches the current value of sem->count into EAX if it fails to
     swap (so we don't need to get it again - hence why "1:" is where it is).

Point (3) could be incorporated into the C version. It may also be desirable
to include a rep;nop in whichever version is used, but I don't think it's
going to spin enough for that.


David