Skip to contentSkip to conversation

PF_DEAD: kill it

PATCH In review Oleg Nesterov 2 participants 3 messages Nov 24, 2005 – Nov 25, 2005

Subsystems (3)
Memory management Scheduler Memory management - OOM killer
lore
Share & tools
lore

Oleg Nesterov

author
#0 lore raw

PATCH 2/2 PF_DEAD: kill it

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 <[email protected]>
include/linux/sched.h renamed from include/linux/sched.h~5_KILL +0 -1
@@ -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 */
kernel/exit.c renamed from kernel/exit.c~5_KILL +1 -3
@@ -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();
kernel/sched.c renamed from kernel/sched.c~5_KILL +4 -4
@@ -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 <[email protected]>
 	 */
-	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);
mm/oom_kill.c renamed from mm/oom_kill.c~5_KILL +1 -1
@@ -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;
Acked-by on v1 2/2
#1 lore raw
Quoted 8 lines · 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.

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 <[email protected]>

Ingo

author
#2 lore raw
Quoted 6 lines · Ingo Molnar
Quoted 3 lines

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.

include/linux/sched.h renamed from include/linux/sched.h~3_RENAME +1 -0
@@ -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)
kernel/exit.c renamed from kernel/exit.c~3_RENAME +2 -2
@@ -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();
kernel/sched.c renamed from kernel/sched.c~3_RENAME +2 -2
@@ -1630,7 +1630,7 @@ static inline void finish_task_switch(ru
 	 * be dropped twice.
 	 *		Manfred Spraul <[email protected]>
 	 */
-	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);
mm/oom_kill.c renamed from mm/oom_kill.c~3_RENAME +1 -1
@@ -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;