hung_task: Change hung_task.c to use for_each_process_thread()
cover hung_task: Change hung_task.c to use for_each_process_thread()
Hi Andrew,
Further work is required to improve khungtaskd. I'll do this later but for now let's start with this trivial clean up.
Aaron Tomlin (1):
hung_task: Change hung_task.c to use for_each_process_thread()
kernel/hung_task.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Signature · 1 line
1.9.3
PATCH 1/1 hung_task: Change hung_task.c to use for_each_process_thread()
In check_hung_uninterruptible_tasks() avoid the use of deprecated while_each_thread().
The "max_count" logic will prevents a livelock - see commit 0c740d0a ("introduce for_each_thread() to replace the buggy while_each_thread()"). Having said this let's use for_each_process_thread().
--- kernel/hung_task.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
kernel/hung_task.c +2 -2
@@ -169,7 +169,7 @@ static void check_hung_uninterruptible_tasks(unsigned long timeout)return ;rcu_read_lock (); - do_each_thread(g, t) { +for_each_process_thread (g, t) {if (!max_count--) goto unlock;if (!--batch_count) {
@@ -180,7 +180,7 @@ static void check_hung_uninterruptible_tasks(unsigned long timeout)/* use "==" to skip the TASK_KILLABLE tasks waiting on NFS */ if (t->state ==TASK_UNINTERRUPTIBLE )check_hung_task (t, timeout); - }while_each_thread (g, t); + } unlock:rcu_read_unlock (); }
Signature · 1 line
1.9.3
Quoted 13 lines · Aaron Tomlin
kernel/hung_task.c +1
-1@@ -169,7 +169,7 @@ static void check_hung_uninterruptible_tasks(unsigned long timeout)return ;rcu_read_lock (); - do_each_thread(g, t) { +for_each_process_thread (g, t) {if (!max_count--) goto unlock;if (!--batch_count) {
kernel/hung_task.c static void check_hung_uninterruptible_tasks(unsigned long timeout) · line 183
Quoted 5 lines
kernel/hung_task.c +1
-1@@ -180,7 +180,7 @@ static void check_hung_uninterruptible_tasks(unsigned long timeout)/* use "==" to skip the TASK_KILLABLE tasks waiting on NFS */ if (t->state ==TASK_UNINTERRUPTIBLE )check_hung_task (t, timeout); - }while_each_thread (g, t); + }
Perhaps it also makes sense to improve this rcu_lock_break a bit... For example, if 't' is dead but 'g' is alive we can continue the "for_each_process" part of this double loop. And if 't' is still alive then we can find the new leader and continue...
But I agree, lets start from this fix, then we will see.
Oleg.
Quoted 25 lines · Oleg Nesterov
Quoted 13 lines · Aaron Tomlin
kernel/hung_task.c +1
-1@@ -169,7 +169,7 @@ static void check_hung_uninterruptible_tasks(unsigned long timeout)return ;rcu_read_lock (); - do_each_thread(g, t) { +for_each_process_thread (g, t) {if (!max_count--) goto unlock;if (!--batch_count) {kernel/hung_task.c static void check_hung_uninterruptible_tasks(unsigned long timeout) · line 183
Quoted 5 lines
kernel/hung_task.c +1
-1@@ -180,7 +180,7 @@ static void check_hung_uninterruptible_tasks(unsigned long timeout)/* use "==" to skip the TASK_KILLABLE tasks waiting on NFS */ if (t->state ==TASK_UNINTERRUPTIBLE )check_hung_task (t, timeout); - }while_each_thread (g, t); + }Acked-by Oleg Nesterov <[email protected]>
Quoted 4 lines
Perhaps it also makes sense to improve this
rcu_lock_breaka bit... For example, if 't' is dead but 'g' is alive we can continue the "for_each_process" part of this double loop. And if 't' is still alive then we can find the new leader and continue...
OK - I'll incorporate that in a separate patch.
Thanks,
Signature · 1 line
Aaron Tomlin
cover hung_task: improve rcu_lock_break() logic
In reply to Oleg NesterovQuoted 26 lines · Oleg Nesterov
Quoted 13 lines · Aaron Tomlin
kernel/hung_task.c +1
-1@@ -169,7 +169,7 @@ static void check_hung_uninterruptible_tasks(unsigned long timeout)return ;rcu_read_lock (); - do_each_thread(g, t) { +for_each_process_thread (g, t) {if (!max_count--) goto unlock;if (!--batch_count) {kernel/hung_task.c static void check_hung_uninterruptible_tasks(unsigned long timeout) · line 183
Quoted 5 lines
kernel/hung_task.c +1
-1@@ -180,7 +180,7 @@ static void check_hung_uninterruptible_tasks(unsigned long timeout)/* use "==" to skip the TASK_KILLABLE tasks waiting on NFS */ if (t->state ==TASK_UNINTERRUPTIBLE )check_hung_task (t, timeout); - }while_each_thread (g, t); + }Acked-by Oleg Nesterov <[email protected]>
Quoted 6 lines
Perhaps it also makes sense to improve this
rcu_lock_breaka bit... For example, if 't' is dead but 'g' is alive we can continue the "for_each_process" part of this double loop. And if 't' is still alive then we can find the new leader and continue...But I agree, lets start from this fix, then we will see.
Something like this. on top of Aaron's change.
But actually I am not sure this really makes a lot of sense. But if yes, we can do more. We can save g->start_time before rcu_lock_break(), and then "both dead" case can use for_each_process() to (try to) find the first process whis has a ge start_time.
Oleg.
PATCH 1/2 hung_task: split for_each_process_thread() into for_each_process() + __for_each_thread()
Preparation. Change the main loop in check_hung_uninterruptible_tasks() to use the nested for_each_process() + __for_each_thread() loops explicitly. Note that we use __for_each_thread(), not for_each_thread(). This way it is clear that the inner loop doesn't depend on 'g' after we read ->signal.
--- kernel/hung_task.c | 23 ++++++++++++++--------- 1 files changed, 14 insertions(+), 9 deletions(-)
kernel/hung_task.c +14 -9
@@ -169,17 +169,22 @@ static void check_hung_uninterruptible_tasks(unsigned long timeout)return ;rcu_read_lock (); -for_each_process_thread (g, t) { -if (!max_count--) - goto unlock; -if (!--batch_count) { - batch_count =HUNG_TASK_BATCHING ; -if (!rcu_lock_break (g, t)) +for_each_process (g) { +struct signal_struct *sig = g->signal; + +__for_each_thread (sig, t) { +if (!max_count--) goto unlock; + +if (!--batch_count) { + batch_count =HUNG_TASK_BATCHING ; +if (!rcu_lock_break (g, t)) + goto unlock; + } +/* use "==" to skip the TASK_KILLABLE tasks */ +if (t->state ==TASK_UNINTERRUPTIBLE ) +check_hung_task (t, timeout); } -/* use "==" to skip the TASK_KILLABLE tasks waiting on NFS */ -if (t->state ==TASK_UNINTERRUPTIBLE ) -check_hung_task (t, timeout); } unlock:rcu_read_unlock ();
Signature · 1 line
1.5.5.1
Re: [PATCH 1/2] hung_task: split for_each_process_thread() into for_each_process() + __for_each_thread()
Quoted 37 lines · Oleg Nesterov
Preparation. Change the main loop in
check_hung_uninterruptible_tasks()to use the nestedfor_each_process()+__for_each_thread()loops explicitly. Note that we use__for_each_thread(), notfor_each_thread(). This way it is clear that the inner loop doesn't depend on 'g' after we read ->signal.Signed-off-by Oleg Nesterov <[email protected]>--- kernel/hung_task.c | 23 ++++++++++++++--------- 1 files changed, 14 insertions(+), 9 deletions(-)kernel/hung_task.c +11
-6@@ -169,17 +169,22 @@ static void check_hung_uninterruptible_tasks(unsigned long timeout)return ;rcu_read_lock (); -for_each_process_thread (g, t) { -if (!max_count--) - goto unlock; -if (!--batch_count) { - batch_count =HUNG_TASK_BATCHING ; -if (!rcu_lock_break (g, t)) +for_each_process (g) { +struct signal_struct *sig = g->signal; + +__for_each_thread (sig, t) { +if (!max_count--) goto unlock; + +if (!--batch_count) { + batch_count =HUNG_TASK_BATCHING ; +if (!rcu_lock_break (g, t)) + goto unlock; + }
kernel/hung_task.c static void check_hung_uninterruptible_tasks(unsigned long timeout) · line 190
Quoted 10 lines
kernel/hung_task.c +3
-3+/* use "==" to skip the TASK_KILLABLE tasks */ +if (t->state ==TASK_UNINTERRUPTIBLE ) +check_hung_task (t, timeout); } -/* use "==" to skip the TASK_KILLABLE tasks waiting on NFS */ -if (t->state ==TASK_UNINTERRUPTIBLE ) -check_hung_task (t, timeout); } unlock:rcu_read_unlock ();
PATCH 2/2 hung_task: improve the rcu_lock_break() logic
In reply to Oleg Nesterovcheck_hung_uninterruptible_tasks() stops after rcu_lock_break() if either "t" or "g" exits, this is suboptimal.
If "t" is alive, we can always continue, t->group_leader can be used as the new "g". We do not even bother to check g != NULL in this case.
If "g" is alive, we can at least continue the outer for_each_process() loop.
--- kernel/hung_task.c | 29 ++++++++++++++++++++--------- 1 files changed, 20 insertions(+), 9 deletions(-)
kernel/hung_task.c +20 -9
@@ -134,20 +134,26 @@ static void check_hung_task(struct task_struct *t, unsigned long timeout)* For preemptible RCU it is sufficient to call rcu_read_unlock in order * to exit the grace period. For classic RCU, a reschedule is required. */ -static bool rcu_lock_break (struct task_struct *g,struct task_struct *t) +static void rcu_lock_break (struct task_struct **g,struct task_struct **t) { -bool can_cont; +bool alive; + +get_task_struct (*g); +get_task_struct (*t); -get_task_struct (g); -get_task_struct (t);rcu_read_unlock ();cond_resched ();rcu_read_lock (); - can_cont =pid_alive (g) &&pid_alive (t); -put_task_struct (t); -put_task_struct (g); -return can_cont; + alive =pid_alive (*g); +put_task_struct (*g); +if (!alive) + *g =NULL ; + + alive =pid_alive (*t); +put_task_struct (*t); +if (!alive) + *t =NULL ; }/*
@@ -178,7 +184,12 @@ static void check_hung_uninterruptible_tasks(unsigned long timeout)if (!--batch_count) { batch_count =HUNG_TASK_BATCHING ; -if (!rcu_lock_break (g, t)) +rcu_lock_break (&g, &t); +if (t)/* in case g == NULL */ + g = t->group_leader; +else if (g)/* continue the outer loop */ +break ; +else /* both dead */ goto unlock; }/* use "==" to skip the TASK_KILLABLE tasks */
Signature · 1 line
1.5.5.1
Re: [PATCH 2/2] hung_task: improve the rcu_lock_break() logic
Quoted 56 lines · Oleg Nesterov
check_hung_uninterruptible_tasks()stops afterrcu_lock_break()if either "t" or "g" exits, this is suboptimal.If "t" is alive, we can always continue,
t->group_leadercan be used as the new "g". We do not even bother to check g != NULL in this case.If "g" is alive, we can at least continue the outer
for_each_process()loop.Signed-off-by Oleg Nesterov <[email protected]>--- kernel/hung_task.c | 29 ++++++++++++++++++++--------- 1 files changed, 20 insertions(+), 9 deletions(-)kernel/hung_task.c +14
-8@@ -134,20 +134,26 @@ static void check_hung_task(struct task_struct *t, unsigned long timeout)* For preemptible RCU it is sufficient to call rcu_read_unlock in order * to exit the grace period. For classic RCU, a reschedule is required. */ -static bool rcu_lock_break (struct task_struct *g,struct task_struct *t) +static void rcu_lock_break (struct task_struct **g,struct task_struct **t) { -bool can_cont; +bool alive; + +get_task_struct (*g); +get_task_struct (*t); -get_task_struct (g); -get_task_struct (t);rcu_read_unlock ();cond_resched ();rcu_read_lock (); - can_cont =pid_alive (g) &&pid_alive (t); -put_task_struct (t); -put_task_struct (g); -return can_cont; + alive =pid_alive (*g); +put_task_struct (*g); +if (!alive) + *g =NULL ; + + alive =pid_alive (*t); +put_task_struct (*t); +if (!alive) + *t =NULL ; }/* @@ -178,7 +184,12 @@ static void check_hung_uninterruptible_tasks(unsigned long timeout)if (!--batch_count) { batch_count =HUNG_TASK_BATCHING ;
kernel/hung_task.c static void check_hung_uninterruptible_tasks(unsigned long timeout) · line 195
Quoted 10 lines
kernel/hung_task.c +6
-1-if (!rcu_lock_break (g, t)) +rcu_lock_break (&g, &t); +if (t)/* in case g == NULL */ + g = t->group_leader; +else if (g)/* continue the outer loop */ +break ; +else /* both dead */ goto unlock; }/* use "==" to skip the TASK_KILLABLE tasks */
Looks good to me. Thanks.