* [PATCH] RWSEM: Rewrite rwsem.c and rwsem-spinlock.c more simply. @ 2008-03-25 11:02 Robert P. J. Day 2008-03-25 12:08 ` Andi Kleen 0 siblings, 1 reply; 6+ messages in thread From: Robert P. J. Day @ 2008-03-25 11:02 UTC (permalink / raw) To: Linux Kernel Mailing List; +Cc: Andrew Morton 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 ======================================================================== ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] RWSEM: Rewrite rwsem.c and rwsem-spinlock.c more simply. 2008-03-25 11:02 [PATCH] RWSEM: Rewrite rwsem.c and rwsem-spinlock.c more simply Robert P. J. Day @ 2008-03-25 12:08 ` Andi Kleen 2008-03-25 12:27 ` Robert P. J. Day 0 siblings, 1 reply; 6+ messages in thread From: Andi Kleen @ 2008-03-25 12:08 UTC (permalink / raw) To: Robert P. J. Day; +Cc: Linux Kernel Mailing List, Andrew Morton "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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] RWSEM: Rewrite rwsem.c and rwsem-spinlock.c more simply. 2008-03-25 12:08 ` Andi Kleen @ 2008-03-25 12:27 ` Robert P. J. Day 2008-03-25 12:36 ` Andi Kleen 0 siblings, 1 reply; 6+ messages in thread From: Robert P. J. Day @ 2008-03-25 12:27 UTC (permalink / raw) To: Andi Kleen; +Cc: Linux Kernel Mailing List, Andrew Morton 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 ======================================================================== ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] RWSEM: Rewrite rwsem.c and rwsem-spinlock.c more simply. 2008-03-25 12:27 ` Robert P. J. Day @ 2008-03-25 12:36 ` Andi Kleen 2008-03-25 12:42 ` Robert P. J. Day 0 siblings, 1 reply; 6+ messages in thread From: Andi Kleen @ 2008-03-25 12:36 UTC (permalink / raw) To: Robert P. J. Day; +Cc: Andi Kleen, Linux Kernel Mailing List, Andrew Morton > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] RWSEM: Rewrite rwsem.c and rwsem-spinlock.c more simply. 2008-03-25 12:36 ` Andi Kleen @ 2008-03-25 12:42 ` Robert P. J. Day 2008-03-25 19:14 ` Andrew Morton 0 siblings, 1 reply; 6+ messages in thread From: Robert P. J. Day @ 2008-03-25 12:42 UTC (permalink / raw) To: Andi Kleen; +Cc: Linux Kernel Mailing List, Andrew Morton 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 ======================================================================== ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] RWSEM: Rewrite rwsem.c and rwsem-spinlock.c more simply. 2008-03-25 12:42 ` Robert P. J. Day @ 2008-03-25 19:14 ` Andrew Morton 0 siblings, 0 replies; 6+ messages in thread From: Andrew Morton @ 2008-03-25 19:14 UTC (permalink / raw) To: Robert P. J. Day; +Cc: andi, linux-kernel 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2008-03-25 19:15 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-03-25 11:02 [PATCH] RWSEM: Rewrite rwsem.c and rwsem-spinlock.c more simply Robert P. J. Day 2008-03-25 12:08 ` Andi Kleen 2008-03-25 12:27 ` Robert P. J. Day 2008-03-25 12:36 ` Andi Kleen 2008-03-25 12:42 ` Robert P. J. Day 2008-03-25 19:14 ` Andrew Morton
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.