[PATCH 2/2] __down_common: use signal_pending_state()

[PATCH 2/2] __down_common: use signal_pending_state()

[Oleg Nesterov]

Cleanup. __down_common() can use the new signal_pending_state() helper.

Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

--- 26-rc2/kernel/semaphore.c~2_SEM_USE_HELPER	2008-05-18 15:44:19.000000000 +0400
+++ 26-rc2/kernel/semaphore.c	2008-06-04 20:45:21.000000000 +0400
@@ -211,9 +211,7 @@ static inline int __sched __down_common(
 	waiter.up = 0;
 
 	for (;;) {
-		if (state == TASK_INTERRUPTIBLE && signal_pending(task))
-			goto interrupted;
-		if (state == TASK_KILLABLE && fatal_signal_pending(task))
+		if (signal_pending_state(state, task))
 			goto interrupted;
 		if (timeout <= 0)
 			goto timed_out;

Re: [PATCH 2/2] __down_common: use signal_pending_state()

[Matthew Wilcox]

On Wed, Jun 04, 2008 at 09:09:06PM +0400, Oleg Nesterov wrote:
> Cleanup. __down_common() can use the new signal_pending_state() helper.

This is a bad optimisation.  __down_common gets inlined and the constant
'state' versions are optimised away for the versions which don't apply.

NAK this patch.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH 2/2] __down_common: use signal_pending_state()

[Oleg Nesterov]

On 06/04, Matthew Wilcox wrote:
>
> On Wed, Jun 04, 2008 at 09:09:06PM +0400, Oleg Nesterov wrote:
> > Cleanup. __down_common() can use the new signal_pending_state() helper.
>
> This is a bad optimisation.  __down_common gets inlined and the constant
> 'state' versions are optimised away for the versions which don't apply.
>
> NAK this patch.

OK.

But this was not optimisation, just code re-use.

Actually, I don't understand why __down_common/__mutex_lock_common are inlines,
perhaps it is better to shrink .text instead.

For example, "size kernel/mutex.o" reports

	2715       0      12    2727     aa7 kernel/mutex.o

with __mutex_lock_common() uninlined, we have

	1565       0      12    1577     629 kernel/mutex.o

the difference is more than K.

Oleg.

Re: [PATCH 2/2] __down_common: use signal_pending_state()

[Matthew Wilcox]

On Wed, Jun 04, 2008 at 10:08:58PM +0400, Oleg Nesterov wrote:
> Actually, I don't understand why __down_common/__mutex_lock_common are inlines,
> perhaps it is better to shrink .text instead.

I believe Ingo did extensive benchmarking to determine that the current
situation with mutex_lock_common is the best one.  I didn't, I assumed
that following Ingo's lead would be the best idea.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH 2/2] __down_common: use signal_pending_state()

[Ingo Molnar]

* Matthew Wilcox <matthew@wil.cx> wrote:

> On Wed, Jun 04, 2008 at 09:09:06PM +0400, Oleg Nesterov wrote:
> > Cleanup. __down_common() can use the new signal_pending_state() helper.
> 
> This is a bad optimisation.  __down_common gets inlined and the 
> constant 'state' versions are optimised away for the versions which 
> don't apply.
> 
> NAK this patch.

well but we had a schedule() bug around this whole topic of 
TASK_KILLABLE, so i'd say robustness trumps micro-optimizations. Is this 
really a big issue? The dual-ness of TASK_KILLABLE checks is a bit ugly, 
and it led to a bug as well.

	Ingo