[PATCH 2/2] PF_DEAD: kill it

[PATCH 2/2] PF_DEAD: kill it

[Oleg Nesterov]

After the previous change (->flags & PF_DEAD) <=> (->state == EXIT_DEAD),
so we can forget about PF_DEAD flag.

In my opinion PF_DEAD has nothing to do with process state, it is just
indication that task_struct should be freed after the last schedule.

Perhaps it makes sense to add new TASK_DEAD flag to use it instead of
EXIT_DEAD, while the latter should live only in ->exit_state.

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

--- 2.6.15-rc2/include/linux/sched.h~5_KILL	2005-11-22 19:35:52.000000000 +0300
+++ 2.6.15-rc2/include/linux/sched.h	2005-11-24 20:06:19.000000000 +0300
@@ -890,7 +890,6 @@ do { if (atomic_dec_and_test(&(tsk)->usa
 					/* Not implemented yet, only for 486*/
 #define PF_STARTING	0x00000002	/* being created */
 #define PF_EXITING	0x00000004	/* getting shut down */
-#define PF_DEAD		0x00000008	/* Dead */
 #define PF_FORKNOEXEC	0x00000040	/* forked but didn't exec */
 #define PF_SUPERPRIV	0x00000100	/* used super-user privileges */
 #define PF_DUMPCORE	0x00000200	/* dumped core */
--- 2.6.15-rc2/kernel/exit.c~5_KILL	2005-11-24 19:14:29.000000000 +0300
+++ 2.6.15-rc2/kernel/exit.c	2005-11-24 20:12:34.000000000 +0300
@@ -871,10 +871,8 @@ fastcall NORET_TYPE void do_exit(long co
 	tsk->mempolicy = NULL;
 #endif
 
-	/* PF_DEAD causes final put_task_struct after we schedule. */
+	/* ->state == EXIT_DEAD causes final put_task_struct after we schedule. */
 	preempt_disable();
-	BUG_ON(tsk->flags & PF_DEAD);
-	tsk->flags |= PF_DEAD;
 	tsk->state = EXIT_DEAD;
 
 	schedule();
--- 2.6.15-rc2/kernel/sched.c~5_KILL	2005-11-24 19:13:34.000000000 +0300
+++ 2.6.15-rc2/kernel/sched.c	2005-11-24 21:12:46.000000000 +0300
@@ -1615,7 +1615,7 @@ static inline void finish_task_switch(ru
 	__releases(rq->lock)
 {
 	struct mm_struct *mm = rq->prev_mm;
-	unsigned long prev_task_flags;
+	int task_dead;
 
 	rq->prev_mm = NULL;
 
@@ -1630,12 +1630,12 @@ static inline void finish_task_switch(ru
 	 * be dropped twice.
 	 *		Manfred Spraul <manfred@colorfullife.com>
 	 */
-	prev_task_flags = prev->flags;
+	task_dead = (prev->state == EXIT_DEAD);
 	finish_arch_switch(prev);
 	finish_lock_switch(rq, prev);
 	if (mm)
 		mmdrop(mm);
-	if (unlikely(prev_task_flags & PF_DEAD))
+	if (unlikely(task_dead))
 		put_task_struct(prev);
 }
 
@@ -4711,7 +4711,7 @@ static void migrate_dead(unsigned int de
 	BUG_ON(tsk->exit_state != EXIT_ZOMBIE && tsk->exit_state != EXIT_DEAD);
 
 	/* Cannot have done final schedule yet: would have vanished. */
-	BUG_ON(tsk->flags & PF_DEAD);
+	BUG_ON(tsk->state == EXIT_DEAD);
 
 	get_task_struct(tsk);
 
--- 2.6.15-rc2/mm/oom_kill.c~5_KILL	2005-10-11 16:22:42.000000000 +0400
+++ 2.6.15-rc2/mm/oom_kill.c	2005-11-24 20:16:34.000000000 +0300
@@ -163,7 +163,7 @@ static struct task_struct * select_bad_p
 		 */
 		releasing = test_tsk_thread_flag(p, TIF_MEMDIE) ||
 						p->flags & PF_EXITING;
-		if (releasing && !(p->flags & PF_DEAD))
+		if (releasing && (p->state != EXIT_DEAD))
 			return ERR_PTR(-1UL);
 		if (p->flags & PF_SWAPOFF)
 			return p;

Re: [PATCH 2/2] PF_DEAD: kill it

[Ingo Molnar]

* Oleg Nesterov <oleg@tv-sign.ru> wrote:

> After the previous change (->flags & PF_DEAD) <=> (->state == 
> EXIT_DEAD), so we can forget about PF_DEAD flag.
> 
> In my opinion PF_DEAD has nothing to do with process state, it is just 
> indication that task_struct should be freed after the last schedule.
> 
> Perhaps it makes sense to add new TASK_DEAD flag to use it instead of 
> EXIT_DEAD, while the latter should live only in ->exit_state.

yes, i'd suggest a followup patch to keep the two flag spaces totally 
disjunct - at least for testing in -mm. This area of code (when it was 
introduced) was pretty fragile and blew up under PREEMPT+SMP a couple of 
times. The separate handling of PF_DEAD was necessary until i fixed 
do_exit() to only have PF_DEAD in an atomic path up to the final 
schedule(). You are right that we can totally eliminate it now.

so your patch looks great to me! (I have also added this patch (and the 
previous patch) to the -rt tree, to give it some more testing in more 
extreme preemption scenarios.)

Acked-by: Ingo Molnar <mingo@elte.hu>

	Ingo

Re: [PATCH 2/2] PF_DEAD: kill it

[Oleg Nesterov]

Ingo Molnar wrote:
>
> > Perhaps it makes sense to add new TASK_DEAD flag to use it instead of
> > EXIT_DEAD, while the latter should live only in ->exit_state.
>
> yes, i'd suggest a followup patch to keep the two flag spaces totally
> disjunct - at least for testing in -mm. This area of code (when it was

Here it is.

I am also resending these 2 cleanups with EXIT_STATE changed to TASK_STATE.

Andrew, please take these new ones unless you or Ingo have any objections.

Oleg.


--- 2.6.15-rc2/include/linux/sched.h~3_RENAME	2005-11-25 21:54:07.000000000 +0300
+++ 2.6.15-rc2/include/linux/sched.h	2005-11-25 21:56:36.000000000 +0300
@@ -127,6 +127,7 @@ extern unsigned long nr_iowait(void);
 #define EXIT_DEAD		32
 /* in tsk->state again */
 #define TASK_NONINTERACTIVE	64
+#define TASK_DEAD		128
 
 #define __set_task_state(tsk, state_value)		\
 	do { (tsk)->state = (state_value); } while (0)
--- 2.6.15-rc2/kernel/exit.c~3_RENAME	2005-11-25 21:54:07.000000000 +0300
+++ 2.6.15-rc2/kernel/exit.c	2005-11-25 21:56:36.000000000 +0300
@@ -871,9 +871,9 @@ fastcall NORET_TYPE void do_exit(long co
 	tsk->mempolicy = NULL;
 #endif
 
-	/* ->state == EXIT_DEAD causes final put_task_struct after we schedule. */
+	/* ->state == TASK_DEAD causes final put_task_struct after we schedule. */
 	preempt_disable();
-	tsk->state = EXIT_DEAD;
+	tsk->state = TASK_DEAD;
 
 	schedule();
 	BUG();
--- 2.6.15-rc2/kernel/sched.c~3_RENAME	2005-11-25 21:54:07.000000000 +0300
+++ 2.6.15-rc2/kernel/sched.c	2005-11-25 21:56:36.000000000 +0300
@@ -1630,7 +1630,7 @@ static inline void finish_task_switch(ru
 	 * be dropped twice.
 	 *		Manfred Spraul <manfred@colorfullife.com>
 	 */
-	task_dead = (prev->state == EXIT_DEAD);
+	task_dead = (prev->state == TASK_DEAD);
 	finish_arch_switch(prev);
 	finish_lock_switch(rq, prev);
 	if (mm)
@@ -4711,7 +4711,7 @@ static void migrate_dead(unsigned int de
 	BUG_ON(tsk->exit_state != EXIT_ZOMBIE && tsk->exit_state != EXIT_DEAD);
 
 	/* Cannot have done final schedule yet: would have vanished. */
-	BUG_ON(tsk->state == EXIT_DEAD);
+	BUG_ON(tsk->state == TASK_DEAD);
 
 	get_task_struct(tsk);
 
--- 2.6.15-rc2/mm/oom_kill.c~3_RENAME	2005-11-25 21:54:07.000000000 +0300
+++ 2.6.15-rc2/mm/oom_kill.c	2005-11-25 21:56:36.000000000 +0300
@@ -163,7 +163,7 @@ static struct task_struct * select_bad_p
 		 */
 		releasing = test_tsk_thread_flag(p, TIF_MEMDIE) ||
 						p->flags & PF_EXITING;
-		if (releasing && (p->state != EXIT_DEAD))
+		if (releasing && (p->state != TASK_DEAD))
 			return ERR_PTR(-1UL);
 		if (p->flags & PF_SWAPOFF)
 			return p;