Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id <S291455AbSBMIUP>; Wed, 13 Feb 2002 03:20:15 -0500
Received: (majordomo@vger.kernel.org) by vger.kernel.org
	id <S291631AbSBMIT5>; Wed, 13 Feb 2002 03:19:57 -0500
Received: from dell-paw-3.cambridge.redhat.com ([195.224.55.237]:63985 "HELO
	executor.cambridge.redhat.com") by vger.kernel.org with SMTP
	id <S291448AbSBMITo>; Wed, 13 Feb 2002 03:19:44 -0500
To: bwatson@kahuna.cag.cpqcorp.net
Cc: marcelo@conectiva.com.br, linux-kernel@vger.kernel.org,
        dhowells@redhat.com, hch@caldera.de
Subject: Re: [PATCH] 2.4.18-pre9, trylock for read/write semaphores 
In-Reply-To: Message from bwatson@kahuna.cag.cpqcorp.net 
   of "Tue, 12 Feb 2002 14:45:00 PST." <200202122257.g1CMv9W05368@kahuna.cag.cpqcorp.net> 
User-Agent: EMH/1.14.1 SEMI/1.14.3 (Ushinoya) FLIM/1.14.3
 (=?ISO-8859-4?Q?Unebigory=F2mae?=) APEL/10.3 Emacs/21.1
 (i386-redhat-linux-gnu) MULE/5.0 (SAKAKI)
MIME-Version: 1.0 (generated by SEMI 1.14.3 - "Ushinoya")
Content-Type: text/plain; charset=US-ASCII
Date: Wed, 13 Feb 2002 08:19:43 +0000
Message-ID: <26130.1013588383@warthog.cambridge.redhat.com>
From: David Howells <dhowells@redhat.com>
Sender: linux-kernel-owner@vger.kernel.org
X-Mailing-List: linux-kernel@vger.kernel.org


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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/