On Fri 01-12-17 13:32:15, Roman Gushchin wrote:
> Hi, Michal!
>
> I totally agree that out_of_memory() function deserves some refactoring.
>
> But I think there is an issue with your patch (see below):
>
> On Fri, Dec 01, 2017 at 10:14:25AM +0100, Michal Hocko wrote:
> > Recently added alloc_pages_before_oomkill gained new caller with this
> > patchset and I think it just grown to deserve a simpler code flow.
> > What do you think about this on top of the series?
> >
> > ---
[...]
> > @@ -1112,13 +1111,8 @@ bool out_of_memory(struct oom_control *oc)
> > }
> >
> > if (mem_cgroup_select_oom_victim(oc)) {
> > - oc->page = alloc_pages_before_oomkill(oc);
> > - if (oc->page) {
> > - if (oc->chosen_memcg &&
> > - oc->chosen_memcg != INFLIGHT_VICTIM)
> > - mem_cgroup_put(oc->chosen_memcg);
>
> You're removing chosen_memcg releasing here, but I don't see where you
> do this instead. And I'm not sure that putting mem_cgroup_put() into
> alloc_pages_before_oomkill() is a way towards simpler code.
Dohh, I though I did. But obviously it is not there.
> I was thinking about a bit larger refactoring: splitting out_of_memory()
> into the following parts (defined as separate functions): victim selection
> (per-process, memcg-aware or just allocating task), last allocation attempt,
> OOM action (kill process, kill memcg, panic). Hopefully it can simplify the things,
> but I don't have code yet.
OK, I will not push if you have further plans of course. This just hit
my eyes...
--
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe cgroups" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
