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

Groups > linux.kernel > #1701134 > unrolled thread

Re: [v4 2/4] mm, oom: 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: [v4 2/4] mm, oom: cgroup-aware OOM killer Michal Hocko <mhocko@kernel.org> - 2017-08-01 17:00 +0200
    Re: [v4 2/4] mm, oom: cgroup-aware OOM killer Michal Hocko <mhocko@kernel.org> - 2017-08-01 19:10 +0200
      Re: [v4 2/4] mm, oom: cgroup-aware OOM killer Michal Hocko <mhocko@kernel.org> - 2017-08-02 09:30 +0200
        Re: [v4 2/4] mm, oom: cgroup-aware OOM killer Michal Hocko <mhocko@kernel.org> - 2017-08-03 15:10 +0200
    Re: [v4 2/4] mm, oom: cgroup-aware OOM killer David Rientjes <rientjes@google.com> - 2017-08-09 01:10 +0200

#1701134 — Re: [v4 2/4] mm, oom: cgroup-aware OOM killer

On Wed 26-07-17 14:27:16, Roman Gushchin wrote:
[...]
> +static long memcg_oom_badness(struct mem_cgroup *memcg,
> +			      const nodemask_t *nodemask)
> +{
> +	long points = 0;
> +	int nid;
> +
> +	for_each_node_state(nid, N_MEMORY) {
> +		if (nodemask && !node_isset(nid, *nodemask))
> +			continue;
> +
> +		points += mem_cgroup_node_nr_lru_pages(memcg, nid,
> +				LRU_ALL_ANON | BIT(LRU_UNEVICTABLE));
> +	}
> +
> +	points += memcg_page_state(memcg, MEMCG_KERNEL_STACK_KB) /
> +		(PAGE_SIZE / 1024);
> +	points += memcg_page_state(memcg, NR_SLAB_UNRECLAIMABLE);
> +	points += memcg_page_state(memcg, MEMCG_SOCK);
> +	points += memcg_page_state(memcg, MEMCG_SWAP);
> +
> +	return points;

I am wondering why are you diverging from the global oom_badness
behavior here. Although doing per NUMA accounting sounds like a better
idea but then you just end up mixing this with non NUMA numbers and the
whole thing is harder to understand without great advantages.

> +static void select_victim_memcg(struct mem_cgroup *root, struct oom_control *oc)
> +{
> +	struct mem_cgroup *iter, *parent;
> +
> +	for_each_mem_cgroup_tree(iter, root) {
> +		if (memcg_has_children(iter)) {
> +			iter->oom_score = 0;
> +			continue;
> +		}
> +
> +		iter->oom_score = oom_evaluate_memcg(iter, oc->nodemask);
> +		if (iter->oom_score == -1) {
> +			oc->chosen_memcg = (void *)-1UL;
> +			mem_cgroup_iter_break(root, iter);
> +			return;
> +		}
> +
> +		if (!iter->oom_score)
> +			continue;
> +
> +		for (parent = parent_mem_cgroup(iter); parent && parent != root;
> +		     parent = parent_mem_cgroup(parent))
> +			parent->oom_score += iter->oom_score;
> +	}
> +
> +	for (;;) {
> +		struct cgroup_subsys_state *css;
> +		struct mem_cgroup *memcg = NULL;
> +		long score = LONG_MIN;
> +
> +		css_for_each_child(css, &root->css) {
> +			struct mem_cgroup *iter = mem_cgroup_from_css(css);
> +
> +			if (iter->oom_score > score) {
> +				memcg = iter;
> +				score = iter->oom_score;
> +			}
> +		}
> +
> +		if (!memcg) {
> +			if (oc->memcg && root == oc->memcg) {
> +				oc->chosen_memcg = oc->memcg;
> +				css_get(&oc->chosen_memcg->css);
> +				oc->chosen_points = oc->memcg->oom_score;
> +			}
> +			break;
> +		}
> +
> +		if (memcg->oom_kill_all_tasks || !memcg_has_children(memcg)) {
> +			oc->chosen_memcg = memcg;
> +			css_get(&oc->chosen_memcg->css);
> +			oc->chosen_points = score;
> +			break;
> +		}
> +
> +		root = memcg;
> +	}
> +}

This and the rest of the victim selection code is really hairy and hard
to follow.

I would reap out the oom_kill_process into a separate patch.

> -static void oom_kill_process(struct oom_control *oc, const char *message)
> +static void __oom_kill_process(struct task_struct *victim)

To the rest of the patch. I have to say I do not quite like how it is
implemented. I was hoping for something much simpler which would hook
into oom_evaluate_task. If a task belongs to a memcg with kill-all flag
then we would update the cumulative memcg badness (more specifically the
badness of the topmost parent with kill-all flag). Memcg will then
compete with existing self contained tasks (oom_badness will have to
tell whether points belong to a task or a memcg to allow the caller to
deal with it). But it shouldn't be much more complex than that.

Or is there something that I am missing and that would prevent such a
simple approach?
-- 
Michal Hocko
SUSE Labs

[toc] | [next] | [standalone]

#1701251

On Tue 01-08-17 16:25:48, Roman Gushchin wrote:
> On Tue, Aug 01, 2017 at 04:54:35PM +0200, Michal Hocko wrote:
[...]
> > I would reap out the oom_kill_process into a separate patch.
> 
> It was a separate patch, I've merged it based on Vladimir's feedback.
> No problems, I can divide it back.

It would make the review slightly more easier
> 
> > > -static void oom_kill_process(struct oom_control *oc, const char *message)
> > > +static void __oom_kill_process(struct task_struct *victim)
> > 
> > To the rest of the patch. I have to say I do not quite like how it is
> > implemented. I was hoping for something much simpler which would hook
> > into oom_evaluate_task. If a task belongs to a memcg with kill-all flag
> > then we would update the cumulative memcg badness (more specifically the
> > badness of the topmost parent with kill-all flag). Memcg will then
> > compete with existing self contained tasks (oom_badness will have to
> > tell whether points belong to a task or a memcg to allow the caller to
> > deal with it). But it shouldn't be much more complex than that.
> 
> I'm not sure, it will be any simpler. Basically I'm doing the same:
> the difference is that you want to iterate over tasks and for each
> task traverse the memcg tree, update per-cgroup oom score and find
> the corresponding memcg(s) with the kill-all flag. I'm doing the opposite:
> traverse the cgroup tree, and for each leaf cgroup iterate over processes.

Yeah but this doesn't fit very well to the existing scheme so we would
need two different schemes which is not ideal from maint. point of view.
We also do not have to duplicate all the tricky checks we already do in
oom_evaluate_task. So I would prefer if we could try to hook there and
do the special handling there.

> Also, please note, that even without the kill-all flag the decision is made
> on per-cgroup level (except tasks in the root cgroup).

Yeah and I am not sure this is a reasonable behavior. Why should we
consider memcgs which are not kill-all as a single entity?

-- 
Michal Hocko
SUSE Labs

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

#1701786

On Tue 01-08-17 19:13:52, Roman Gushchin wrote:
> On Tue, Aug 01, 2017 at 07:03:03PM +0200, Michal Hocko wrote:
> > On Tue 01-08-17 16:25:48, Roman Gushchin wrote:
> > > On Tue, Aug 01, 2017 at 04:54:35PM +0200, Michal Hocko wrote:
> > [...]
> > > > I would reap out the oom_kill_process into a separate patch.
> > > 
> > > It was a separate patch, I've merged it based on Vladimir's feedback.
> > > No problems, I can divide it back.
> > 
> > It would make the review slightly more easier
> > > 
> > > > > -static void oom_kill_process(struct oom_control *oc, const char *message)
> > > > > +static void __oom_kill_process(struct task_struct *victim)
> > > > 
> > > > To the rest of the patch. I have to say I do not quite like how it is
> > > > implemented. I was hoping for something much simpler which would hook
> > > > into oom_evaluate_task. If a task belongs to a memcg with kill-all flag
> > > > then we would update the cumulative memcg badness (more specifically the
> > > > badness of the topmost parent with kill-all flag). Memcg will then
> > > > compete with existing self contained tasks (oom_badness will have to
> > > > tell whether points belong to a task or a memcg to allow the caller to
> > > > deal with it). But it shouldn't be much more complex than that.
> > > 
> > > I'm not sure, it will be any simpler. Basically I'm doing the same:
> > > the difference is that you want to iterate over tasks and for each
> > > task traverse the memcg tree, update per-cgroup oom score and find
> > > the corresponding memcg(s) with the kill-all flag. I'm doing the opposite:
> > > traverse the cgroup tree, and for each leaf cgroup iterate over processes.
> > 
> > Yeah but this doesn't fit very well to the existing scheme so we would
> > need two different schemes which is not ideal from maint. point of view.
> > We also do not have to duplicate all the tricky checks we already do in
> > oom_evaluate_task. So I would prefer if we could try to hook there and
> > do the special handling there.
> 
> I hope, that iterating over all tasks just to check if there are
> in-flight OOM victims might be optimized at some point.
> That means, we would be able to choose a victim much cheaper.
> It's not easy, but it feels as a right direction to go.

You would have to count per each oom domain and that sounds quite
unfeasible to me.
 
> Also, adding new tricks to the oom_evaluate_task() will make the code
> even more hairy. Some of the existing tricks are useless for memcg selection.

Not sure what you mean but oom_evaluate_task has been usable for both
global and memcg oom paths so far. I do not see any reason why this
shouldn't hold for a different oom killing strategy.

> > > Also, please note, that even without the kill-all flag the decision is made
> > > on per-cgroup level (except tasks in the root cgroup).
> > 
> > Yeah and I am not sure this is a reasonable behavior. Why should we
> > consider memcgs which are not kill-all as a single entity?
> 
> I think, it's reasonable to choose a cgroup/container to blow off based on
> the cgroup oom_priority/size (including hierarchical settings), and then
> kill one biggest or all tasks depending on cgroup settings.

But that doesn't mean you have to treat even !kill-all memcgs like a
single entity. In fact we should compare killable entities which is
either a task or the whole memcg if configured that way.
-- 
Michal Hocko
SUSE Labs

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

#1703089

On Thu 03-08-17 13:47:51, Roman Gushchin wrote:
> On Wed, Aug 02, 2017 at 09:29:01AM +0200, Michal Hocko wrote:
> > On Tue 01-08-17 19:13:52, Roman Gushchin wrote:
> > > On Tue, Aug 01, 2017 at 07:03:03PM +0200, Michal Hocko wrote:
> > > > On Tue 01-08-17 16:25:48, Roman Gushchin wrote:
> > > > > On Tue, Aug 01, 2017 at 04:54:35PM +0200, Michal Hocko wrote:
> > > > [...]
> > > > > > I would reap out the oom_kill_process into a separate patch.
> > > > > 
> > > > > It was a separate patch, I've merged it based on Vladimir's feedback.
> > > > > No problems, I can divide it back.
> > > > 
> > > > It would make the review slightly more easier
> > > > > 
> > > > > > > -static void oom_kill_process(struct oom_control *oc, const char *message)
> > > > > > > +static void __oom_kill_process(struct task_struct *victim)
> > > > > > 
> > > > > > To the rest of the patch. I have to say I do not quite like how it is
> > > > > > implemented. I was hoping for something much simpler which would hook
> > > > > > into oom_evaluate_task. If a task belongs to a memcg with kill-all flag
> > > > > > then we would update the cumulative memcg badness (more specifically the
> > > > > > badness of the topmost parent with kill-all flag). Memcg will then
> > > > > > compete with existing self contained tasks (oom_badness will have to
> > > > > > tell whether points belong to a task or a memcg to allow the caller to
> > > > > > deal with it). But it shouldn't be much more complex than that.
> > > > > 
> > > > > I'm not sure, it will be any simpler. Basically I'm doing the same:
> > > > > the difference is that you want to iterate over tasks and for each
> > > > > task traverse the memcg tree, update per-cgroup oom score and find
> > > > > the corresponding memcg(s) with the kill-all flag. I'm doing the opposite:
> > > > > traverse the cgroup tree, and for each leaf cgroup iterate over processes.
> > > > 
> > > > Yeah but this doesn't fit very well to the existing scheme so we would
> > > > need two different schemes which is not ideal from maint. point of view.
> > > > We also do not have to duplicate all the tricky checks we already do in
> > > > oom_evaluate_task. So I would prefer if we could try to hook there and
> > > > do the special handling there.
> > > 
> > > I hope, that iterating over all tasks just to check if there are
> > > in-flight OOM victims might be optimized at some point.
> > > That means, we would be able to choose a victim much cheaper.
> > > It's not easy, but it feels as a right direction to go.
> > 
> > You would have to count per each oom domain and that sounds quite
> > unfeasible to me.
> 
> It's hard, but traversing the whole cgroup tree from bottom to top
> for each task is just not scalable.

We are talking about the oom path which is a slow path. Besides that
memcg hierarchies will not be very deep usually (we are not talking
about hundreds). 

> This is exactly why I've choosen a compromise right now: let's
> iterate over all tasks, but do it by iterating over the cgroup tree.
> 
> >  
> > > Also, adding new tricks to the oom_evaluate_task() will make the code
> > > even more hairy. Some of the existing tricks are useless for memcg selection.
> > 
> > Not sure what you mean but oom_evaluate_task has been usable for both
> > global and memcg oom paths so far. I do not see any reason why this
> > shouldn't hold for a different oom killing strategy.
> 
> Yes, but in both cases we've evaluated tasks, not cgroups.
> 
> > 
> > > > > Also, please note, that even without the kill-all flag the decision is made
> > > > > on per-cgroup level (except tasks in the root cgroup).
> > > > 
> > > > Yeah and I am not sure this is a reasonable behavior. Why should we
> > > > consider memcgs which are not kill-all as a single entity?
> > > 
> > > I think, it's reasonable to choose a cgroup/container to blow off based on
> > > the cgroup oom_priority/size (including hierarchical settings), and then
> > > kill one biggest or all tasks depending on cgroup settings.
> > 
> > But that doesn't mean you have to treat even !kill-all memcgs like a
> > single entity. In fact we should compare killable entities which is
> > either a task or the whole memcg if configured that way.
> 
> I believe it's absolutely valid user's intention to prioritize some
> cgroups over other, even if only one task should be killed in case of OOM.

This is mixing different concepts which I really dislike. The semantic
is getting really fuzzy. How are you going to apply memcg priority when
you are killing a single task? What portion of the memcgs priority does
the task get? Then you have that root is special...

No I really dislike this. We should start simple and compare killable
entities. If you want to apply memcg priority then only on the whole
memcgs.
-- 
Michal Hocko
SUSE Labs

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

#1706932

On Tue, 1 Aug 2017, Roman Gushchin wrote:

> > To the rest of the patch. I have to say I do not quite like how it is
> > implemented. I was hoping for something much simpler which would hook
> > into oom_evaluate_task. If a task belongs to a memcg with kill-all flag
> > then we would update the cumulative memcg badness (more specifically the
> > badness of the topmost parent with kill-all flag). Memcg will then
> > compete with existing self contained tasks (oom_badness will have to
> > tell whether points belong to a task or a memcg to allow the caller to
> > deal with it). But it shouldn't be much more complex than that.
> 
> I'm not sure, it will be any simpler. Basically I'm doing the same:
> the difference is that you want to iterate over tasks and for each
> task traverse the memcg tree, update per-cgroup oom score and find
> the corresponding memcg(s) with the kill-all flag. I'm doing the opposite:
> traverse the cgroup tree, and for each leaf cgroup iterate over processes.
> 
> Also, please note, that even without the kill-all flag the decision is made
> on per-cgroup level (except tasks in the root cgroup).
> 

I think your implementation is preferred and is actually quite simple to 
follow, and I would encourage you to follow through with it.  It has a 
similar implementation to what we have done for years to kill a process 
from a leaf memcg.

I did notice that oom_kill_memcg_victim() calls directly into 
__oom_kill_process(), however, so we lack the traditional oom killer 
output that shows memcg usage and potential tasklist.  I think we should 
still be dumping this information to the kernel log so that we can see a 
breakdown of charged memory.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web