[PATCH] RWSEM: Rewrite rwsem.c and rwsem-spinlock.c more simply.

[PATCH] RWSEM: Rewrite rwsem.c and rwsem-spinlock.c more simply.

[Robert P. J. Day]

Rewrite these source files more simply by deleting the superfluous
"tsk" task_struct pointer and rephrasing in terms of the "current"
task pointer.

Signed-off-by: Robert P. J. Day <rpjday@crashcourse.ca>

---

  in addition to making the code slightly simpler, this makes it
easier to read since there's more information in:

  set_current_state(TASK_UNINTERRUPTIBLE);

as opposed to:

  set_task_state(tsk, TASK_UNINTERRUPTIBLE);

if it's not clear what "tsk" refers to.

  compile tested on x86 with "make defconfig", but that doesn't really
mean much since that configuration doesn't even compile the second of
those source files.  so i'll leave it to someone who's a locking
expert to sanity check this.  i'm quite willing to be told i screwed
something up.


 lib/rwsem-spinlock.c |   24 ++++++++++--------------
 lib/rwsem.c          |   11 +++++------
 2 files changed, 15 insertions(+), 20 deletions(-)

diff --git a/lib/rwsem-spinlock.c b/lib/rwsem-spinlock.c
index 9df3ca5..8c3fc63 100644
--- a/lib/rwsem-spinlock.c
+++ b/lib/rwsem-spinlock.c
@@ -128,7 +128,6 @@ __rwsem_wake_one_writer(struct rw_semaphore *sem)
 void __sched __down_read(struct rw_semaphore *sem)
 {
 	struct rwsem_waiter waiter;
-	struct task_struct *tsk;

 	spin_lock_irq(&sem->wait_lock);

@@ -139,13 +138,12 @@ void __sched __down_read(struct rw_semaphore *sem)
 		goto out;
 	}

-	tsk = current;
-	set_task_state(tsk, TASK_UNINTERRUPTIBLE);
+	set_current_state(TASK_UNINTERRUPTIBLE);

 	/* set up my own style of waitqueue */
-	waiter.task = tsk;
+	waiter.task = current;
 	waiter.flags = RWSEM_WAITING_FOR_READ;
-	get_task_struct(tsk);
+	get_task_struct(current);

 	list_add_tail(&waiter.list, &sem->wait_list);

@@ -157,10 +155,10 @@ void __sched __down_read(struct rw_semaphore *sem)
 		if (!waiter.task)
 			break;
 		schedule();
-		set_task_state(tsk, TASK_UNINTERRUPTIBLE);
+		set_current_state(TASK_UNINTERRUPTIBLE);
 	}

-	tsk->state = TASK_RUNNING;
+	set_current_state(TASK_RUNNING);
  out:
 	;
 }
@@ -194,7 +192,6 @@ int __down_read_trylock(struct rw_semaphore *sem)
 void __sched __down_write_nested(struct rw_semaphore *sem, int subclass)
 {
 	struct rwsem_waiter waiter;
-	struct task_struct *tsk;

 	spin_lock_irq(&sem->wait_lock);

@@ -205,13 +202,12 @@ void __sched __down_write_nested(struct rw_semaphore *sem, int subclass)
 		goto out;
 	}

-	tsk = current;
-	set_task_state(tsk, TASK_UNINTERRUPTIBLE);
+	set_current_state(TASK_UNINTERRUPTIBLE);

 	/* set up my own style of waitqueue */
-	waiter.task = tsk;
+	waiter.task = current;
 	waiter.flags = RWSEM_WAITING_FOR_WRITE;
-	get_task_struct(tsk);
+	get_task_struct(current);

 	list_add_tail(&waiter.list, &sem->wait_list);

@@ -223,10 +219,10 @@ void __sched __down_write_nested(struct rw_semaphore *sem, int subclass)
 		if (!waiter.task)
 			break;
 		schedule();
-		set_task_state(tsk, TASK_UNINTERRUPTIBLE);
+		set_current_state(TASK_UNINTERRUPTIBLE);
 	}

-	tsk->state = TASK_RUNNING;
+	set_current_state(TASK_RUNNING);
  out:
 	;
 }
diff --git a/lib/rwsem.c b/lib/rwsem.c
index 3e3365e..f1bdb97 100644
--- a/lib/rwsem.c
+++ b/lib/rwsem.c
@@ -150,15 +150,14 @@ static struct rw_semaphore __sched *
 rwsem_down_failed_common(struct rw_semaphore *sem,
 			struct rwsem_waiter *waiter, signed long adjustment)
 {
-	struct task_struct *tsk = current;
 	signed long count;

-	set_task_state(tsk, TASK_UNINTERRUPTIBLE);
+	set_current_state(TASK_UNINTERRUPTIBLE);

 	/* set up my own style of waitqueue */
 	spin_lock_irq(&sem->wait_lock);
-	waiter->task = tsk;
-	get_task_struct(tsk);
+	waiter->task = current;
+	get_task_struct(current);

 	list_add_tail(&waiter->list, &sem->wait_list);

@@ -176,10 +175,10 @@ rwsem_down_failed_common(struct rw_semaphore *sem,
 		if (!waiter->task)
 			break;
 		schedule();
-		set_task_state(tsk, TASK_UNINTERRUPTIBLE);
+		set_current_state(TASK_UNINTERRUPTIBLE);
 	}

-	tsk->state = TASK_RUNNING;
+	set_current_state(TASK_RUNNING);

 	return sem;
 }
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry:
    Have classroom, will lecture.

http://crashcourse.ca                          Waterloo, Ontario, CANADA
========================================================================

Re: [PATCH] RWSEM: Rewrite rwsem.c and rwsem-spinlock.c more simply.

[Andi Kleen]

"Robert P. J. Day" <rpjday@crashcourse.ca> writes:

> Rewrite these source files more simply by deleting the superfluous
> "tsk" task_struct pointer and rephrasing in terms of the "current"
> task pointer.

This is likely a code pessimization because "current" is inline
assembler and many gcc versions cannot CSE it.

-Andi

Re: [PATCH] RWSEM: Rewrite rwsem.c and rwsem-spinlock.c more simply.

[Robert P. J. Day]

On Tue, 25 Mar 2008, Andi Kleen wrote:

> "Robert P. J. Day" <rpjday@crashcourse.ca> writes:
>
> > Rewrite these source files more simply by deleting the superfluous
> > "tsk" task_struct pointer and rephrasing in terms of the "current"
> > task pointer.
>
> This is likely a code pessimization because "current" is inline
> assembler and many gcc versions cannot CSE it.

i'm not sure what this means -- which of the transformations in that
patch is considered unsafe?  here's a typical simplification:

-       tsk = current;
-       set_task_state(tsk, TASK_UNINTERRUPTIBLE);
+       set_current_state(TASK_UNINTERRUPTIBLE);


  there's all sorts of usage of set_current_state() throughout the
tree.  how is simplifying the code in these two files in exactly the
same way any different?  or am i missing something because this
involves semaphores?

rday

p.s.  given this bit from sched.h:

...
#define __set_task_state(tsk, state_value)              \
        do { (tsk)->state = (state_value); } while (0)
#define set_task_state(tsk, state_value)                \
        set_mb((tsk)->state, (state_value))

...

#define __set_current_state(state_value)                        \
        do { current->state = (state_value); } while (0)
#define set_current_state(state_value)          \
        set_mb(current->state, (state_value))
...

it's not clear why set_current_state() and __current_state() are
defined the way they are when it would seem to be simpler (and less
error-prone) to just write:

#define __set_current_state(sv)  __set_task_state(current, sv)
#define set_current_state(sv)   set_task_state(current, sv)

  the law of parsimony and all that.  or, once again, is there
something subtle i'm not seeing?

--


========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry:
    Have classroom, will lecture.

http://crashcourse.ca                          Waterloo, Ontario, CANADA
========================================================================

Re: [PATCH] RWSEM: Rewrite rwsem.c and rwsem-spinlock.c more simply.

[Andi Kleen]

> i'm not sure what this means -- which of the transformations in that
> patch is considered unsafe?  here's a typical simplification:

It is not unsafe, just generates slight worse code.

current is inline assembler and the compiler doesn't know that it could
cache it in a register because it is not marked pure 
for various reasons. That is why current is often cached explicitely
in a local variable to tell the compiler that.

Before you run off and do that everywhere: it is also not a large
win, just a small one unless current is used very often.

-Andi

Re: [PATCH] RWSEM: Rewrite rwsem.c and rwsem-spinlock.c more simply.

[Robert P. J. Day]

On Tue, 25 Mar 2008, Andi Kleen wrote:

> > i'm not sure what this means -- which of the transformations in
> > that patch is considered unsafe?  here's a typical simplification:
>
> It is not unsafe, just generates slight worse code.
>
> current is inline assembler and the compiler doesn't know that it
> could cache it in a register because it is not marked pure for
> various reasons. That is why current is often cached explicitely in
> a local variable to tell the compiler that.

  ah, i think i see, thanks.  learn something every day.

> Before you run off and do that everywhere: it is also not a large
> win, just a small one unless current is used very often.

  there's actually not that many explicit calls to either
set_task_state or __set_task_state in the entire tree, and a lot of
those don't count as they really are setting the state for a different
task or for some other reason.  in fact, here's the entire list for
the whole tree:

$ grep -r set_task_state *
arch/powerpc/kernel/semaphore.c:	__set_task_state(tsk, TASK_UNINTERRUPTIBLE);
arch/powerpc/kernel/semaphore.c:		set_task_state(tsk, TASK_UNINTERRUPTIBLE);
arch/powerpc/kernel/semaphore.c:	__set_task_state(tsk, TASK_RUNNING);
arch/powerpc/kernel/semaphore.c:	__set_task_state(tsk, TASK_INTERRUPTIBLE);
arch/powerpc/kernel/semaphore.c:		set_task_state(tsk, TASK_INTERRUPTIBLE);
arch/powerpc/kernel/semaphore.c:	__set_task_state(tsk, TASK_RUNNING);
arch/alpha/kernel/semaphore.c:		set_task_state(tsk, TASK_UNINTERRUPTIBLE);
arch/alpha/kernel/semaphore.c:		set_task_state(tsk, TASK_INTERRUPTIBLE);
arch/parisc/kernel/semaphore.c:		set_task_state(current, TASK_UNINTERRUPTIBLE);
arch/parisc/kernel/semaphore.c:		set_task_state(current, TASK_INTERRUPTIBLE);
arch/mn10300/kernel/semaphore.c:	set_task_state(tsk, TASK_UNINTERRUPTIBLE);
arch/mn10300/kernel/semaphore.c:		set_task_state(tsk, TASK_UNINTERRUPTIBLE);
arch/mn10300/kernel/semaphore.c:	set_task_state(tsk, TASK_INTERRUPTIBLE);
arch/mn10300/kernel/semaphore.c:		set_task_state(tsk, TASK_INTERRUPTIBLE);
arch/s390/kernel/semaphore.c:	__set_task_state(tsk, TASK_UNINTERRUPTIBLE);
arch/s390/kernel/semaphore.c:		set_task_state(tsk, TASK_UNINTERRUPTIBLE);
arch/s390/kernel/semaphore.c:	__set_task_state(tsk, TASK_RUNNING);
arch/s390/kernel/semaphore.c:	__set_task_state(tsk, TASK_INTERRUPTIBLE);
arch/s390/kernel/semaphore.c:		set_task_state(tsk, TASK_INTERRUPTIBLE);
arch/s390/kernel/semaphore.c:	__set_task_state(tsk, TASK_RUNNING);
arch/s390/mm/fault.c:		set_task_state(tsk, TASK_UNINTERRUPTIBLE);
arch/s390/mm/fault.c:			set_task_state(tsk, TASK_RUNNING);
arch/frv/kernel/semaphore.c:	set_task_state(tsk, TASK_UNINTERRUPTIBLE);
arch/frv/kernel/semaphore.c:		set_task_state(tsk, TASK_UNINTERRUPTIBLE);
arch/frv/kernel/semaphore.c:	set_task_state(tsk, TASK_INTERRUPTIBLE);
arch/frv/kernel/semaphore.c:		set_task_state(tsk, TASK_INTERRUPTIBLE);
arch/mips/kernel/semaphore.c:	__set_task_state(tsk, TASK_UNINTERRUPTIBLE);
arch/mips/kernel/semaphore.c:		set_task_state(tsk, TASK_UNINTERRUPTIBLE);
arch/mips/kernel/semaphore.c:	__set_task_state(tsk, TASK_RUNNING);
arch/mips/kernel/semaphore.c:	__set_task_state(tsk, TASK_INTERRUPTIBLE);
arch/mips/kernel/semaphore.c:		set_task_state(tsk, TASK_INTERRUPTIBLE);
arch/mips/kernel/semaphore.c:	__set_task_state(tsk, TASK_RUNNING);
Documentation/scheduler/sched-coding.txt:set_task_state(tsk, state_value)
drivers/mmc/core/sdio_irq.c:		set_task_state(current, TASK_INTERRUPTIBLE);
drivers/mmc/core/sdio_irq.c:		set_task_state(current, TASK_RUNNING);
drivers/mfd/ucb1x00-ts.c:			set_task_state(tsk, TASK_INTERRUPTIBLE);
drivers/mfd/ucb1x00-ts.c:			set_task_state(tsk, TASK_INTERRUPTIBLE);
fs/aio.c:	set_task_state(tsk, TASK_UNINTERRUPTIBLE);
fs/aio.c:		set_task_state(tsk, TASK_UNINTERRUPTIBLE);
fs/aio.c:	__set_task_state(tsk, TASK_RUNNING);
fs/aio.c:			set_task_state(tsk, TASK_INTERRUPTIBLE);
fs/aio.c:		set_task_state(tsk, TASK_RUNNING);
include/linux/sched.h:/* Convenience macros for the sake of set_task_state */
include/linux/sched.h:#define __set_task_state(tsk, state_value)		\
include/linux/sched.h:#define set_task_state(tsk, state_value)		\
kernel/ptrace.c:			__set_task_state(child, TASK_STOPPED);
kernel/fork.c:			__set_task_state(p, TASK_STOPPED);
kernel/mutex.c:		__set_task_state(task, state);
lib/rwsem-spinlock.c:	set_task_state(tsk, TASK_UNINTERRUPTIBLE);
lib/rwsem-spinlock.c:		set_task_state(tsk, TASK_UNINTERRUPTIBLE);
lib/rwsem-spinlock.c:	set_task_state(tsk, TASK_UNINTERRUPTIBLE);
lib/rwsem-spinlock.c:		set_task_state(tsk, TASK_UNINTERRUPTIBLE);
lib/rwsem.c:	set_task_state(tsk, TASK_UNINTERRUPTIBLE);
lib/rwsem.c:		set_task_state(tsk, TASK_UNINTERRUPTIBLE);

  once you ignore the semaphore stuff (and a bit of the rest, like the
stuff in sched.h), there's not really that much left that could be
rewritten with set_current_state() anyway.

rday
--

========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry:
    Have classroom, will lecture.

http://crashcourse.ca                          Waterloo, Ontario, CANADA
========================================================================

Re: [PATCH] RWSEM: Rewrite rwsem.c and rwsem-spinlock.c more simply.

[Andrew Morton]

On Tue, 25 Mar 2008 08:42:48 -0400 (EDT)
"Robert P. J. Day" <rpjday@crashcourse.ca> wrote:

> On Tue, 25 Mar 2008, Andi Kleen wrote:
> 
> > > i'm not sure what this means -- which of the transformations in
> > > that patch is considered unsafe?  here's a typical simplification:
> >
> > It is not unsafe, just generates slight worse code.
> >
> > current is inline assembler and the compiler doesn't know that it
> > could cache it in a register because it is not marked pure for
> > various reasons. That is why current is often cached explicitely in
> > a local variable to tell the compiler that.
> 
>   ah, i think i see, thanks.  learn something every day.

A crude measyure is /usr/bin/size.  Your patch increased rwsem-spinlock.o
from 1595 bytes of text up to 1629.

A text size increase isn't necessarily always a bad thing, but it does need
to be monitored, understood, explained, etc.