Re: [PATCH] Fix tsk->exit_state usage (resend)
Hi Eugene,
This already got merged into -mm, but ...
On Sun, 19 Aug 2007, Eugene Teo wrote:
>
> 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.
> +++ b/fs/proc/array.c
> @@ -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?
> +++ 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
> @@ -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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Fix tsk-exit_state usage (resend)
Hi Eugene,
This already got merged into -mm, but ...
On Sun, 19 Aug 2007, Eugene Teo wrote:
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.
+++ b/fs/proc/array.c
@@ -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?
+++ 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
@@ -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
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] 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(-)
diff --git a/fs/proc/array.c b/fs/proc/array.c
index 965625a..babb24d 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -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 = _state_array[0];
while (state) {
diff --git a/kernel/fork.c b/kernel/fork.c
index 7332e23..56d1b8b 100644
--- a/kernel/fork.c
+++ 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);
WARN_ON(atomic_read(>usage));
WARN_ON(tsk == current);
diff --git a/kernel/sched.c b/kernel/sched.c
index 45e17b8..8c27f08 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -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);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] 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(-)
diff --git a/fs/proc/array.c b/fs/proc/array.c
index 965625a..babb24d 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -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) {
diff --git a/kernel/fork.c b/kernel/fork.c
index 7332e23..56d1b8b 100644
--- a/kernel/fork.c
+++ 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);
WARN_ON(atomic_read(tsk-usage));
WARN_ON(tsk == current);
diff --git a/kernel/sched.c b/kernel/sched.c
index 45e17b8..8c27f08 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -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);
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
