Re: [PATCH 0/2] mm->owner to mm->memcg fixes

[Michal Hocko]

On Wed 23-05-18 14:46:43, Eric W. Biederman wrote:
> Michal Hocko <mhocko@xxxxxxxxxx> writes:
> 
> > On Thu 10-05-18 14:14:18, Michal Hocko wrote:
> >> On Fri 04-05-18 12:26:03, Eric W. Biederman wrote:
> >> > 
> >> > Andrew can you pick up these two fixes as well.
> >> > 
> >> > These address the issues Michal Hocko and Oleg Nesterov noticed.
> >> 
> >> I completely got lost in this thread. There are way too many things
> >> discussed at once. Could you repost the full series for a proper review
> >> please?
> >
> > ping
> 
> Quick summary of where things stand.  (Just getting back from vacation
> and catching up with things).
> 
> Andrew has picked up the best version of these patches and you can look
> at his tree to find the current patches.

I would really prefer and appreciate a repost with all the fixes folded
in. Wrapping my head around -fix+ is not something I have time for.

> Looking at my tree the issues that have been looked at above
> the basic patch are:
> !CONFIG_MMU support (code motion)
> Races during exec.  (Roughly solved but I think we can do better by
>                      expanding the scope of
>                      cgroup_threadgroup_change_begin/end in exec and
>                      just making exec atomic wrt to cgroup changes)
> 
> While I was on vacation you posted an old concern about a condtion
> where get_mem_cgroup_from_mm was not guaranteed to complete, and how
> that interacts with charge migration.
> 
> 
> Looking at your description the concern is that cgroup_rmdir can succeed
> when a cgroup has just an mm in it (and no tasks).  The result  is
> that mem_cgroup_try_charge can loop indefinitely in that case.

right

> That is possible with two processes sharing the same mm, but living in
> different memory cgroups.

right

> That is a silly useless configuration but
> something to support at least to the level of making certain kernel's
> don't wedge when it happens by accident or maliciously.

absolutely. Processes sharing the mm without being in the same thread
group is something we should have never allowed IMHO. It just generates
a lot of corner cases. I guess this is a relict from old threading
models so here we are.

> The suggestion of having cgroup_is_populated take this into account
> seems like a good general solution but I don't think that is strictly
> necessary.
> 
> In the spirit of let's make this work.  How about:

Please, repost with the whole series. I really want to see the full
picture.

Thanks!
-- 
Michal Hocko
SUSE Labs