[PATCH]:Replacing current->state with set_current_state in kernel/signal.c

[PATCH]:Replacing current->state with set_current_state in kernel/signal.c

[Shani Moideen]

Hi,

Replacing current->state with set_current_state in kernel/signal.c
	

thanks.

Signed-off-by: Shani Moideen <shani.moideen@wipro.com>
----


diff --git a/kernel/signal.c b/kernel/signal.c
index 3670225..44e2cf3 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2596,7 +2596,7 @@ sys_signal(int sig, __sighandler_t handler)
 asmlinkage long
 sys_pause(void)
 {
-       current->state = TASK_INTERRUPTIBLE;
+       set_current_state(TASK_INTERRUPTIBLE);
        schedule();
        return -ERESTARTNOHAND;
 }
@@ -2622,7 +2622,7 @@ asmlinkage long sys_rt_sigsuspend(sigset_t __user *unewset, size_t sigsetsize)
        recalc_sigpending();
        spin_unlock_irq(&current->sighand->siglock);

-       current->state = TASK_INTERRUPTIBLE;
+       set_current_state(TASK_INTERRUPTIBLE);
        schedule();
        set_thread_flag(TIF_RESTORE_SIGMASK);
        return -ERESTARTNOHAND;

-- 
Shani 

Re: [PATCH]:Replacing current->state with set_current_state in kernel/signal.c

[Eric Dumazet]

On Wed, 25 Apr 2007 12:08:58 +0530
Shani Moideen <shani.moideen@wipro.com> wrote:

> Hi,
> 
> Replacing current->state with set_current_state in kernel/signal.c
> 	

> @@ -2596,7 +2596,7 @@ sys_signal(int sig, __sighandler_t handler)
>  asmlinkage long
>  sys_pause(void)
>  {
> -       current->state = TASK_INTERRUPTIBLE;
> +       set_current_state(TASK_INTERRUPTIBLE);
>         schedule();

Hi Shani

Either you think you corrected a BUG, so please state it clearly in Changelog so that Linus immediatly apply your patch for 2.6.21 :)

Either you dont know the exact semantic of set_current_state() and think it's a cleaner way to set current->state.
It might looks better for you but it's not the *same* thing.

I suggest you carefully study the difference between set_current_state() and __set_current_state(), and submit a new patch, once you feel comfortable with it.

Here is the relevant extract from include/linux/sched.h

/*
 * set_current_state() includes a barrier so that the write of current->state
 * is correctly serialised wrt the caller's subsequent test of whether to
 * actually sleep:
 *
 *      set_current_state(TASK_UNINTERRUPTIBLE);
 *      if (do_i_need_to_sleep())
 *              schedule();
 *
 * If the caller does not need such serialisation then use __set_current_state()
 */
#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))


Thank you

Re: [PATCH]:Replacing current->state with set_current_state in kernel/signal.c

[Linus Torvalds]

On Wed, 25 Apr 2007, Eric Dumazet wrote:
> 
> Either you think you corrected a BUG, so please state it clearly in 
> Changelog so that Linus immediatly apply your patch for 2.6.21 :)

It's not a bug.

Setting current state manually is fine _iff_ you don't actually test a 
condition value. It's only if you do

	/* This has a race, and is bad! */
	current->state = TASK_INTERRUPTIBLE;
	if (some_condition)
		schedule();

that you have a race: the CPU (or the compiler, for that matter) can move 
the "some_condition" check up to before setting TASK_INTERRUPTIBLE, and if 
another CPU comes in and wakes you up, you might lose the wakeup.

But doing

	/* This is fine */
	current->state = TASK_INTERRUPTIBLE;
	schedule();

is fine.

		Linus