Patch: linux-2.5.29 __downgrade_write() for CONFIG_RWSEM_GENERIC_SPINLOCK

Patch: linux-2.5.29 __downgrade_write() for CONFIG_RWSEM_GENERIC_SPINLOCK

[Adam J. Richter]

	linux-2.5.29 lacks __downgrade_write() platforms that use
CONFIG_RWSEM_GENERIC_SPINLOCK, such as i386 (as opposed to later x86
processors).  This causes a compiler warnings for compilations
of numerous files.

	Although noting in 2.5.29 appears to use downgrade_write(),
I assume that the facility was added because it is going to be used
in the near future.  So, I've added what I think is an implementation
of __downgrade_write for lib/rwsem-spinlock.c.  It is the same as
__up_write, except that it sets sem->activity to 1.  Since nothing
uses it yet, I haven't tested it.

	David, please let me know if you are going to forward this
to Linus, if you want me to submit it to Linus, or if you want to
do something else.

Adam J. Richter     __     ______________   575 Oroville Road
adam@yggdrasil.com     \ /                  Milpitas, California 95035
+1 408 309-6081         | g g d r a s i l   United States of America
                         "Free Software For The Rest Of Us."

--- linux-2.5.29/include/linux/rwsem-spinlock.h	2002-07-26 19:58:39.000000000 -0700
+++ linux/include/linux/rwsem-spinlock.h	2002-07-28 10:38:59.000000000 -0700
@@ -57,6 +57,7 @@
 extern void FASTCALL(__down_write(struct rw_semaphore *sem));
 extern void FASTCALL(__up_read(struct rw_semaphore *sem));
 extern void FASTCALL(__up_write(struct rw_semaphore *sem));
+extern void FASTCALL(__downgrade_write(struct rw_semaphore *sem));
 
 #endif /* __KERNEL__ */
 #endif /* _LINUX_RWSEM_SPINLOCK_H */
--- linux-2.5.29/lib/rwsem-spinlock.c	2002-07-26 19:58:30.000000000 -0700
+++ linux/lib/rwsem-spinlock.c	2002-07-28 10:49:30.000000000 -0700
@@ -229,11 +229,30 @@
 	rwsemtrace(sem,"Leaving __up_write");
 }
 
+/*
+ * downgrade a write lock into a read lock
+ */
+void __downgrade_write(struct rw_semaphore *sem)
+{
+	rwsemtrace(sem,"Entering __downgrade_write");
+
+	spin_lock(&sem->wait_lock);
+
+	sem->activity = 1;
+	if (!list_empty(&sem->wait_list))
+		sem = __rwsem_do_wake(sem);
+
+	spin_unlock(&sem->wait_lock);
+
+	rwsemtrace(sem,"Leaving __downgrade_write");
+}
+
 EXPORT_SYMBOL(init_rwsem);
 EXPORT_SYMBOL(__down_read);
 EXPORT_SYMBOL(__down_write);
 EXPORT_SYMBOL(__up_read);
 EXPORT_SYMBOL(__up_write);
+EXPORT_SYMBOL(__downgrade_write);
 #if RWSEM_DEBUG
 EXPORT_SYMBOL(rwsemtrace);
 #endif

Re: Patch: linux-2.5.29 __downgrade_write() for CONFIG_RWSEM_GENERIC_SPINLOCK

[Roman Zippel]

Hi,

On Sun, 28 Jul 2002, Adam J. Richter wrote:

> 	Although noting in 2.5.29 appears to use downgrade_write(),
> I assume that the facility was added because it is going to be used
> in the near future.  So, I've added what I think is an implementation
> of __downgrade_write for lib/rwsem-spinlock.c.  It is the same as
> __up_write, except that it sets sem->activity to 1.

IMO you have to add the patch below, otherwise you may wake up a writer.

>  Since nothing
> uses it yet, I haven't tested it.

Same problem here.

bye, Roman

diff -u -p -r1.1.1.1 rwsem-spinlock.c
--- lib/rwsem-spinlock.c	21 Oct 2001 23:50:13 -0000	1.1.1.1
+++ lib/rwsem-spinlock.c	28 Jul 2002 18:04:08 -0000
@@ -59,7 +59,7 @@ static inline struct rw_semaphore *__rws
 	/* try to grant a single write lock if there's a writer at the front of the queue
 	 * - we leave the 'waiting count' incremented to signify potential contention
 	 */
-	if (waiter->flags & RWSEM_WAITING_FOR_WRITE) {
+	if (waiter->flags & RWSEM_WAITING_FOR_WRITE && !sem->activity) {
 		sem->activity = -1;
 		list_del(&waiter->list);
 		waiter->flags = 0;

Re: Patch: linux-2.5.29 __downgrade_write() for CONFIG_RWSEM_GENERIC_SPINLOCK

[Christoph Hellwig]

On Sun, Jul 28, 2002 at 10:50:58AM -0700, Adam J. Richter wrote:
> 	linux-2.5.29 lacks __downgrade_write() platforms that use
> CONFIG_RWSEM_GENERIC_SPINLOCK, such as i386 (as opposed to later x86
> processors).  This causes a compiler warnings for compilations
> of numerous files.

It was part of the patch I sent to David.  In fact that was the first
implementation of it at all..

The full patch I sent around is attached as reference.


Index: include/asm-i386/rwsem.h
===================================================================
RCS file: /cvs/linux-2.4-xfs/linux/include/asm-i386/rwsem.h,v
retrieving revision 1.3
diff -u -p -r1.3 rwsem.h
--- include/asm-i386/rwsem.h	2002/05/29 22:20:17	1.3
+++ include/asm-i386/rwsem.h	2002/07/28 10:07:35
@@ -46,6 +46,7 @@ struct rwsem_waiter;
 extern struct rw_semaphore *FASTCALL(rwsem_down_read_failed(struct rw_semaphore *sem));
 extern struct rw_semaphore *FASTCALL(rwsem_down_write_failed(struct rw_semaphore *sem));
 extern struct rw_semaphore *FASTCALL(rwsem_wake(struct rw_semaphore *));
+extern struct rw_semaphore *FASTCALL(rwsem_downgrade_write(struct rw_semaphore *sem));
 
 /*
  * the semaphore definition
@@ -117,6 +118,29 @@ LOCK_PREFIX	"  incl      (%%eax)\n\t" /*
 }
 
 /*
+ * trylock for reading -- returns 1 if successful, 0 if contention
+ */
+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"
+		"2:\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;
+}
+
+/*
  * lock for writing
  */
 static inline void __down_write(struct rw_semaphore *sem)
@@ -144,6 +168,19 @@ LOCK_PREFIX	"  xadd      %0,(%%eax)\n\t"
 }
 
 /*
+ * trylock for writing -- returns 1 if successful, 0 if contention
+ */
+static inline int __down_write_trylock(struct rw_semaphore *sem)
+{
+	signed long ret = cmpxchg(&sem->count,
+				  RWSEM_UNLOCKED_VALUE, 
+				  RWSEM_ACTIVE_WRITE_BIAS);
+	if (ret == RWSEM_UNLOCKED_VALUE)
+		return 1;
+	return 0;
+}
+
+/*
  * unlock after reading
  */
 static inline void __up_read(struct rw_semaphore *sem)
@@ -184,6 +221,7 @@ LOCK_PREFIX	"  xaddl     %%edx,(%%eax)\n
 		"2:\n\t"
 		"  decw      %%dx\n\t" /* did the active count reduce to 0? */
 		"  jnz       1b\n\t" /* jump back if not */
+		"  pushl     $1\n\t"
 		"  pushl     %%ecx\n\t"
 		"  call      rwsem_wake\n\t"
 		"  popl      %%ecx\n\t"
@@ -194,6 +232,35 @@ LOCK_PREFIX	"  xaddl     %%edx,(%%eax)\n
 		: "a"(sem), "i"(-RWSEM_ACTIVE_WRITE_BIAS)
 		: "memory", "cc", "edx");
 }
+
+/*
+ * downgrade write lock to read lock
+ */
+static inline void __downgrade_write(struct rw_semaphore *sem)
+{
+	__asm__ __volatile__(
+		"# beginning __downgrade_write\n\t"
+LOCK_PREFIX	"  addl      %2,(%%eax)\n\t" /* transitions 0xZZZZ0001 -> 0xYYYY0001 */
+		"  js        2f\n\t" /* jump if the lock is being waited upon */
+		"1:\n\t"
+		LOCK_SECTION_START("")
+		"2:\n\t"
+		"  pushl     %%ecx\n\t"
+		"  pushl     %%edx\n\t"
+		"  call      rwsem_downgrade_wake\n\t"
+		"  popl      %%edx\n\t"
+		"  popl      %%ecx\n\t"
+		"  jmp       1b\n"
+		LOCK_SECTION_END
+		"# ending __downgrade_write\n"
+		: "=m"(sem->count)
+		: "a"(sem), "i"(-RWSEM_WAITING_BIAS), "m"(sem->count)
+		: "memory", "cc");
+}
+
+/*
+ * implement atomic add functionality
+ */
 
 /*
  * implement atomic add functionality
Index: include/linux/rwsem-spinlock.h
===================================================================
RCS file: /cvs/linux-2.4-xfs/linux/include/linux/rwsem-spinlock.h,v
retrieving revision 1.2
diff -u -p -r1.2 rwsem-spinlock.h
--- include/linux/rwsem-spinlock.h	2001/05/29 19:53:13	1.2
+++ include/linux/rwsem-spinlock.h	2002/07/28 10:07:37
@@ -54,9 +54,12 @@ struct rw_semaphore {
 
 extern void FASTCALL(init_rwsem(struct rw_semaphore *sem));
 extern void FASTCALL(__down_read(struct rw_semaphore *sem));
+extern int FASTCALL(__down_read_trylock(struct rw_semaphore *sem));
 extern void FASTCALL(__down_write(struct rw_semaphore *sem));
+extern int FASTCALL(__down_write_trylock(struct rw_semaphore *sem));
 extern void FASTCALL(__up_read(struct rw_semaphore *sem));
 extern void FASTCALL(__up_write(struct rw_semaphore *sem));
+extern void FASTCALL(__downgrade_write(struct rw_semaphore *sem));
 
 #endif /* __KERNEL__ */
 #endif /* _LINUX_RWSEM_SPINLOCK_H */
Index: include/linux/rwsem.h
===================================================================
RCS file: /cvs/linux-2.4-xfs/linux/include/linux/rwsem.h,v
retrieving revision 1.2
diff -u -p -r1.2 rwsem.h
--- include/linux/rwsem.h	2001/05/29 19:53:13	1.2
+++ include/linux/rwsem.h	2002/07/28 10:07:37
@@ -46,6 +46,18 @@ static inline void down_read(struct rw_s
 }
 
 /*
+ * trylock for reading -- returns 1 if successful, 0 if contention
+ */
+static inline int down_read_trylock(struct rw_semaphore *sem)
+{
+	int ret;
+	rwsemtrace(sem,"Entering down_read_trylock");
+	ret = __down_read_trylock(sem);
+	rwsemtrace(sem,"Leaving down_read_trylock");
+	return ret;
+}
+
+/*
  * lock for writing
  */
 static inline void down_write(struct rw_semaphore *sem)
@@ -56,6 +68,18 @@ static inline void down_write(struct rw_
 }
 
 /*
+ * trylock for writing -- returns 1 if successful, 0 if contention
+ */
+static inline int down_write_trylock(struct rw_semaphore *sem)
+{
+	int ret;
+	rwsemtrace(sem,"Entering down_write_trylock");
+	ret = __down_write_trylock(sem);
+	rwsemtrace(sem,"Leaving down_write_trylock");
+	return ret;
+}
+
+/*
  * release a read lock
  */
 static inline void up_read(struct rw_semaphore *sem)
@@ -73,6 +97,16 @@ static inline void up_write(struct rw_se
 	rwsemtrace(sem,"Entering up_write");
 	__up_write(sem);
 	rwsemtrace(sem,"Leaving up_write");
+}
+
+/*
+ * downgrade write lock to read lock
+ */
+static inline void downgrade_write(struct rw_semaphore *sem)
+{
+	rwsemtrace(sem,"Entering downgrade_write");
+	__downgrade_write(sem);
+	rwsemtrace(sem,"Leaving downgrade_write");
 }
 
 
Index: lib/rwsem-spinlock.c
===================================================================
RCS file: /cvs/linux-2.4-xfs/linux/lib/rwsem-spinlock.c,v
retrieving revision 1.1
diff -u -p -r1.1 rwsem-spinlock.c
--- lib/rwsem-spinlock.c	2001/05/02 06:22:13	1.1
+++ lib/rwsem-spinlock.c	2002/07/28 10:07:37
@@ -46,8 +46,9 @@ void init_rwsem(struct rw_semaphore *sem
  *   - the 'waiting count' is non-zero
  * - the spinlock must be held by the caller
  * - woken process blocks are discarded from the list after having flags zeroised
+ * - writers are only woken if wakewrite is non-zero
  */
-static inline struct rw_semaphore *__rwsem_do_wake(struct rw_semaphore *sem)
+static inline struct rw_semaphore *__rwsem_do_wake(struct rw_semaphore *sem, int wakewrite)
 {
 	struct rwsem_waiter *waiter;
 	int woken;
@@ -56,7 +57,14 @@ static inline struct rw_semaphore *__rws
 
 	waiter = list_entry(sem->wait_list.next,struct rwsem_waiter,list);
 
-	/* try to grant a single write lock if there's a writer at the front of the queue
+	if (!wakewrite) {
+		if (waiter->flags & RWSEM_WAITING_FOR_WRITE)
+			goto out;
+		goto dont_wake_writers;
+	}
+
+	/* if we are allowed to wake writers try to grant a single write lock if there's a
+	 * writer at the front of the queue
 	 * - we leave the 'waiting count' incremented to signify potential contention
 	 */
 	if (waiter->flags & RWSEM_WAITING_FOR_WRITE) {
@@ -68,16 +76,19 @@ static inline struct rw_semaphore *__rws
 	}
 
 	/* grant an infinite number of read locks to the readers at the front of the queue */
+ dont_wake_writers:
 	woken = 0;
-	do {
+	while (waiter->flags&RWSEM_WAITING_FOR_READ) {
+		struct list_head *next = waiter->list.next;
+
 		list_del(&waiter->list);
 		waiter->flags = 0;
 		wake_up_process(waiter->task);
 		woken++;
 		if (list_empty(&sem->wait_list))
 			break;
-		waiter = list_entry(sem->wait_list.next,struct rwsem_waiter,list);
-	} while (waiter->flags&RWSEM_WAITING_FOR_READ);
+		waiter = list_entry(next,struct rwsem_waiter,list);
+	}
 
 	sem->activity += woken;
 
@@ -149,6 +160,28 @@ void __down_read(struct rw_semaphore *se
 }
 
 /*
+ * trylock for reading -- returns 1 if successful, 0 if contention
+ */
+int __down_read_trylock(struct rw_semaphore *sem)
+{
+	int ret = 0;
+	rwsemtrace(sem,"Entering __down_read_trylock");
+
+	spin_lock(&sem->wait_lock);
+
+	if (sem->activity>=0 && list_empty(&sem->wait_list)) {
+		/* granted */
+		sem->activity++;
+		ret = 1;
+	}
+
+	spin_unlock(&sem->wait_lock);
+
+	rwsemtrace(sem,"Leaving __down_read_trylock");
+	return ret;
+}
+
+/*
  * get a write lock on the semaphore
  * - note that we increment the waiting count anyway to indicate an exclusive lock
  */
@@ -195,6 +228,28 @@ void __down_write(struct rw_semaphore *s
 }
 
 /*
+ * trylock for writing -- returns 1 if successful, 0 if contention
+ */
+int __down_write_trylock(struct rw_semaphore *sem)
+{
+	int ret = 0;
+	rwsemtrace(sem,"Entering __down_write_trylock");
+
+	spin_lock(&sem->wait_lock);
+
+	if (sem->activity==0 && list_empty(&sem->wait_list)) {
+		/* granted */
+		sem->activity = -1;
+		ret = 1;
+	}
+
+	spin_unlock(&sem->wait_lock);
+
+	rwsemtrace(sem,"Leaving __down_write_trylock");
+	return ret;
+}
+
+/*
  * release a read lock on the semaphore
  */
 void __up_read(struct rw_semaphore *sem)
@@ -222,18 +277,40 @@ void __up_write(struct rw_semaphore *sem
 
 	sem->activity = 0;
 	if (!list_empty(&sem->wait_list))
-		sem = __rwsem_do_wake(sem);
+		sem = __rwsem_do_wake(sem, 1);
 
 	spin_unlock(&sem->wait_lock);
 
 	rwsemtrace(sem,"Leaving __up_write");
 }
 
+/*
+ * downgrade a write lock into a read lock
+ * - just wake up any readers at the front of the queue
+ */
+void __downgrade_write(struct rw_semaphore *sem)
+{
+	rwsemtrace(sem,"Entering __rwsem_downgrade");
+
+	spin_lock(&sem->wait_lock);
+
+	sem->activity = 1;
+	if (!list_empty(&sem->wait_list))
+		sem = __rwsem_do_wake(sem,0);
+
+	spin_unlock(&sem->wait_lock);
+
+	rwsemtrace(sem,"Leaving __rwsem_downgrade");
+}
+
 EXPORT_SYMBOL(init_rwsem);
 EXPORT_SYMBOL(__down_read);
+EXPORT_SYMBOL(__down_read_trylock);
 EXPORT_SYMBOL(__down_write);
+EXPORT_SYMBOL(__down_write_trylock);
 EXPORT_SYMBOL(__up_read);
 EXPORT_SYMBOL(__up_write);
+EXPORT_SYMBOL(__downgrade_write);
 #if RWSEM_DEBUG
 EXPORT_SYMBOL(rwsemtrace);
 #endif
Index: lib/rwsem.c
===================================================================
RCS file: /cvs/linux-2.4-xfs/linux/lib/rwsem.c,v
retrieving revision 1.2
diff -u -p -r1.2 rwsem.c
--- lib/rwsem.c	2001/07/11 17:53:24	1.2
+++ lib/rwsem.c	2002/07/28 10:07:37
@@ -34,8 +34,9 @@ void rwsemtrace(struct rw_semaphore *sem
  *   - there must be someone on the queue
  * - the spinlock must be held by the caller
  * - woken process blocks are discarded from the list after having flags zeroised
+ * - writers are only woken if wakewrite is non-zero
  */
-static inline struct rw_semaphore *__rwsem_do_wake(struct rw_semaphore *sem)
+static inline struct rw_semaphore *__rwsem_do_wake(struct rw_semaphore *sem, int wakewrite)
 {
 	struct rwsem_waiter *waiter;
 	struct list_head *next;
@@ -44,6 +45,9 @@ static inline struct rw_semaphore *__rws
 
 	rwsemtrace(sem,"Entering __rwsem_do_wake");
 
+	if (!wakewrite)
+		goto dont_wake_writers;
+
 	/* only wake someone up if we can transition the active part of the count from 0 -> 1 */
  try_again:
 	oldcount = rwsem_atomic_update(RWSEM_ACTIVE_BIAS,sem) - RWSEM_ACTIVE_BIAS;
@@ -64,6 +68,12 @@ static inline struct rw_semaphore *__rws
 	wake_up_process(waiter->task);
 	goto out;
 
+	/* don't want to wake any writers */
+ dont_wake_writers:
+	waiter = list_entry(sem->wait_list.next,struct rwsem_waiter,list);
+	if (waiter->flags & RWSEM_WAITING_FOR_WRITE)
+		goto out;
+
 	/* grant an infinite number of read locks to the readers at the front of the queue
 	 * - note we increment the 'active part' of the count by the number of readers (less one
 	 *   for the activity decrement we've already done) before waking any processes up
@@ -132,7 +142,7 @@ static inline struct rw_semaphore *rwsem
 	 * - it might even be this process, since the waker takes a more active part
 	 */
 	if (!(count & RWSEM_ACTIVE_MASK))
-		sem = __rwsem_do_wake(sem);
+		sem = __rwsem_do_wake(sem,1);
 
 	spin_unlock(&sem->wait_lock);
 
@@ -193,7 +203,7 @@ struct rw_semaphore *rwsem_wake(struct r
 
 	/* do nothing if list empty */
 	if (!list_empty(&sem->wait_list))
-		sem = __rwsem_do_wake(sem);
+		sem = __rwsem_do_wake(sem,1);
 
 	spin_unlock(&sem->wait_lock);
 
@@ -202,9 +212,31 @@ struct rw_semaphore *rwsem_wake(struct r
 	return sem;
 }
 
+/*
+ * downgrade a write lock into a read lock
+ * - caller incremented waiting part of count, and discovered it to be still negative
+ * - just wake up any readers at the front of the queue
+ */
+struct rw_semaphore *rwsem_downgrade_wake(struct rw_semaphore *sem)
+{
+	rwsemtrace(sem,"Entering rwsem_downgrade_wake");
+
+	spin_lock(&sem->wait_lock);
+
+	/* do nothing if list empty */
+	if (!list_empty(&sem->wait_list))
+		sem = __rwsem_do_wake(sem,0);
+
+	spin_unlock(&sem->wait_lock);
+
+	rwsemtrace(sem,"Leaving rwsem_downgrade_wake");
+	return sem;
+}
+
 EXPORT_SYMBOL_NOVERS(rwsem_down_read_failed);
 EXPORT_SYMBOL_NOVERS(rwsem_down_write_failed);
 EXPORT_SYMBOL_NOVERS(rwsem_wake);
+EXPORT_SYMBOL_NOVERS(rwsem_downgrade_wake);
 #if RWSEM_DEBUG
 EXPORT_SYMBOL(rwsemtrace);
 #endif

Re: Patch: linux-2.5.29 __downgrade_write() for CONFIG_RWSEM_GENERIC_SPINLOCK

[Roman Zippel]

Hi,

On Sun, 28 Jul 2002, Christoph Hellwig wrote:

> -static inline struct rw_semaphore *__rwsem_do_wake(struct rw_semaphore *sem)
> +static inline struct rw_semaphore *__rwsem_do_wake(struct rw_semaphore *sem, int wakewrite)
>  {
>  	struct rwsem_waiter *waiter;
>  	int woken;
> @@ -56,7 +57,14 @@ static inline struct rw_semaphore *__rws
>
>  	waiter = list_entry(sem->wait_list.next,struct rwsem_waiter,list);
>
> -	/* try to grant a single write lock if there's a writer at the front of the queue
> +	if (!wakewrite) {
> +		if (waiter->flags & RWSEM_WAITING_FOR_WRITE)
> +			goto out;
> +		goto dont_wake_writers;
> +	}
> +
> +	/* if we are allowed to wake writers try to grant a single write lock if there's a
> +	 * writer at the front of the queue
>  	 * - we leave the 'waiting count' incremented to signify potential contention
>  	 */
>  	if (waiter->flags & RWSEM_WAITING_FOR_WRITE) {

You don't really need that extra argument, testing sem->activity should do
the same job.
If you exchange the wakewrite (or sem->activity) test and the
waiter->flags you can fold it into the next test (this means all the extra
work would only be done, if we have a writer waiting at the top).

bye, Roman

Re: Patch: linux-2.5.29 __downgrade_write() for CONFIG_RWSEM_GENERIC_SPINLOCK

[David Howells]

Roman Zippel <zippel@linux-m68k.org> wrote:
> You don't really need that extra argument, testing sem->activity should do
> the same job.
> If you exchange the wakewrite (or sem->activity) test and the
> waiter->flags you can fold it into the next test (this means all the extra
> work would only be done, if we have a writer waiting at the top).

The reason for doing it this way is that it allows the compiler to discard
parts of the function when inlining it since the value is set at compile time
rather than being worked out at runtime. The value itself should be
disappeared entirely by the compiler.

David

Re: Patch: linux-2.5.29 __downgrade_write() for CONFIG_RWSEM_GENERIC_SPINLOCK

[Roman Zippel]

Hi,

On Mon, 29 Jul 2002, David Howells wrote:

> > You don't really need that extra argument, testing sem->activity should do
> > the same job.
> > If you exchange the wakewrite (or sem->activity) test and the
> > waiter->flags you can fold it into the next test (this means all the extra
> > work would only be done, if we have a writer waiting at the top).
>
> The reason for doing it this way is that it allows the compiler to discard
> parts of the function when inlining it since the value is set at compile time
> rather than being worked out at runtime. The value itself should be
> disappeared entirely by the compiler.

Did you look at the code? gcc should be able to optimize that itself.

bye, Roman

Re: Patch: linux-2.5.29 __downgrade_write() for CONFIG_RWSEM_GENERIC_SPINLOCK

[David Howells]

Roman Zippel <zippel@linux-m68k.org> wrote:
> Did you look at the code? gcc should be able to optimize that itself.

Maybe... gcc should also optimise my version to the same extent, I think (the
result of one of the additional tests is known at compile time, and the other
one is the same as the next test down). What I'm unsure about is how gcc will
handle the variable being stored in memory not marked volatile and then
retrieved again; whether it'll actually issue a read, or just assume it's got
it cached.

However, I've changed it to your suggestion, and I'm compiling it to have a
look. I've attached the changed C file for your perusal.

David


static inline struct rw_semaphore *__rwsem_do_wake(struct rw_semaphore *sem)
{
	struct rwsem_waiter *waiter;
	int woken;

	rwsemtrace(sem,"Entering __rwsem_do_wake");

	waiter = list_entry(sem->wait_list.next,struct rwsem_waiter,list);

	/* if we are allowed to wake writers try to grant a single write lock if there's a
	 * writer at the front of the queue
	 * - we leave the 'waiting count' incremented to signify potential contention
	 */
	if (waiter->flags & RWSEM_WAITING_FOR_WRITE) {
		if (!sem->activity) {
			sem->activity = -1;
			list_del(&waiter->list);
			waiter->flags = 0;
			wake_up_process(waiter->task);
		}
		goto out;
	}

	/* grant an infinite number of read locks to the readers at the front of the queue */
	woken = 0;
	while (waiter->flags&RWSEM_WAITING_FOR_READ) {
		struct list_head *next = waiter->list.next;

		list_del(&waiter->list);
		waiter->flags = 0;
		wake_up_process(waiter->task);
		woken++;
		if (list_empty(&sem->wait_list))
			break;
		waiter = list_entry(next,struct rwsem_waiter,list);
	}

	sem->activity += woken;

 out:
	rwsemtrace(sem,"Leaving __rwsem_do_wake");
	return sem;
}

Re: Patch: linux-2.5.29 __downgrade_write() for CONFIG_RWSEM_GENERIC_SPINLOCK

[David Howells]

> Roman Zippel <zippel@linux-m68k.org> wrote:
> > Did you look at the code? gcc should be able to optimize that itself.
> 
> Maybe... gcc should also optimise my version to the same extent, I think
> (the result of one of the additional tests is known at compile time, and the
> other one is the same as the next test down). What I'm unsure about is how
> gcc will handle the variable being stored in memory not marked volatile and
> then retrieved again; whether it'll actually issue a read, or just assume
> it's got it cached.

It doesn't appear to make any difference which way it is done. The i386 code
from both looks the same.

David

Re: Patch: linux-2.5.29 __downgrade_write() for CONFIG_RWSEM_GENERIC_SPINLOCK

[Roman Zippel]

Hi,

On Mon, 29 Jul 2002, David Howells wrote:

> It doesn't appear to make any difference which way it is done. The i386 code
> from both looks the same.

Then I vote for the simpler version. :)
BTW even if gcc had problems optimizing that, I'd rather make it explicit,
that the two variables contain the same information:

	activity = sem->activity = 0;
	if (!list_empty(&sem->wait_list))
		sem = __rwsem_do_wake(sem, activity);

IMO that's more readable and will still work if gcc had to flush the
cached information before using it.

bye, Roman

Re: Patch: linux-2.5.29 __downgrade_write() for CONFIG_RWSEM_GENERIC_SPINLOCK

[David Howells]

Roman Zippel <zippel@linux-m68k.org>
> > It doesn't appear to make any difference which way it is done. The i386
> > code from both looks the same.
> 
> Then I vote for the simpler version. :)

Maybe... I've got a patch for that variation too.

> BTW even if gcc had problems optimizing that, I'd rather make it explicit,
> that the two variables contain the same information:
> 
> 	activity = sem->activity = 0;
> 	if (!list_empty(&sem->wait_list))
> 		sem = __rwsem_do_wake(sem, activity);
> 
> IMO that's more readable and will still work if gcc had to flush the
> cached information before using it.

Brrr... I don't like that. If I'm going to pass in a second argument, then I
want it to be what Christoph's version because it's more readable and more
obvious what it's doing (and, since the value is constant, the optimiser can
obviously get rid of it easily). I don't think gcc is going to be a problem in
that respect since the activity member is not volatile.

David

Re: Patch: linux-2.5.29 __downgrade_write() for CONFIG_RWSEM_GENERIC_SPINLOCK

[Roman Zippel]

Hi,

On Mon, 29 Jul 2002, David Howells wrote:

> Brrr... I don't like that. If I'm going to pass in a second argument, then I
> want it to be what Christoph's version because it's more readable and more
> obvious what it's doing (and, since the value is constant, the optimiser can
> obviously get rid of it easily).

If the intention was to help the gcc optimizing the code, that was not
readable from the old version. It also wasn't that clear that wakewrite
and sem->activity basically have the same information (only if one reads
the code carefully one sees that "wakewrite" actually means "only if
there's no activity, wake a writer").

bye, Roman