Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1726753 > unrolled thread

Re: [v7 5/5] mm, oom: cgroup v2 mount option to disable cgroup-aware OOM killer

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [v7 5/5] mm, oom: cgroup v2 mount option to disable cgroup-aware  OOM killer Michal Hocko <mhocko@kernel.org> - 2017-09-05 15:50 +0200
    Re: [v7 5/5] mm, oom: cgroup v2 mount option to disable cgroup-aware  OOM killer Michal Hocko <mhocko@kernel.org> - 2017-09-05 17:20 +0200
      Re: [v7 5/5] mm, oom: cgroup v2 mount option to disable cgroup-aware  OOM killer Michal Hocko <mhocko@kernel.org> - 2017-09-06 10:50 +0200
        Re: [v7 5/5] mm, oom: cgroup v2 mount option to disable cgroup-aware  OOM killer Michal Hocko <mhocko@kernel.org> - 2017-09-06 20:00 +0200
        Re: [v7 5/5] mm, oom: cgroup v2 mount option to disable cgroup-aware  OOM killer David Rientjes <rientjes@google.com> - 2017-09-06 23:00 +0200
          Re: [v7 5/5] mm, oom: cgroup v2 mount option to disable cgroup-aware  OOM killer Christopher Lameter <cl@linux.com> - 2017-09-07 16:50 +0200
            Re: [v7 5/5] mm, oom: cgroup v2 mount option to disable cgroup-aware  OOM killer Christopher Lameter <cl@linux.com> - 2017-09-07 17:10 +0200
              Re: [v7 5/5] mm, oom: cgroup v2 mount option to disable cgroup-aware  OOM killer Christopher Lameter <cl@linux.com> - 2017-09-07 19:10 +0200
            Re: [v7 5/5] mm, oom: cgroup v2 mount option to disable cgroup-aware  OOM killer David Rientjes <rientjes@google.com> - 2017-09-08 00:00 +0200
      Re: [v7 5/5] mm, oom: cgroup v2 mount option to disable cgroup-aware  OOM killer Christopher Lameter <cl@linux.com> - 2017-09-07 18:30 +0200
    Re: [v7 5/5] mm, oom: cgroup v2 mount option to disable cgroup-aware  OOM killer Johannes Weiner <hannes@cmpxchg.org> - 2017-09-06 00:00 +0200
      Re: [v7 5/5] mm, oom: cgroup v2 mount option to disable cgroup-aware  OOM killer Michal Hocko <mhocko@kernel.org> - 2017-09-06 10:30 +0200
        Re: [v7 5/5] mm, oom: cgroup v2 mount option to disable cgroup-aware  OOM killer Johannes Weiner <hannes@cmpxchg.org> - 2017-09-07 18:20 +0200
        Re: [v7 5/5] mm, oom: cgroup v2 mount option to disable cgroup-aware  OOM killer Christopher Lameter <cl@linux.com> - 2017-09-07 18:30 +0200
          Re: [v7 5/5] mm, oom: cgroup v2 mount option to disable cgroup-aware  OOM killer David Rientjes <rientjes@google.com> - 2017-09-08 00:10 +0200
            Re: [v7 5/5] mm, oom: cgroup v2 mount option to disable cgroup-aware  OOM killer Christopher Lameter <cl@linux.com> - 2017-09-08 23:10 +0200
              Re: [v7 5/5] mm, oom: cgroup v2 mount option to disable cgroup-aware  OOM killer David Rientjes <rientjes@google.com> - 2017-09-09 10:50 +0200

#1726753 — Re: [v7 5/5] mm, oom: cgroup v2 mount option to disable cgroup-aware OOM killer

I will go and check patch 2 more deeply but this is something that I
wanted to sort out first.

On Mon 04-09-17 15:21:08, Roman Gushchin wrote:
> Introducing of cgroup-aware OOM killer changes the victim selection
> algorithm used by default: instead of picking the largest process,
> it will pick the largest memcg and then the largest process inside.
> 
> This affects only cgroup v2 users.
> 
> To provide a way to use cgroups v2 if the old OOM victim selection
> algorithm is preferred for some reason, the nogroupoom mount option
> is added.
> 
> If set, the OOM selection is performed in a "traditional" per-process
> way. Both oom_priority and oom_group memcg knobs are ignored.

Why is this an opt out rather than opt-in? IMHO the original oom logic
should be preserved by default and specific workloads should opt in for
the cgroup aware logic. Changing the global behavior depending on
whether cgroup v2 interface is in use is more than unexpected and IMHO
wrong approach to take. I think we should instead go with 
oom_strategy=[alloc_task,biggest_task,cgroup]

we currently have alloc_task (via sysctl_oom_kill_allocating_task) and
biggest_task which is the default. You are adding cgroup and the more I
think about the more I agree that it doesn't really make sense to try to
fit thew new semantic into the existing one (compare tasks to kill-all
memcgs). Just introduce a new strategy and define a new semantic from
scratch. Memcg priority and kill-all are a natural extension of this new
strategy. This will make the life easier and easier to understand by
users.

Does that make sense to you?

> Signed-off-by: Roman Gushchin <guro@fb.com>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: kernel-team@fb.com
> Cc: cgroups@vger.kernel.org
> Cc: linux-doc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-mm@kvack.org
> ---
>  Documentation/admin-guide/kernel-parameters.txt | 1 +
>  mm/memcontrol.c                                 | 8 ++++++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 28f1a0f84456..07891f1030aa 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -489,6 +489,7 @@
>  			Format: <string>
>  			nosocket -- Disable socket memory accounting.
>  			nokmem -- Disable kernel memory accounting.
> +			nogroupoom -- Disable cgroup-aware OOM killer.
>  
>  	checkreqprot	[SELINUX] Set initial checkreqprot flag value.
>  			Format: { "0" | "1" }
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index d7dd293897ca..6a8235dc41f6 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -87,6 +87,9 @@ static bool cgroup_memory_nosocket;
>  /* Kernel memory accounting disabled? */
>  static bool cgroup_memory_nokmem;
>  
> +/* Cgroup-aware OOM  disabled? */
> +static bool cgroup_memory_nogroupoom;
> +
>  /* Whether the swap controller is active */
>  #ifdef CONFIG_MEMCG_SWAP
>  int do_swap_account __read_mostly;
> @@ -2822,6 +2825,9 @@ bool mem_cgroup_select_oom_victim(struct oom_control *oc)
>  	if (mem_cgroup_disabled())
>  		return false;
>  
> +	if (cgroup_memory_nogroupoom)
> +		return false;
> +
>  	if (!cgroup_subsys_on_dfl(memory_cgrp_subsys))
>  		return false;
>  
> @@ -6188,6 +6194,8 @@ static int __init cgroup_memory(char *s)
>  			cgroup_memory_nosocket = true;
>  		if (!strcmp(token, "nokmem"))
>  			cgroup_memory_nokmem = true;
> +		if (!strcmp(token, "nogroupoom"))
> +			cgroup_memory_nogroupoom = true;
>  	}
>  	return 0;
>  }
> -- 
> 2.13.5

-- 
Michal Hocko
SUSE Labs

[toc] | [next] | [standalone]

#1726814

On Tue 05-09-17 15:30:21, Roman Gushchin wrote:
> On Tue, Sep 05, 2017 at 03:44:12PM +0200, Michal Hocko wrote:
[...]
> > Why is this an opt out rather than opt-in? IMHO the original oom logic
> > should be preserved by default and specific workloads should opt in for
> > the cgroup aware logic. Changing the global behavior depending on
> > whether cgroup v2 interface is in use is more than unexpected and IMHO
> > wrong approach to take. I think we should instead go with 
> > oom_strategy=[alloc_task,biggest_task,cgroup]
> > 
> > we currently have alloc_task (via sysctl_oom_kill_allocating_task) and
> > biggest_task which is the default. You are adding cgroup and the more I
> > think about the more I agree that it doesn't really make sense to try to
> > fit thew new semantic into the existing one (compare tasks to kill-all
> > memcgs). Just introduce a new strategy and define a new semantic from
> > scratch. Memcg priority and kill-all are a natural extension of this new
> > strategy. This will make the life easier and easier to understand by
> > users.
> > 
> > Does that make sense to you?
> 
> Absolutely.
> 
> The only thing: I'm not sure that we have to preserve the existing logic
> as default option. For most users (except few very specific usecases),
> it should be at least as good, as the existing one.

But this is really an unexpected change. Users even might not know that
they are using cgroup v2 and memcg is in use.

> Making it opt-in means that corresponding code will be executed only
> by few users, who cares.

Yeah, which is the way we should introduce new features no?

> Then we should probably hide corresponding
> cgroup interface (oom_group and oom_priority knobs) by default,
> and it feels as unnecessary complication and is overall against
> cgroup v2 interface design.

Why. If we care enough, we could simply return EINVAL when those knobs
are written while the corresponding strategy is not used.

> > I think we should instead go with
> > oom_strategy=[alloc_task,biggest_task,cgroup]
> 
> It would be a really nice interface; although I've no idea how to implement it:
> "alloc_task" is an existing sysctl, which we have to preserve;

I would argue that we should simply deprecate and later drop the sysctl.
I _strongly_ suspect anybody is using this. If yes it is not that hard
to change the kernel command like rather than select the sysctl. The
deprecation process would be
	- warn when somebody writes to the sysctl and check both boot
	  and sysctl values
	[ wait some time ]
	- keep the sysctl but return EINVAL
	[ wait some time ]
	- remove the sysctl

> while "cgroup" depends on cgroup v2.

Which is not a big deal either. Simply fall back to default if there are
no cgroup v2. The implementation would have essentially the same effect
because there won't be any kill-all cgroups and so we will select the
largest task.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1727237

On Tue 05-09-17 20:16:09, Roman Gushchin wrote:
> On Tue, Sep 05, 2017 at 05:12:51PM +0200, Michal Hocko wrote:
[...]
> > > Then we should probably hide corresponding
> > > cgroup interface (oom_group and oom_priority knobs) by default,
> > > and it feels as unnecessary complication and is overall against
> > > cgroup v2 interface design.
> > 
> > Why. If we care enough, we could simply return EINVAL when those knobs
> > are written while the corresponding strategy is not used.
> 
> It doesn't look as a nice default interface.

I do not have a strong opinion on this. A printk_once could explain why
the knob is ignored and instruct the admin how to enable the feature
completely.
 
> > > > I think we should instead go with
> > > > oom_strategy=[alloc_task,biggest_task,cgroup]
> > > 
> > > It would be a really nice interface; although I've no idea how to implement it:
> > > "alloc_task" is an existing sysctl, which we have to preserve;
> > 
> > I would argue that we should simply deprecate and later drop the sysctl.
> > I _strongly_ suspect anybody is using this. If yes it is not that hard
> > to change the kernel command like rather than select the sysctl.
> 
> I agree. And if so, why do we need a new interface for an useless feature?

Well, I won't be opposed just deprecating the sysfs and only add a
"real" kill-allocate strategy if somebody explicitly asks for it.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1727667

On Wed 06-09-17 18:40:43, Roman Gushchin wrote:
[...]
> >From f6e2339926a07500834d86548f3f116af7335d71 Mon Sep 17 00:00:00 2001
> From: Roman Gushchin <guro@fb.com>
> Date: Wed, 6 Sep 2017 17:43:44 +0100
> Subject: [PATCH] mm, oom: first step towards oom_kill_allocating_task
>  deprecation
> 
> The oom_kill_allocating_task sysctl which causes the OOM killer
> to simple kill the allocating task is useless. Killing the random

useless is quite strong ;) I would say dubious.

> task is not the best idea.
> 
> Nobody likes it, and hopefully nobody uses it.
> We want to completely deprecate it at some point.
> 
> To make a first step towards deprecation, let's warn potential
> users about deprecation plans.
> 
> Signed-off-by: Roman Gushchin <guro@fb.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org

Other than that
Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  kernel/sysctl.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 655686d546cb..9158f1980584 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -220,6 +220,17 @@ static int sysrq_sysctl_handler(struct ctl_table *table, int write,
>  
>  #endif
>  
> +static int proc_oom_kill_allocating_tasks(struct ctl_table *table, int write,
> +				   void __user *buffer, size_t *lenp,
> +				   loff_t *ppos)
> +{
> +	pr_warn_once("The oom_kill_allocating_task sysctl will be deprecated.\n"
> +		     "If you're using it, please, report to "
> +		     "linux-mm@kvack.kernel.org.\n");
> +
> +	return proc_dointvec(table, write, buffer, lenp, ppos);
> +}
> +
>  static struct ctl_table kern_table[];
>  static struct ctl_table vm_table[];
>  static struct ctl_table fs_table[];
> @@ -1235,7 +1246,7 @@ static struct ctl_table vm_table[] = {
>  		.data		= &sysctl_oom_kill_allocating_task,
>  		.maxlen		= sizeof(sysctl_oom_kill_allocating_task),
>  		.mode		= 0644,
> -		.proc_handler	= proc_dointvec,
> +		.proc_handler	= proc_oom_kill_allocating_tasks,
>  	},
>  	{
>  		.procname	= "oom_dump_tasks",
> -- 
> 2.13.5

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1727729

On Wed, 6 Sep 2017, Roman Gushchin wrote:

> From f6e2339926a07500834d86548f3f116af7335d71 Mon Sep 17 00:00:00 2001
> From: Roman Gushchin <guro@fb.com>
> Date: Wed, 6 Sep 2017 17:43:44 +0100
> Subject: [PATCH] mm, oom: first step towards oom_kill_allocating_task
>  deprecation
> 
> The oom_kill_allocating_task sysctl which causes the OOM killer
> to simple kill the allocating task is useless. Killing the random
> task is not the best idea.
> 
> Nobody likes it, and hopefully nobody uses it.
> We want to completely deprecate it at some point.
> 

SGI required it when it was introduced simply to avoid the very expensive 
tasklist scan.  Adding Christoph Lameter to the cc since he was involved 
back then.

I attempted to deprecate the old /proc/pid/oom_adj in this same manner; we 
warned about it for over a year and then finally removed it, one person 
complained of breakage, and it was reverted with a strict policy that 
Linux doesn't break userspace.

Although it would be good to do, I'm not sure that this is possible unless 
it can be shown nobody is using it.  Talking to SGI would be the first 
step.

I'm not sure what this has to do with the overall patchset though :)

> To make a first step towards deprecation, let's warn potential
> users about deprecation plans.
> 
> Signed-off-by: Roman Gushchin <guro@fb.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  kernel/sysctl.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 655686d546cb..9158f1980584 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -220,6 +220,17 @@ static int sysrq_sysctl_handler(struct ctl_table *table, int write,
>  
>  #endif
>  
> +static int proc_oom_kill_allocating_tasks(struct ctl_table *table, int write,
> +				   void __user *buffer, size_t *lenp,
> +				   loff_t *ppos)
> +{
> +	pr_warn_once("The oom_kill_allocating_task sysctl will be deprecated.\n"
> +		     "If you're using it, please, report to "
> +		     "linux-mm@kvack.kernel.org.\n");
> +
> +	return proc_dointvec(table, write, buffer, lenp, ppos);
> +}
> +
>  static struct ctl_table kern_table[];
>  static struct ctl_table vm_table[];
>  static struct ctl_table fs_table[];
> @@ -1235,7 +1246,7 @@ static struct ctl_table vm_table[] = {
>  		.data		= &sysctl_oom_kill_allocating_task,
>  		.maxlen		= sizeof(sysctl_oom_kill_allocating_task),
>  		.mode		= 0644,
> -		.proc_handler	= proc_dointvec,
> +		.proc_handler	= proc_oom_kill_allocating_tasks,
>  	},
>  	{
>  		.procname	= "oom_dump_tasks",

[toc] | [prev] | [next] | [standalone]

#1728248

On Wed, 6 Sep 2017, David Rientjes wrote:

> > The oom_kill_allocating_task sysctl which causes the OOM killer
> > to simple kill the allocating task is useless. Killing the random
> > task is not the best idea.
> >
> > Nobody likes it, and hopefully nobody uses it.
> > We want to completely deprecate it at some point.
> >
>
> SGI required it when it was introduced simply to avoid the very expensive
> tasklist scan.  Adding Christoph Lameter to the cc since he was involved
> back then.

Really? From what I know and worked on way back when: The reason was to be
able to contain the affected application in a cpuset. Multiple apps may
have been running in multiple cpusets on a large NUMA machine and the OOM
condition in one cpuset should not affect the other. It also helped to
isolate the application behavior causing the oom in numerous cases.

Doesnt this requirement transfer to cgroups in the same way?

Left SGI in 2008 so adding Dimitri who may know about the current
situation. Robin Holt also left SGI as far as I know.

[toc] | [prev] | [next] | [standalone]

#1728254

On Thu, 7 Sep 2017, Roman Gushchin wrote:

> > Really? From what I know and worked on way back when: The reason was to be
> > able to contain the affected application in a cpuset. Multiple apps may
> > have been running in multiple cpusets on a large NUMA machine and the OOM
> > condition in one cpuset should not affect the other. It also helped to
> > isolate the application behavior causing the oom in numerous cases.
> >
> > Doesnt this requirement transfer to cgroups in the same way?
>
> We have per-node memory stats and plan to use them during the OOM victim
> selection. Hopefully it can help.

One of the OOM causes could be that memory was restricted to a certain
node set. Killing the allocating task is (was?) default behavior in that
case so that the task that has the restrictions is killed. Not any task
that may not have the restrictions and woiuld not experience OOM.

[toc] | [prev] | [next] | [standalone]

#1728319

On Thu, 7 Sep 2017, Roman Gushchin wrote:

> On Thu, Sep 07, 2017 at 10:03:24AM -0500, Christopher Lameter wrote:
> > On Thu, 7 Sep 2017, Roman Gushchin wrote:
> >
> > > > Really? From what I know and worked on way back when: The reason was to be
> > > > able to contain the affected application in a cpuset. Multiple apps may
> > > > have been running in multiple cpusets on a large NUMA machine and the OOM
> > > > condition in one cpuset should not affect the other. It also helped to
> > > > isolate the application behavior causing the oom in numerous cases.
> > > >
> > > > Doesnt this requirement transfer to cgroups in the same way?
> > >
> > > We have per-node memory stats and plan to use them during the OOM victim
> > > selection. Hopefully it can help.
> >
> > One of the OOM causes could be that memory was restricted to a certain
> > node set. Killing the allocating task is (was?) default behavior in that
> > case so that the task that has the restrictions is killed. Not any task
> > that may not have the restrictions and woiuld not experience OOM.
>
> As I can see, it's not the default behavior these days. If we have a way
> to select a victim between memcgs/tasks which are actually using
> the corresponding type of memory, it's much better than to kill
> an allocating task.

Kill the whole set of processes constituting an app in a cgroup or so
sounds good to me.

[toc] | [prev] | [next] | [standalone]

#1728466

On Thu, 7 Sep 2017, Christopher Lameter wrote:

> > SGI required it when it was introduced simply to avoid the very expensive
> > tasklist scan.  Adding Christoph Lameter to the cc since he was involved
> > back then.
> 
> Really? From what I know and worked on way back when: The reason was to be
> able to contain the affected application in a cpuset. Multiple apps may
> have been running in multiple cpusets on a large NUMA machine and the OOM
> condition in one cpuset should not affect the other. It also helped to
> isolate the application behavior causing the oom in numerous cases.
> 
> Doesnt this requirement transfer to cgroups in the same way?
> 
> Left SGI in 2008 so adding Dimitri who may know about the current
> situation. Robin Holt also left SGI as far as I know.
> 

It may have been Paul Jackson, but I remember the oom_kill_allocating_task 
knob being required due to very slow oom killer due to the very lengthy 
iteration of the tasklist.  It would be helpful if someone from SGI could 
confirm whether or not they actively use this sysctl.

[toc] | [prev] | [next] | [standalone]

#1728300

On Tue, 5 Sep 2017, Michal Hocko wrote:

> I would argue that we should simply deprecate and later drop the sysctl.
> I _strongly_ suspect anybody is using this. If yes it is not that hard
> to change the kernel command like rather than select the sysctl. The
> deprecation process would be
> 	- warn when somebody writes to the sysctl and check both boot
> 	  and sysctl values
> 	[ wait some time ]
> 	- keep the sysctl but return EINVAL
> 	[ wait some time ]
> 	- remove the sysctl

Note that the behavior that would be enabled by the sysctl is the default
behavior for the case of a constrained allocation. If a process does an
mbind to numa node 3 and it runs out of memory then that process should be
killed and the rest is fine.

[toc] | [prev] | [next] | [standalone]

#1727014

On Tue, Sep 05, 2017 at 03:44:12PM +0200, Michal Hocko wrote:
> Why is this an opt out rather than opt-in? IMHO the original oom logic
> should be preserved by default and specific workloads should opt in for
> the cgroup aware logic. Changing the global behavior depending on
> whether cgroup v2 interface is in use is more than unexpected and IMHO
> wrong approach to take. I think we should instead go with 
> oom_strategy=[alloc_task,biggest_task,cgroup]
> 
> we currently have alloc_task (via sysctl_oom_kill_allocating_task) and
> biggest_task which is the default. You are adding cgroup and the more I
> think about the more I agree that it doesn't really make sense to try to
> fit thew new semantic into the existing one (compare tasks to kill-all
> memcgs). Just introduce a new strategy and define a new semantic from
> scratch. Memcg priority and kill-all are a natural extension of this new
> strategy. This will make the life easier and easier to understand by
> users.

oom_kill_allocating_task is actually a really good example of why
cgroup-awareness *should* be the new default.

Before we had the oom killer victim selection, we simply killed the
faulting/allocating task. While a valid answer to the problem, it's
not very fair or representative of what the user wants or intends.

Then we added code to kill the biggest offender instead, which should
have been the case from the start and was hence made the new default.
The oom_kill_allocating_task was added on the off-chance that there
might be setups who, for historical reasons, rely on the old behavior.
But our default was chosen based on what behavior is fair, expected,
and most reflective of the user's intentions.

The cgroup-awareness in the OOM killer is exactly the same thing. It
should have been the default from the beginning, because the user
configures a group of tasks to be an interdependent, terminal unit of
memory consumption, and it's undesirable for the OOM killer to ignore
this intention and compare members across these boundaries.

We should go the same way here as with kill_alloc_task: the default
should be what's sane and expected by the vast majority of our users,
with a knob (I would prefer a sysctl here, actually) to switch back in
case somebody - unexpectedly - actually relies on the old behavior.

I don't see why couldn't change user-visible behavior here if we don't
expect anyone (quirky setups aside) to rely on it AND we provide a
knob to revert in the field for the exceptions.

[toc] | [prev] | [next] | [standalone]

#1727214

On Tue 05-09-17 17:53:44, Johannes Weiner wrote:
> On Tue, Sep 05, 2017 at 03:44:12PM +0200, Michal Hocko wrote:
> > Why is this an opt out rather than opt-in? IMHO the original oom logic
> > should be preserved by default and specific workloads should opt in for
> > the cgroup aware logic. Changing the global behavior depending on
> > whether cgroup v2 interface is in use is more than unexpected and IMHO
> > wrong approach to take. I think we should instead go with 
> > oom_strategy=[alloc_task,biggest_task,cgroup]
> > 
> > we currently have alloc_task (via sysctl_oom_kill_allocating_task) and
> > biggest_task which is the default. You are adding cgroup and the more I
> > think about the more I agree that it doesn't really make sense to try to
> > fit thew new semantic into the existing one (compare tasks to kill-all
> > memcgs). Just introduce a new strategy and define a new semantic from
> > scratch. Memcg priority and kill-all are a natural extension of this new
> > strategy. This will make the life easier and easier to understand by
> > users.
> 
> oom_kill_allocating_task is actually a really good example of why
> cgroup-awareness *should* be the new default.
> 
> Before we had the oom killer victim selection, we simply killed the
> faulting/allocating task. While a valid answer to the problem, it's
> not very fair or representative of what the user wants or intends.
> 
> Then we added code to kill the biggest offender instead, which should
> have been the case from the start and was hence made the new default.
> The oom_kill_allocating_task was added on the off-chance that there
> might be setups who, for historical reasons, rely on the old behavior.
> But our default was chosen based on what behavior is fair, expected,
> and most reflective of the user's intentions.

I am not sure this is how things evolved actually. This is way before
my time so my git log interpretation might be imprecise. We do have
oom_badness heuristic since out_of_memory has been introduced and
oom_kill_allocating_task has been introduced much later because of large
boxes with zillions of tasks (SGI I suspect) which took too long to
select a victim so David has added this heuristic.
 
> The cgroup-awareness in the OOM killer is exactly the same thing. It
> should have been the default from the beginning, because the user
> configures a group of tasks to be an interdependent, terminal unit of
> memory consumption, and it's undesirable for the OOM killer to ignore
> this intention and compare members across these boundaries.

I would agree if that was true in general. I can completely see how the
cgroup awareness is useful in e.g. containerized environments (especially
with kill-all enabled) but memcgs are used in a large variety of
usecases and I cannot really say all of them really demand the new
semantic. Say I have a workload which doesn't want to see reclaim
interference from others on the same machine. Why should I kill a
process from that particular memcg just because it is the largest one
when there is a memory hog/leak outside of this memcg?

From my point of view the safest (in a sense of the least surprise)
way to go with opt-in for the new heuristic. I am pretty sure all who
would benefit from the new behavior will enable it while others will not
regress in unexpected way.

We can talk about the way _how_ to control these oom strategies, of
course. But I would be really reluctant to change the default which is
used for years and people got used to it.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1728286

On Wed, Sep 06, 2017 at 10:28:59AM +0200, Michal Hocko wrote:
> On Tue 05-09-17 17:53:44, Johannes Weiner wrote:
> > The cgroup-awareness in the OOM killer is exactly the same thing. It
> > should have been the default from the beginning, because the user
> > configures a group of tasks to be an interdependent, terminal unit of
> > memory consumption, and it's undesirable for the OOM killer to ignore
> > this intention and compare members across these boundaries.
> 
> I would agree if that was true in general. I can completely see how the
> cgroup awareness is useful in e.g. containerized environments (especially
> with kill-all enabled) but memcgs are used in a large variety of
> usecases and I cannot really say all of them really demand the new
> semantic. Say I have a workload which doesn't want to see reclaim
> interference from others on the same machine. Why should I kill a
> process from that particular memcg just because it is the largest one
> when there is a memory hog/leak outside of this memcg?

Sure, it's always possible to come up with a config for which this
isn't the optimal behavior. But this is about picking a default that
makes sense to most users, and that type of cgroup usage just isn't
the common case.

> From my point of view the safest (in a sense of the least surprise)
> way to go with opt-in for the new heuristic. I am pretty sure all who
> would benefit from the new behavior will enable it while others will not
> regress in unexpected way.

This thinking simply needs to be balanced against the need to make an
unsurprising and consistent final interface.

The current behavior breaks isolation by letting tasks in different
cgroups compete with each other during an OOM kill. While you can
rightfully argue that it's possible for usecases to rely on this, you
cannot tell me that this is the least-surprising thing we can offer
users; certainly not new users, but also not many/most existing ones.

> We can talk about the way _how_ to control these oom strategies, of
> course. But I would be really reluctant to change the default which is
> used for years and people got used to it.

I really doubt there are many cgroup users that rely on that
particular global OOM behavior.

We have to agree to disagree, I guess.

[toc] | [prev] | [next] | [standalone]

#1728296

On Wed, 6 Sep 2017, Michal Hocko wrote:

> I am not sure this is how things evolved actually. This is way before
> my time so my git log interpretation might be imprecise. We do have
> oom_badness heuristic since out_of_memory has been introduced and
> oom_kill_allocating_task has been introduced much later because of large
> boxes with zillions of tasks (SGI I suspect) which took too long to
> select a victim so David has added this heuristic.

Nope. The logic was required for tasks that run out of memory when the
restriction on the allocation did not allow the use of all of memory.
cpuset restrictions and memory policy restrictions where the prime
considerations at the time.

It has *nothing* to do with zillions of tasks. Its amusing that the SGI
ghost is still haunting the discussion here. The company died a couple of
years ago finally (ok somehow HP has an "SGI" brand now I believe). But
there are multiple companies that have large NUMA configurations and they
all have configurations where they want to restrict allocations of a
process to subset of system memory. This is even more important now that
we get new forms of memory (NVDIMM, PCI-E device memory etc). You need to
figure out what to do with allocations that fail because the *allowed*
memory pools are empty.

[toc] | [prev] | [next] | [standalone]

#1728468

On Thu, 7 Sep 2017, Christopher Lameter wrote:

> > I am not sure this is how things evolved actually. This is way before
> > my time so my git log interpretation might be imprecise. We do have
> > oom_badness heuristic since out_of_memory has been introduced and
> > oom_kill_allocating_task has been introduced much later because of large
> > boxes with zillions of tasks (SGI I suspect) which took too long to
> > select a victim so David has added this heuristic.
> 
> Nope. The logic was required for tasks that run out of memory when the
> restriction on the allocation did not allow the use of all of memory.
> cpuset restrictions and memory policy restrictions where the prime
> considerations at the time.
> 
> It has *nothing* to do with zillions of tasks. Its amusing that the SGI
> ghost is still haunting the discussion here. The company died a couple of
> years ago finally (ok somehow HP has an "SGI" brand now I believe). But
> there are multiple companies that have large NUMA configurations and they
> all have configurations where they want to restrict allocations of a
> process to subset of system memory. This is even more important now that
> we get new forms of memory (NVDIMM, PCI-E device memory etc). You need to
> figure out what to do with allocations that fail because the *allowed*
> memory pools are empty.
> 

We already had CONSTRAINT_CPUSET at the time, this was requested by Paul 
and acked by him in https://marc.info/?l=linux-mm&m=118306851418425.

[toc] | [prev] | [next] | [standalone]

#1729292

On Thu, 7 Sep 2017, David Rientjes wrote:

> > It has *nothing* to do with zillions of tasks. Its amusing that the SGI
> > ghost is still haunting the discussion here. The company died a couple of
> > years ago finally (ok somehow HP has an "SGI" brand now I believe). But
> > there are multiple companies that have large NUMA configurations and they
> > all have configurations where they want to restrict allocations of a
> > process to subset of system memory. This is even more important now that
> > we get new forms of memory (NVDIMM, PCI-E device memory etc). You need to
> > figure out what to do with allocations that fail because the *allowed*
> > memory pools are empty.
> >
>
> We already had CONSTRAINT_CPUSET at the time, this was requested by Paul
> and acked by him in https://marc.info/?l=linux-mm&m=118306851418425.

Ok. Certainly there were scalability issues (lots of them) and the sysctl
may have helped there if set globally. But the ability to kill the
allocating tasks was primarily used in cpusets for constrained allocation.

The issue of scaling is irrelevant in the context of deciding what to do
about the sysctl. You can address the issue differently if it still
exists. The systems with super high NUMA nodes (hundreds to a
thousand) have somehow fallen out of fashion a bit. So I doubt that this
is still an issue. And no one of the old stakeholders is speaking up.

What is the current approach for an OOM occuring in a cpuset or cgroup
with a restricted numa node set?

[toc] | [prev] | [next] | [standalone]

#1729409

On Fri, 8 Sep 2017, Christopher Lameter wrote:

> Ok. Certainly there were scalability issues (lots of them) and the sysctl
> may have helped there if set globally. But the ability to kill the
> allocating tasks was primarily used in cpusets for constrained allocation.
> 

I remember discussing it with him and he had some data with pretty extreme 
numbers for how long the tasklist iteration was taking.  Regardless, I 
agree it's not pertinent to the discussion if anybody is actively using 
the sysctl, just fun to try to remember the discussions from 10 years ago.  

The problem I'm having with the removal, though, is that the kernel source 
actually uses it itself in tools/testing/fault-injection/failcmd.sh.  
That, to me, suggests there are people outside the kernel source that are 
also probably use it.  We use it as part of our unit testing, although we 
could convert away from it.

These are things that can probably be worked around, but I'm struggling to 
see the whole benefit of it.  It's only defined, there's generic sysctl 
handling, and there's a single conditional in the oom killer.  I wouldn't 
risk the potential userspace breakage.

> The issue of scaling is irrelevant in the context of deciding what to do
> about the sysctl. You can address the issue differently if it still
> exists. The systems with super high NUMA nodes (hundreds to a
> thousand) have somehow fallen out of fashion a bit. So I doubt that this
> is still an issue. And no one of the old stakeholders is speaking up.
> 
> What is the current approach for an OOM occuring in a cpuset or cgroup
> with a restricted numa node set?
> 

It's always been shaky, we simply exclude potential kill victims based on 
whether or not they share mempolicy nodes or cpuset mems with the 
allocating process.  Of course, this could result in no memory freeing 
because a potential victim being allowed to allocate on a particular node 
right now doesn't mean killing it will free memory on that node.  It's 
just more probable in practice.  Nobody has complained about that 
methodology, but we do have internal code that simply kills current for 
mempolicy ooms.  That is because we have priority based oom killing much 
like this patchset implements and then extends it even further to 
processes.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web