Re: [patch] speed up / fix the new generic semaphore code (fixAIM7 40% regression with 2.6.26-rc1)

From: Ingo Molnar
Date: Thu May 08 2008 - 10:44:07 EST



* Ingo Molnar <mingo@xxxxxxx> wrote:

> Peter pointed it out that because sem->count is u32, the <= 0 is in
> fact a "== 0" condition - the patch below does that. As expected gcc
> figured out the same thing too so the resulting code output did not
> change. (so this is just a cleanup)

a second update patch, i've further simplified the semaphore wakeup
logic: there's no need for the wakeup to remove the task from the wait
list. This will make them a slighly bit more fair, but more importantly,
this closes a race in my first patch for the unlikely case of a signal
(or a timeout) and an unlock coming in at the same time and the task not
getting removed from the wait-list.

( my performance testing with 2000 AIM7 tasks on a quad never hit that
race but x86.git QA actually triggered it after about 30 random kernel
bootups and it caused a nasty crash and lockup. )

Ingo

---------------->
Subject: sem: simplify queue management
From: Ingo Molnar <mingo@xxxxxxx>
Date: Tue May 06 19:32:42 CEST 2008

kernel/semaphore.o:

text data bss dec hex filename
1040 0 0 1040 410 semaphore.o.before
975 0 0 975 3cf semaphore.o.after

Signed-off-by: Ingo Molnar <mingo@xxxxxxx>
---
kernel/semaphore.c | 32 ++++++++++++++++----------------
1 file changed, 16 insertions(+), 16 deletions(-)

Index: linux/kernel/semaphore.c
===================================================================
--- linux.orig/kernel/semaphore.c
+++ linux/kernel/semaphore.c
@@ -202,33 +202,34 @@ static inline int __sched __down_common(
{
struct task_struct *task = current;
struct semaphore_waiter waiter;
+ int ret = 0;

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

for (;;) {
- list_add_tail(&waiter.list, &sem->wait_list);
-
- if (state == TASK_INTERRUPTIBLE && signal_pending(task))
- goto interrupted;
- if (state == TASK_KILLABLE && fatal_signal_pending(task))
- goto interrupted;
- if (timeout <= 0)
- goto timed_out;
+ if (state == TASK_INTERRUPTIBLE && signal_pending(task)) {
+ ret = -EINTR;
+ break;
+ }
+ if (state == TASK_KILLABLE && fatal_signal_pending(task)) {
+ ret = -EINTR;
+ break;
+ }
+ if (timeout <= 0) {
+ ret = -ETIME;
+ break;
+ }
__set_task_state(task, state);
spin_unlock_irq(&sem->lock);
timeout = schedule_timeout(timeout);
spin_lock_irq(&sem->lock);
if (sem->count > 0)
- return 0;
+ break;
}

- timed_out:
- list_del(&waiter.list);
- return -ETIME;
-
- interrupted:
list_del(&waiter.list);
- return -EINTR;
+ return ret;
}

static noinline void __sched __down(struct semaphore *sem)
@@ -255,6 +256,5 @@ static noinline void __sched __up(struct
{
struct semaphore_waiter *waiter = list_first_entry(&sem->wait_list,
struct semaphore_waiter, list);
- list_del(&waiter->list);
wake_up_process(waiter->task);
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/