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

[Michal Hocko]

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