Re: [PATCH][1/2] fix for -mm add-sem_is_read-write_locked.patch

[David Howells <dhowells@redhat.com>]

Rik van Riel <riel@redhat.com> wrote:

> Here is an incremental fix to the add-sem_is_read-write_locked
> patch in -mm.  Also attached is a full version of that file,
> which can just be dropped into place - I've verified that none
> of the patches in your stack get rejects.

The comment attached to the drop-in replacement patch is wrong:

| [1. text/plain; add-sem_is_read-write_locked.patch]   
| 
| From: Rik Van Riel <riel@redhat.com>
| 
| Add sem_is_read/write_locked functions to the read/write semaphores, along the
| same lines of the *_is_locked spinlock functions.  The swap token tuning patch
| uses sem_is_read_locked; sem_is_write_locked is added for completeness.

The function names you've used are incorrect.

Furthermore, the substance of the patch is wrong in a number of ways:

| Index: linux-2.6.13/include/asm-ppc64/rwsem.h
| ===================================================================
| --- linux-2.6.13.orig/include/asm-ppc64/rwsem.h
| +++ linux-2.6.13/include/asm-ppc64/rwsem.h
| @@ -163,5 +163,10 @@ static inline int rwsem_atomic_update(in
|  	return atomic_add_return(delta, (atomic_t *)(&sem->count));
|  }
|  
| +static inline int sem_is_read_locked(struct rw_semaphore *sem)
| +{
| +	return (sem->count != 0);
| +}
| +

This uses the function wrong name. And:

| Index: linux-2.6.13/include/linux/rwsem-spinlock.h
| ===================================================================
| --- linux-2.6.13.orig/include/linux/rwsem-spinlock.h
| +++ linux-2.6.13/include/linux/rwsem-spinlock.h
| @@ -61,5 +61,15 @@ extern void FASTCALL(__up_read(struct rw
|  extern void FASTCALL(__up_write(struct rw_semaphore *sem));
|  extern void FASTCALL(__downgrade_write(struct rw_semaphore *sem));
|  
| +static inline int sem_is_read_locked(struct rw_semaphore *sem)
| +{
| +	return (sem->activity > 0);
| +}
| +
| +static inline int sem_is_write_locked(struct rw_semaphore *sem)
| +{
| +	return (sem->activity < 0);
| +}
| +

Is inconsistent, though the tests are valid.

Also, you don't need to bracket the expression handed to the return directive,
but that's a minor matter.

David