Date:   Wed, 02 May 2018 09:05:05 -0500
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     Johannes Weiner <hannes@...xchg.org>
Cc:     Michal Hocko <mhocko@...nel.org>,
        Kirill Tkhai <ktkhai@...tuozzo.com>, akpm@...ux-foundation.org,
        peterz@...radead.org, oleg@...hat.com, viro@...iv.linux.org.uk,
        mingo@...nel.org, paulmck@...ux.vnet.ibm.com,
        keescook@...omium.org, riel@...hat.com, tglx@...utronix.de,
        kirill.shutemov@...ux.intel.com, marcos.souza.org@...il.com,
        hoeun.ryu@...il.com, pasha.tatashin@...cle.com, gs051095@...il.com,
        dhowells@...hat.com, rppt@...ux.vnet.ibm.com,
        linux-kernel@...r.kernel.org,
        Balbir Singh <balbir@...ux.vnet.ibm.com>,
        Tejun Heo <tj@...nel.org>
Subject: Re: [RFC][PATCH] memcg: Replace mm->owner with mm->memcg

Johannes Weiner <hannes@...xchg.org> writes:

> Hi Eric,
>
> On Wed, May 02, 2018 at 10:47:08AM +0200, Michal Hocko wrote:
>> [CC johannes and Tejun as well. I am sorry but my backlog is so huge I
>> will not get to this week.]
>> 
>> On Tue 01-05-18 12:35:16, Eric W. Biederman wrote:
>> > Recently it was reported that mm_update_next_owner could get into
>> > cases where it was executing it's fallback for_each_process part of
>> > the loop and thus taking up a lot of time.
>> > 
>> > To deal with this replace mm->owner with mm->memcg.  This just reduces
>> > the complexity of everything.  As much as possible I have maintained
>> > the current semantics.  There are two siginificant exceptions.  During
>> > fork the memcg of the process calling fork is charged rather than
>> > init_css_set.  During memory cgroup migration the charges are migrated
>> > not if the process is the owner of the mm, but if the process being
>> > migrated has the same memory cgroup as the mm.
>> > 
>> > I believe it was a bug if init_css_set is charged for memory activity
>> > during fork, and the old behavior was simply a consequence of the new
>> > task not having tsk->cgroup not initialized to it's proper cgroup.
>> > 
>> > Durhing cgroup migration only thread group leaders are allowed to
>> > migrate.  Which means in practice there should only be one.  Linux
>> > tasks created with CLONE_VM are the only exception, but the common
>> > cases are already ruled out.  Processes created with vfork have a
>> > suspended parent and can do nothing but call exec so they should never
>> > show up.  Threads of the same cgroup are not the thread group leader
>> > so also should not show up.  That leaves the old LinuxThreads library
>> > which is probably out of use by now, and someone doing something very
>> > creative with cgroups, and rolling their own threads with CLONE_VM.
>> > So in practice I don't think the difference charge migration will
>> > affect anyone.
>> > 
>> > To ensure that mm->memcg is updated appropriately I have implemented
>> > cgroup "attach" and "fork" methods.  This ensures that at those
>> > points the mm pointed to the task has the appropriate memory cgroup.
>> > 
>> > For simplicity instead of introducing a new mm lock I simply use
>> > exchange on the pointer where the mm->memcg is updated to get
>> > atomic updates.
>> > 
>> > Looking at the history effectively this change is a revert.  The
>> > reason given for adding mm->owner is so that multiple cgroups can be
>> > attached to the same mm.  In the last 8 years a second user of
>> > mm->owner has not appeared.  A feature that has never used, makes the
>> > code more complicated and has horrible worst case performance should
>> > go.
>> > 
>> > Fixes: cf475ad28ac3 ("cgroups: add an owner to the mm_struct")
>> > Reported-by:  Kirill Tkhai <ktkhai@...tuozzo.com>
>> > Signed-off-by: "Eric W. Biederman" <ebiederm@...ssion.com>
>
> I really like this. The multiple css per mm future that was referenced
> in cf475ad28ac3's changelog never materialized, so there is no need to
> always go through the task just to have the full css_set.
>
>> > @@ -4827,15 +4813,16 @@ static int mem_cgroup_can_attach(struct cgroup_taskset *tset)
>> >  	if (!move_flags)
>> >  		return 0;
>> >  
>> > -	from = mem_cgroup_from_task(p);
>> > +	from = mem_cgroup_from_css(task_css(p, memory_cgrp_id));
>> >  
>> >  	VM_BUG_ON(from == memcg);
>> >  
>> >  	mm = get_task_mm(p);
>> >  	if (!mm)
>> >  		return 0;
>> > +
>> >  	/* We move charges only when we move a owner of the mm */
>> > -	if (mm->owner == p) {
>> > +	if (mm->memcg == from) {
>> >  		VM_BUG_ON(mc.from);
>> >  		VM_BUG_ON(mc.to);
>> >  		VM_BUG_ON(mc.precharge);
>
> mm->memcg is updated on every single ->attach(), so this can only
> happen to a task when a CLONE_VM sibling is moved out of its
> group. Essentially, in the new scheme the "owner" is whichever task
> with that mm migrated most recently.

Yes.  The charges will only fail to be migrated in some CLONE_VM
situations.

> I agree that it's hard to conjure up a practical usecase that would
> straddle mms like this over multiple cgroups - especially given how
> the memory charges themselves can only belong to one cgroup, too. So
> in practice task->mm->memcg will always be task->css_set[memory].
>
> But could you please update the comment to outline the cornercase?
> "owner" isn't really a thing anymore after this patch.

I can.  How about:
	/* We move charges except for creative uses of CLONE_VM */

> Oh, and mm/debug.c::dump_mm() still refers to mm->owner, that needs to
> be switched to mm->memcg as well.

The kbuild test robot pointed that out as well.

> Aside from that, this looks great to me. For the fixed version:
>
> Acked-by: Johannes Weiner <hannes@...xchg.org>

Will do and will repost in a bit.

Eric

Powered by blists - more mailing lists