[PATCH] mm, oom: remove 'prefer children over parent' heuristic

[PATCH] mm, oom: remove 'prefer children over parent' heuristic

[Shakeel Butt]

From the start of the git history of Linux, the kernel after selecting
the worst process to be oom-killed, prefer to kill its child (if the
child does not share mm with the parent). Later it was changed to prefer
to kill a child who is worst. If the parent is still the worst then the
parent will be killed.

This heuristic assumes that the children did less work than their parent
and by killing one of them, the work lost will be less. However this is
very workload dependent. If there is a workload which can benefit from
this heuristic, can use oom_score_adj to prefer children to be killed
before the parent.

The select_bad_process() has already selected the worst process in the
system/memcg. There is no need to recheck the badness of its children
and hoping to find a worse candidate. That's a lot of unneeded racy
work. So, let's remove this whole heuristic.

Signed-off-by: Shakeel Butt <shakeelb@google.com>
---
 mm/oom_kill.c | 49 ++++---------------------------------------------
 1 file changed, 4 insertions(+), 45 deletions(-)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 1a007dae1e8f..6cee185dc147 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -944,12 +944,7 @@ static int oom_kill_memcg_member(struct task_struct *task, void *unused)
 static void oom_kill_process(struct oom_control *oc, const char *message)
 {
 	struct task_struct *p = oc->chosen;
-	unsigned int points = oc->chosen_points;
-	struct task_struct *victim = p;
-	struct task_struct *child;
-	struct task_struct *t;
 	struct mem_cgroup *oom_group;
-	unsigned int victim_points = 0;
 	static DEFINE_RATELIMIT_STATE(oom_rs, DEFAULT_RATELIMIT_INTERVAL,
 					      DEFAULT_RATELIMIT_BURST);
 
@@ -971,53 +966,17 @@ static void oom_kill_process(struct oom_control *oc, const char *message)
 	if (__ratelimit(&oom_rs))
 		dump_header(oc, p);
 
-	pr_err("%s: Kill process %d (%s) score %u or sacrifice child\n",
-		message, task_pid_nr(p), p->comm, points);
-
-	/*
-	 * If any of p's children has a different mm and is eligible for kill,
-	 * the one with the highest oom_badness() score is sacrificed for its
-	 * parent.  This attempts to lose the minimal amount of work done while
-	 * still freeing memory.
-	 */
-	read_lock(&tasklist_lock);
-
-	/*
-	 * The task 'p' might have already exited before reaching here. The
-	 * put_task_struct() will free task_struct 'p' while the loop still try
-	 * to access the field of 'p', so, get an extra reference.
-	 */
-	get_task_struct(p);
-	for_each_thread(p, t) {
-		list_for_each_entry(child, &t->children, sibling) {
-			unsigned int child_points;
-
-			if (process_shares_mm(child, p->mm))
-				continue;
-			/*
-			 * oom_badness() returns 0 if the thread is unkillable
-			 */
-			child_points = oom_badness(child,
-				oc->memcg, oc->nodemask, oc->totalpages);
-			if (child_points > victim_points) {
-				put_task_struct(victim);
-				victim = child;
-				victim_points = child_points;
-				get_task_struct(victim);
-			}
-		}
-	}
-	put_task_struct(p);
-	read_unlock(&tasklist_lock);
+	pr_err("%s: Kill process %d (%s) score %lu or sacrifice child\n",
+		message, task_pid_nr(p), p->comm, oc->chosen_points);
 
 	/*
 	 * Do we need to kill the entire memory cgroup?
 	 * Or even one of the ancestor memory cgroups?
 	 * Check this out before killing the victim task.
 	 */
-	oom_group = mem_cgroup_get_oom_group(victim, oc->memcg);
+	oom_group = mem_cgroup_get_oom_group(p, oc->memcg);
 
-	__oom_kill_process(victim);
+	__oom_kill_process(p);
 
 	/*
 	 * If necessary, kill all tasks in the selected memory cgroup.
-- 
2.20.1.321.g9e740568ce-goog

Re: [PATCH] mm, oom: remove 'prefer children over parent' heuristic

[Tetsuo Handa]

Shakeel Butt wrote:
> +	pr_err("%s: Kill process %d (%s) score %lu or sacrifice child\n",
> +		message, task_pid_nr(p), p->comm, oc->chosen_points);

This patch is to make "or sacrifice child" false. And, the process reported
by this line will become always same with the process reported by

	pr_err("Killed process %d (%s) total-vm:%lukB, anon-rss:%lukB, file-rss:%lukB, shmem-rss:%lukB\n",
		task_pid_nr(victim), victim->comm, K(victim->mm->total_vm),
		K(get_mm_counter(victim->mm, MM_ANONPAGES)),
		K(get_mm_counter(victim->mm, MM_FILEPAGES)),
		K(get_mm_counter(victim->mm, MM_SHMEMPAGES)));

. Then, better to merge these pr_err() lines?

Re: [PATCH] mm, oom: remove 'prefer children over parent' heuristic

[Michal Hocko]

On Sun 20-01-19 13:50:59, Shakeel Butt wrote:
> >From the start of the git history of Linux, the kernel after selecting
> the worst process to be oom-killed, prefer to kill its child (if the
> child does not share mm with the parent). Later it was changed to prefer
> to kill a child who is worst. If the parent is still the worst then the
> parent will be killed.
> 
> This heuristic assumes that the children did less work than their parent
> and by killing one of them, the work lost will be less. However this is
> very workload dependent. If there is a workload which can benefit from
> this heuristic, can use oom_score_adj to prefer children to be killed
> before the parent.
> 
> The select_bad_process() has already selected the worst process in the
> system/memcg. There is no need to recheck the badness of its children
> and hoping to find a worse candidate. That's a lot of unneeded racy
> work. So, let's remove this whole heuristic.

Yes, I agree with this direction. Let's try it and see whether there is
anything really depending on the heuristic. I hope that is not the case
but at least we will hear about it and the reasoning behind.

I think the changelog should also mension that the heuristic is
dangerous because it make fork bomb like workloads to recover much later
because we constantly pick and kill processes which are not memory hogs.

> Signed-off-by: Shakeel Butt <shakeelb@google.com>

Appart from the nit in the printk output
Acked-by: Michal Hocko <mhocko@suse.com>

Also I would prefer s@p@victim@ because it makes the code more readable

I pressume you are going to send this along with the fix for the
use-after-free in one series.

Thanks.

> ---
>  mm/oom_kill.c | 49 ++++---------------------------------------------
>  1 file changed, 4 insertions(+), 45 deletions(-)
> 
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 1a007dae1e8f..6cee185dc147 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -944,12 +944,7 @@ static int oom_kill_memcg_member(struct task_struct *task, void *unused)
>  static void oom_kill_process(struct oom_control *oc, const char *message)
>  {
>  	struct task_struct *p = oc->chosen;
> -	unsigned int points = oc->chosen_points;
> -	struct task_struct *victim = p;
> -	struct task_struct *child;
> -	struct task_struct *t;
>  	struct mem_cgroup *oom_group;
> -	unsigned int victim_points = 0;
>  	static DEFINE_RATELIMIT_STATE(oom_rs, DEFAULT_RATELIMIT_INTERVAL,
>  					      DEFAULT_RATELIMIT_BURST);
>  
> @@ -971,53 +966,17 @@ static void oom_kill_process(struct oom_control *oc, const char *message)
>  	if (__ratelimit(&oom_rs))
>  		dump_header(oc, p);
>  
> -	pr_err("%s: Kill process %d (%s) score %u or sacrifice child\n",
> -		message, task_pid_nr(p), p->comm, points);
> -
> -	/*
> -	 * If any of p's children has a different mm and is eligible for kill,
> -	 * the one with the highest oom_badness() score is sacrificed for its
> -	 * parent.  This attempts to lose the minimal amount of work done while
> -	 * still freeing memory.
> -	 */
> -	read_lock(&tasklist_lock);
> -
> -	/*
> -	 * The task 'p' might have already exited before reaching here. The
> -	 * put_task_struct() will free task_struct 'p' while the loop still try
> -	 * to access the field of 'p', so, get an extra reference.
> -	 */
> -	get_task_struct(p);
> -	for_each_thread(p, t) {
> -		list_for_each_entry(child, &t->children, sibling) {
> -			unsigned int child_points;
> -
> -			if (process_shares_mm(child, p->mm))
> -				continue;
> -			/*
> -			 * oom_badness() returns 0 if the thread is unkillable
> -			 */
> -			child_points = oom_badness(child,
> -				oc->memcg, oc->nodemask, oc->totalpages);
> -			if (child_points > victim_points) {
> -				put_task_struct(victim);
> -				victim = child;
> -				victim_points = child_points;
> -				get_task_struct(victim);
> -			}
> -		}
> -	}
> -	put_task_struct(p);
> -	read_unlock(&tasklist_lock);
> +	pr_err("%s: Kill process %d (%s) score %lu or sacrifice child\n",
> +		message, task_pid_nr(p), p->comm, oc->chosen_points);
>  
>  	/*
>  	 * Do we need to kill the entire memory cgroup?
>  	 * Or even one of the ancestor memory cgroups?
>  	 * Check this out before killing the victim task.
>  	 */
> -	oom_group = mem_cgroup_get_oom_group(victim, oc->memcg);
> +	oom_group = mem_cgroup_get_oom_group(p, oc->memcg);
>  
> -	__oom_kill_process(victim);
> +	__oom_kill_process(p);
>  
>  	/*
>  	 * If necessary, kill all tasks in the selected memory cgroup.
> -- 
> 2.20.1.321.g9e740568ce-goog

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm, oom: remove 'prefer children over parent' heuristic

[Shakeel Butt]

On Sun, Jan 20, 2019 at 5:23 PM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> Shakeel Butt wrote:
> > +     pr_err("%s: Kill process %d (%s) score %lu or sacrifice child\n",
> > +             message, task_pid_nr(p), p->comm, oc->chosen_points);
>
> This patch is to make "or sacrifice child" false. And, the process reported
> by this line will become always same with the process reported by
>
>         pr_err("Killed process %d (%s) total-vm:%lukB, anon-rss:%lukB, file-rss:%lukB, shmem-rss:%lukB\n",
>                 task_pid_nr(victim), victim->comm, K(victim->mm->total_vm),
>                 K(get_mm_counter(victim->mm, MM_ANONPAGES)),
>                 K(get_mm_counter(victim->mm, MM_FILEPAGES)),
>                 K(get_mm_counter(victim->mm, MM_SHMEMPAGES)));
>
> . Then, better to merge these pr_err() lines?

Thanks, will remove the one in oom_kill_process.

Re: [PATCH] mm, oom: remove 'prefer children over parent' heuristic

[Shakeel Butt]

On Mon, Jan 21, 2019 at 1:19 AM Michal Hocko <mhocko@kernel.org> wrote:
>
> On Sun 20-01-19 13:50:59, Shakeel Butt wrote:
> > >From the start of the git history of Linux, the kernel after selecting
> > the worst process to be oom-killed, prefer to kill its child (if the
> > child does not share mm with the parent). Later it was changed to prefer
> > to kill a child who is worst. If the parent is still the worst then the
> > parent will be killed.
> >
> > This heuristic assumes that the children did less work than their parent
> > and by killing one of them, the work lost will be less. However this is
> > very workload dependent. If there is a workload which can benefit from
> > this heuristic, can use oom_score_adj to prefer children to be killed
> > before the parent.
> >
> > The select_bad_process() has already selected the worst process in the
> > system/memcg. There is no need to recheck the badness of its children
> > and hoping to find a worse candidate. That's a lot of unneeded racy
> > work. So, let's remove this whole heuristic.
>
> Yes, I agree with this direction. Let's try it and see whether there is
> anything really depending on the heuristic. I hope that is not the case
> but at least we will hear about it and the reasoning behind.
>
> I think the changelog should also mension that the heuristic is
> dangerous because it make fork bomb like workloads to recover much later
> because we constantly pick and kill processes which are not memory hogs.
>
> > Signed-off-by: Shakeel Butt <shakeelb@google.com>
>
> Appart from the nit in the printk output
> Acked-by: Michal Hocko <mhocko@suse.com>
>
> Also I would prefer s@p@victim@ because it makes the code more readable
>
> I pressume you are going to send this along with the fix for the
> use-after-free in one series.
>
> Thanks.

Yes, I will resend the series after incorporating the feedback.

>
> > ---
> >  mm/oom_kill.c | 49 ++++---------------------------------------------
> >  1 file changed, 4 insertions(+), 45 deletions(-)
> >
> > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > index 1a007dae1e8f..6cee185dc147 100644
> > --- a/mm/oom_kill.c
> > +++ b/mm/oom_kill.c
> > @@ -944,12 +944,7 @@ static int oom_kill_memcg_member(struct task_struct *task, void *unused)
> >  static void oom_kill_process(struct oom_control *oc, const char *message)
> >  {
> >       struct task_struct *p = oc->chosen;
> > -     unsigned int points = oc->chosen_points;
> > -     struct task_struct *victim = p;
> > -     struct task_struct *child;
> > -     struct task_struct *t;
> >       struct mem_cgroup *oom_group;
> > -     unsigned int victim_points = 0;
> >       static DEFINE_RATELIMIT_STATE(oom_rs, DEFAULT_RATELIMIT_INTERVAL,
> >                                             DEFAULT_RATELIMIT_BURST);
> >
> > @@ -971,53 +966,17 @@ static void oom_kill_process(struct oom_control *oc, const char *message)
> >       if (__ratelimit(&oom_rs))
> >               dump_header(oc, p);
> >
> > -     pr_err("%s: Kill process %d (%s) score %u or sacrifice child\n",
> > -             message, task_pid_nr(p), p->comm, points);
> > -
> > -     /*
> > -      * If any of p's children has a different mm and is eligible for kill,
> > -      * the one with the highest oom_badness() score is sacrificed for its
> > -      * parent.  This attempts to lose the minimal amount of work done while
> > -      * still freeing memory.
> > -      */
> > -     read_lock(&tasklist_lock);
> > -
> > -     /*
> > -      * The task 'p' might have already exited before reaching here. The
> > -      * put_task_struct() will free task_struct 'p' while the loop still try
> > -      * to access the field of 'p', so, get an extra reference.
> > -      */
> > -     get_task_struct(p);
> > -     for_each_thread(p, t) {
> > -             list_for_each_entry(child, &t->children, sibling) {
> > -                     unsigned int child_points;
> > -
> > -                     if (process_shares_mm(child, p->mm))
> > -                             continue;
> > -                     /*
> > -                      * oom_badness() returns 0 if the thread is unkillable
> > -                      */
> > -                     child_points = oom_badness(child,
> > -                             oc->memcg, oc->nodemask, oc->totalpages);
> > -                     if (child_points > victim_points) {
> > -                             put_task_struct(victim);
> > -                             victim = child;
> > -                             victim_points = child_points;
> > -                             get_task_struct(victim);
> > -                     }
> > -             }
> > -     }
> > -     put_task_struct(p);
> > -     read_unlock(&tasklist_lock);
> > +     pr_err("%s: Kill process %d (%s) score %lu or sacrifice child\n",
> > +             message, task_pid_nr(p), p->comm, oc->chosen_points);
> >
> >       /*
> >        * Do we need to kill the entire memory cgroup?
> >        * Or even one of the ancestor memory cgroups?
> >        * Check this out before killing the victim task.
> >        */
> > -     oom_group = mem_cgroup_get_oom_group(victim, oc->memcg);
> > +     oom_group = mem_cgroup_get_oom_group(p, oc->memcg);
> >
> > -     __oom_kill_process(victim);
> > +     __oom_kill_process(p);
> >
> >       /*
> >        * If necessary, kill all tasks in the selected memory cgroup.
> > --
> > 2.20.1.321.g9e740568ce-goog
>
> --
> Michal Hocko
> SUSE Labs