[PATCH 0/3] state/exit_state cleanups (Was: Remove unused variable ret from sync_thread_master())

[PATCH 0/3] state/exit_state cleanups (Was: Remove unused variable ret from sync_thread_master())

[Oleg Nesterov]

On 11/12, Oleg Nesterov wrote:
>
> On 11/12, Peter Zijlstra wrote:
> >
> > We have to put in something...
> >
> > 	BUILD_BUG_ON(1 + ilog2(TASK_STATE_MAX) != ARRAY_SIZE(task_state_array));
> >
> > However, since we always set it together with TASK_UNINTERUPTIBLE
> > userspace shouldn't actually ever see the I thing.
>
> OOPS. I didn't know that get_task_state() does &= TASK_REPORT. So it
> can never report anything > EXIT_DEAD.
>
> Perhaps we should change BUILD_BUG_ON() and shrink task_state_array?

Seriously, imho this looks confusing enough and deserves a cleanup.


As for "nobody should use exit_state". I'll try to recheck, but iirc
we already discussed this... do you remember any reason why
schedule_debug() can't check prev->state == TASK_DEAD instead of
->exit_state?

Note that ->exit_state is not exactly right, it is set by exit_notify()
but in_atomic_preempt_off() should be only ignored when the task does
the last schedule() in TASK_DEAD.

Oleg.

[PATCH 1/3] proc: cleanup/simplify get_task_state/task_state_array

[Oleg Nesterov]

get_task_state() and task_state_array[] look confusing and
suboptimal, it is not clear what it can actually report to
user-space and task_state_array[] blows .data for no reason.

1. state = (tsk->state & TASK_REPORT) | tsk->exit_state is not
   clear. TASK_REPORT is self-documenting but it is not clear
   what ->exit_state can add.

   Move the potential exit_state's (EXIT_ZOMBIE and EXIT_DEAD)
   into TASK_REPORT and use it to calculate the final result.

2. With the change above it is obvious that task_state_array[]
   has the unused entries just to make BUILD_BUG_ON() happy.

   Change this BUILD_BUG_ON() to use TASK_REPORT rather than
   TASK_STATE_MAX and shrink task_state_array[].

3. Turn the "while (state)" loop into fls(state).

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 fs/proc/array.c       |   15 +++------------
 include/linux/sched.h |    2 +-
 2 files changed, 4 insertions(+), 13 deletions(-)

diff --git a/fs/proc/array.c b/fs/proc/array.c
index cbd0f1b..c995807 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -140,24 +140,15 @@ static const char * const task_state_array[] = {
 	"t (tracing stop)",	/*   8 */
 	"Z (zombie)",		/*  16 */
 	"X (dead)",		/*  32 */
-	"x (dead)",		/*  64 */
-	"K (wakekill)",		/* 128 */
-	"W (waking)",		/* 256 */
-	"P (parked)",		/* 512 */
 };
 
 static inline const char *get_task_state(struct task_struct *tsk)
 {
-	unsigned int state = (tsk->state & TASK_REPORT) | tsk->exit_state;
-	const char * const *p = &task_state_array[0];
+	unsigned int state = (tsk->state | tsk->exit_state) & TASK_REPORT;
 
-	BUILD_BUG_ON(1 + ilog2(TASK_STATE_MAX) != ARRAY_SIZE(task_state_array));
+	BUILD_BUG_ON(1 + ilog2(TASK_REPORT) != ARRAY_SIZE(task_state_array)-1);
 
-	while (state) {
-		p++;
-		state >>= 1;
-	}
-	return *p;
+	return task_state_array[fls(state)];
 }
 
 static inline void task_state(struct seq_file *m, struct pid_namespace *ns,
diff --git a/include/linux/sched.h b/include/linux/sched.h
index e27baee..ee2c2e6 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -163,7 +163,7 @@ extern char ___assert_task_state[1 - 2*!!(
 /* get_task_state() */
 #define TASK_REPORT		(TASK_RUNNING | TASK_INTERRUPTIBLE | \
 				 TASK_UNINTERRUPTIBLE | __TASK_STOPPED | \
-				 __TASK_TRACED)
+				 __TASK_TRACED | EXIT_ZOMBIE | EXIT_DEAD)
 
 #define task_is_traced(task)	((task->state & __TASK_TRACED) != 0)
 #define task_is_stopped(task)	((task->state & __TASK_STOPPED) != 0)
-- 
1.5.5.1

[PATCH 2/3] fork: no need to initialize child->exit_state

[Oleg Nesterov]

A zombie task obviously can't fork(), remove the unnecessary
initialization of child->exit_state. It is zero anyway after
dup_task_struct().

Note: copy_process() is huge and it has a lot of chaotic
initializations, probably it makes sense to move them into the
new helper called by dup_task_struct().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/fork.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/kernel/fork.c b/kernel/fork.c
index 8531609..2cb6024 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1405,13 +1405,11 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 		p->tgid = p->pid;
 	}
 
-	p->pdeath_signal = 0;
-	p->exit_state = 0;
-
 	p->nr_dirtied = 0;
 	p->nr_dirtied_pause = 128 >> (PAGE_SHIFT - 10);
 	p->dirty_paused_when = 0;
 
+	p->pdeath_signal = 0;
 	INIT_LIST_HEAD(&p->thread_group);
 	p->task_works = NULL;
 
-- 
1.5.5.1

[tip:sched/core] tasks/fork: Remove unnecessary child->exit_state

[tip-bot for Oleg Nesterov]

Commit-ID:  bb8cbbfee68518796df4050868e5b0f5ad078f9f
Gitweb:     http://git.kernel.org/tip/bb8cbbfee68518796df4050868e5b0f5ad078f9f
Author:     Oleg Nesterov <oleg@redhat.com>
AuthorDate: Wed, 13 Nov 2013 15:36:12 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 27 Nov 2013 13:50:50 +0100

tasks/fork: Remove unnecessary child->exit_state

A zombie task obviously can't fork(), remove the unnecessary
initialization of child->exit_state. It is zero anyway after
dup_task_struct().

Note: copy_process() is huge and it has a lot of chaotic
initializations, probably it makes sense to move them into the
new helper called by dup_task_struct().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Cc: David Laight <David.Laight@ACULAB.COM>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/20131113143612.GA10540@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/fork.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/kernel/fork.c b/kernel/fork.c
index 728d5be..b308082 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1402,13 +1402,11 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 		p->tgid = p->pid;
 	}
 
-	p->pdeath_signal = 0;
-	p->exit_state = 0;
-
 	p->nr_dirtied = 0;
 	p->nr_dirtied_pause = 128 >> (PAGE_SHIFT - 10);
 	p->dirty_paused_when = 0;
 
+	p->pdeath_signal = 0;
 	INIT_LIST_HEAD(&p->thread_group);
 	p->task_works = NULL;
 

[PATCH 3/3] exit_state: kill task_is_dead()

[Oleg Nesterov]

task_is_dead() has no users since commit 43e13cc107cf
"cred: remove task_is_dead() from __task_cred() validation", and
nobody except exit.c should rely on ->exit_state (we still have
the users which should be changed).

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 include/linux/sched.h |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index ee2c2e6..33cf854 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -167,7 +167,6 @@ extern char ___assert_task_state[1 - 2*!!(
 
 #define task_is_traced(task)	((task->state & __TASK_TRACED) != 0)
 #define task_is_stopped(task)	((task->state & __TASK_STOPPED) != 0)
-#define task_is_dead(task)	((task)->exit_state != 0)
 #define task_is_stopped_or_traced(task)	\
 			((task->state & (__TASK_STOPPED | __TASK_TRACED)) != 0)
 #define task_contributes_to_load(task)	\
-- 
1.5.5.1

[tip:sched/core] tasks/exit: Remove unused task_is_dead() method

[tip-bot for Oleg Nesterov]

Commit-ID:  86506a99a62400e9f7b7d1344bcc9ea235faf98f
Gitweb:     http://git.kernel.org/tip/86506a99a62400e9f7b7d1344bcc9ea235faf98f
Author:     Oleg Nesterov <oleg@redhat.com>
AuthorDate: Wed, 13 Nov 2013 15:36:14 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 27 Nov 2013 13:50:52 +0100

tasks/exit: Remove unused task_is_dead() method

task_is_dead() has no users since commit 43e13cc107cf
("cred: remove task_is_dead() from __task_cred() validation"), and
nobody except exit.c should rely on ->exit_state (we still have
the users which should be changed).

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Cc: David Laight <David.Laight@ACULAB.COM>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Link: http://lkml.kernel.org/r/20131113143614.GA10547@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 include/linux/sched.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 7e35d4b..cda8d63 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -168,7 +168,6 @@ extern char ___assert_task_state[1 - 2*!!(
 
 #define task_is_traced(task)	((task->state & __TASK_TRACED) != 0)
 #define task_is_stopped(task)	((task->state & __TASK_STOPPED) != 0)
-#define task_is_dead(task)	((task)->exit_state != 0)
 #define task_is_stopped_or_traced(task)	\
 			((task->state & (__TASK_STOPPED | __TASK_TRACED)) != 0)
 #define task_contributes_to_load(task)	\

Re: [PATCH 0/3] state/exit_state cleanups (Was: Remove unused variable ret from sync_thread_master())

[Peter Zijlstra]

On Wed, Nov 13, 2013 at 03:35:52PM +0100, Oleg Nesterov wrote:
> On 11/12, Oleg Nesterov wrote:
> >
> > On 11/12, Peter Zijlstra wrote:
> > >
> > > We have to put in something...
> > >
> > > 	BUILD_BUG_ON(1 + ilog2(TASK_STATE_MAX) != ARRAY_SIZE(task_state_array));
> > >
> > > However, since we always set it together with TASK_UNINTERUPTIBLE
> > > userspace shouldn't actually ever see the I thing.
> >
> > OOPS. I didn't know that get_task_state() does &= TASK_REPORT. So it
> > can never report anything > EXIT_DEAD.
> >
> > Perhaps we should change BUILD_BUG_ON() and shrink task_state_array?
> 
> Seriously, imho this looks confusing enough and deserves a cleanup.
> 
> 
> As for "nobody should use exit_state". I'll try to recheck, but iirc
> we already discussed this... do you remember any reason why
> schedule_debug() can't check prev->state == TASK_DEAD instead of
> ->exit_state?

I have no such memories :/ but a quick test shows that such a kernel
does boot without issue.

> Note that ->exit_state is not exactly right, it is set by exit_notify()
> but in_atomic_preempt_off() should be only ignored when the task does
> the last schedule() in TASK_DEAD.

Agreed.

For these patches:

Acked-by: Peter Zijlstra <peterz@infradead.org>

Re: [PATCH 0/3] state/exit_state cleanups (Was: Remove unused variable ret from sync_thread_master())

[Peter Zijlstra]

On Wed, Nov 13, 2013 at 03:48:56PM +0100, Peter Zijlstra wrote:
> For these patches:
> 
> Acked-by: Peter Zijlstra <peterz@infradead.org>

On that, I'll just pick them up and stuff them through the scheduler
tree, its mostly sched related anyway.

[PATCH 0/1] sched: Check TASK_DEAD rather than EXIT_DEAD in schedule_debug()

[Oleg Nesterov]

On 11/13, Peter Zijlstra wrote:
>
> On Wed, Nov 13, 2013 at 03:35:52PM +0100, Oleg Nesterov wrote:
> >
> > As for "nobody should use exit_state". I'll try to recheck, but iirc
> > we already discussed this... do you remember any reason why
> > schedule_debug() can't check prev->state == TASK_DEAD instead of
> > ->exit_state?
>
> I have no such memories :/ but a quick test shows that such a kernel
> does boot without issue.

probably my memory fools me. OK, it seems that you agree with this change,
hopefully we both can't be wrong ;)

> Acked-by: Peter Zijlstra <peterz@infradead.org>

Thanks! And since picked this series, perhaps you can also queue this
one?

Oleg.

[PATCH 1/1] sched: Check TASK_DEAD rather than EXIT_DEAD in schedule_debug()

[Oleg Nesterov]

schedule_debug() ignores in_atomic() if prev->exit_state != 0.
This is not what we want, ->exit_state is set by exit_notify()
but we should complain until the task does the last schedule()
in TASK_DEAD.

See also 7407251a0e2e "PF_DEAD cleanup", I think this ancient
commit explains why schedule() had to rely on ->exit_state,
until that commit exit_notify() disabled preemption and set
PF_DEAD which was used to detect the exiting task.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/sched/core.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 5ac63c9..7184357 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2287,10 +2287,10 @@ static inline void schedule_debug(struct task_struct *prev)
 {
 	/*
 	 * Test if we are atomic. Since do_exit() needs to call into
-	 * schedule() atomically, we ignore that path for now.
-	 * Otherwise, whine if we are scheduling when we should not be.
+	 * schedule() atomically, we ignore that path. Otherwise whine
+	 * if we are scheduling when we should not.
 	 */
-	if (unlikely(in_atomic_preempt_off() && !prev->exit_state))
+	if (unlikely(in_atomic_preempt_off() && prev->state != TASK_DEAD))
 		__schedule_bug(prev);
 	rcu_sleep_check();
 
-- 
1.5.5.1

[tip:sched/core] sched: Check TASK_DEAD rather than EXIT_DEAD in schedule_debug()

[tip-bot for Oleg Nesterov]

Commit-ID:  192301e70af3f6803c6354a464ebfa742da738ae
Gitweb:     http://git.kernel.org/tip/192301e70af3f6803c6354a464ebfa742da738ae
Author:     Oleg Nesterov <oleg@redhat.com>
AuthorDate: Wed, 13 Nov 2013 16:45:38 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 27 Nov 2013 13:50:53 +0100

sched: Check TASK_DEAD rather than EXIT_DEAD in schedule_debug()

schedule_debug() ignores in_atomic() if prev->exit_state != 0.
This is not what we want, ->exit_state is set by exit_notify()
but we should complain until the task does the last schedule()
in TASK_DEAD.

See also 7407251a0e2e "PF_DEAD cleanup", I think this ancient
commit explains why schedule() had to rely on ->exit_state,
until that commit exit_notify() disabled preemption and set
PF_DEAD which was used to detect the exiting task.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Cc: David Laight <David.Laight@ACULAB.COM>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Link: http://lkml.kernel.org/r/20131113154538.GB15810@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/sched/core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 687985b..19db8f3 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2414,10 +2414,10 @@ static inline void schedule_debug(struct task_struct *prev)
 {
 	/*
 	 * Test if we are atomic. Since do_exit() needs to call into
-	 * schedule() atomically, we ignore that path for now.
-	 * Otherwise, whine if we are scheduling when we should not be.
+	 * schedule() atomically, we ignore that path. Otherwise whine
+	 * if we are scheduling when we should not.
 	 */
-	if (unlikely(in_atomic_preempt_off() && !prev->exit_state))
+	if (unlikely(in_atomic_preempt_off() && prev->state != TASK_DEAD))
 		__schedule_bug(prev);
 	rcu_sleep_check();
 

Re: [PATCH 0/1] sched: Check TASK_DEAD rather than EXIT_DEAD in schedule_debug()

[Peter Zijlstra]

On Wed, Nov 13, 2013 at 04:45:16PM +0100, Oleg Nesterov wrote:
> Thanks! And since picked this series, perhaps you can also queue this
> one?

Done; if there's something fishy we're bound to hit it before .14 (I
hope).