Skip to contentSkip to conversation

Fix tsk->exit_state usage (resend)

PATCH In review Eugene Teo 2 participants 2 messages Aug 19, 2007 – Sep 5, 2007

Subsystems (4)
MM core Scheduler exec and binfmt Proc filesystem
lore
Share & tools
lore

Eugene Teo

author
#0 lore raw

PATCH 1/1 Fix tsk->exit_state usage (resend)

tsk->exit_state can only be 0, EXIT_ZOMBIE, or EXIT_DEAD. A non-zero test is the same as tsk->exit_state & (EXIT_ZOMBIE | EXIT_DEAD), so just testing tsk->exit_state is sufficient.

Signed-off-by Eugene Teo <[email protected]>
---
 fs/proc/array.c |    3 +--
 kernel/fork.c   |    2 +-
 kernel/sched.c  |    2 +-
 3 files changed, 3 insertions(+), 4 deletions(-)
fs/proc/array.c +1 -2
index 965625a..babb24d 100644
@@ -145,8 +145,7 @@ static inline const char *get_task_state(struct task_struct *tsk)
 					    TASK_UNINTERRUPTIBLE |
 					    TASK_STOPPED |
 					    TASK_TRACED)) |
-			(tsk->exit_state & (EXIT_ZOMBIE |
-					    EXIT_DEAD));
+					   tsk->exit_state;
 	const char **p = &task_state_array[0];
 
 	while (state) {
kernel/fork.c +1 -1
index 7332e23..56d1b8b 100644
@@ -115,7 +115,7 @@ EXPORT_SYMBOL(free_task);
 
 void __put_task_struct(struct task_struct *tsk)
 {
-	WARN_ON(!(tsk->exit_state & (EXIT_DEAD | EXIT_ZOMBIE)));
+	WARN_ON(!tsk->exit_state);
 	WARN_ON(atomic_read(&tsk->usage));
 	WARN_ON(tsk == current);
kernel/sched.c +1 -1
index 45e17b8..8c27f08 100644
@@ -5190,7 +5190,7 @@ static void migrate_dead(unsigned int dead_cpu, struct task_struct *p)
 	struct rq *rq = cpu_rq(dead_cpu);
 
 	/* Must be exiting, otherwise would be on tasklist. */
-	BUG_ON(p->exit_state != EXIT_ZOMBIE && p->exit_state != EXIT_DEAD);
+	BUG_ON(!p->exit_state);
 
 	/* Cannot have done final schedule yet: would have vanished. */
 	BUG_ON(p->state == TASK_DEAD);
#1 lore raw

Hi Eugene,

This already got merged into -mm, but ...

Quoted 4 lines · Eugene Teo

tsk->exit_state can only be 0, EXIT_ZOMBIE, or EXIT_DEAD. A non-zero test is the same as tsk->exit_state & (EXIT_ZOMBIE | EXIT_DEAD), so just testing tsk->exit_state is sufficient.

... IMHO this change harms the readability of the code.

fs/proc/array.c static inline const char *get_task_state(struct task_struct *tsk) · line 148

Quoted 8 lines

+++ b/fs/proc/array.c

fs/proc/array.c +1 -2
@@ -145,8 +145,7 @@ static inline const char *get_task_state(struct task_struct *tsk)
 					    TASK_UNINTERRUPTIBLE |
 					    TASK_STOPPED |
 					    TASK_TRACED)) |
-			(tsk->exit_state & (EXIT_ZOMBIE |
-					    EXIT_DEAD));
+					   tsk->exit_state;

Here, for example, the code is /purposefully/ enumerating all the task states, probably it makes sense to explicitly enumerate the exit states as well?

Quoted 11 lines
kernel/fork.c +3 -1
+++ b/kernel/fork.c

@@ -115,7 +115,7 @@ EXPORT_SYMBOL(free_task);
 
 void __put_task_struct(struct task_struct *tsk)
 {
-	WARN_ON(!(tsk->exit_state & (EXIT_DEAD | EXIT_ZOMBIE)));
+	WARN_ON(!tsk->exit_state);
 
 
+++ b/kernel/sched.c

kernel/fork.c static void migrate_dead(unsigned int dead_cpu, struct task_struct *p) · line 5193

Quoted 5 lines
kernel/fork.c +1 -1
@@ -5190,7 +5190,7 @@ static void migrate_dead(unsigned int dead_cpu, struct task_struct *p)
 	struct rq *rq = cpu_rq(dead_cpu);
 
 	/* Must be exiting, otherwise would be on tasklist. */
-	BUG_ON(p->exit_state != EXIT_ZOMBIE && p->exit_state != EXIT_DEAD);
+	BUG_ON(!p->exit_state);

Regarding above two changes -- agreed, we want to catch /any/ exiting task state, so (!p->exit_state) is /correct/, but still, enumerating those explicitly helps readability. And although it's unlikely, in the future, we may have an exit_state value for which we may _not_ want to complain (WARN or BUG) in this code. So I'd still vote to keep the code explicit like it was ...

Satyam