Re: [PATCH 2/2] hung_task: improve the rcu_lock_break() logic

Aaron Tomlin
2015-03-20T16:56:20Z

On Tue 2015-03-17 20:25 +0100, Oleg Nesterov wrote:
> check_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.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
> kernel/hung_task.c | 29 ++++++++++++++++++++---------
> 1 files changed, 20 insertions(+), 9 deletions(-)
>
> diff --git a/kernel/hung_task.c b/kernel/hung_task.c
> index 4735b99..f488059 100644
> --- a/kernel/hung_task.c
> +++ b/kernel/hung_task.c
> @@ -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 */

Looks good to me. Thanks.

Acked-by: Aaron Tomlin <atomlin@redhat.com>