Re: [RFC][PATCH 1/2] memcg: Ensure every task that uses an mm is in the same memory cgroup

From: Michal Hocko <mhocko@kernel.org>
To: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Johannes Weiner <hannes@cmpxchg.org>,
	Kirill Tkhai <ktkhai@virtuozzo.com>,
	peterz@infradead.org, viro@zeniv.linux.org.uk, mingo@kernel.org,
	paulmck@linux.vnet.ibm.com, keescook@chromium.org,
	riel@redhat.com, tglx@linutronix.de,
	kirill.shutemov@linux.intel.com, marcos.souza.org@gmail.com,
	hoeun.ryu@gmail.com, pasha.tatashin@oracle.com,
	gs051095@gmail.com, dhowells@redhat.com, rppt@linux.vnet.ibm.com,
	linux-kernel@vger.kernel.org,
	Balbir Singh <balbir@linux.vnet.ibm.com>,
	Tejun Heo <tj@kernel.org>, Oleg Nesterov <oleg@redhat.com>
Subject: Re: [RFC][PATCH 1/2] memcg: Ensure every task that uses an mm is in the same memory cgroup
Date: Thu, 7 Jun 2018 14:19:13 +0200	[thread overview]
Message-ID: <20180607121913.GO32433@dhcp22.suse.cz> (raw)
In-Reply-To: <87wova6cw6.fsf@xmission.com>

On Thu 07-06-18 06:42:49, Eric W. Biederman wrote:
> Michal Hocko <mhocko@kernel.org> writes:
[...]
> > Btw. MMF_ALIEN_MM could be used in the OOM proper as well.
> 
> There are two big issues I see with your suggested alternative.
> 1) cgroupv1 the task interface.  We still need to deny migrating
>    part of a thread group.

Hmm. Can we simply check the migrating tsk to be the thread group
leader in can_attach and keep the silent migration skip?

> 2) vfork.  That uses CLONE_MM and it gets used.

Right. MMF_ALIEN_MM would need a better check. I've forgot about this
case.
+		if (unlikely(!(clone_flags & (CLONE_THREAD|CLONE_VFORK)))
+			set_bit(MMF_ALIEN_MM, &mm->flags);

this would still allow to migrate mm via vforked task which is wrong
so can_attach would have to special case vforked task as well. Which is,
ehm, fuggly but that is something we have to handle regardless of
MMF_ALIEN_MM... I will have to check your other patch but I suspect you
haven't done that. Btw. neither did I when trying to work on this
previously.

>    At a quick look
>    I am seeing gcc, g++, cpp, emacs24, strace, calendar, nm, telnet, gdb,
>    and several other programs I don't recognize.
> 
> I believe your proposal will prevent onlining the memcgroup if there

did you mean s@onlining@offlining@ ?

> is a compile running, because of failure to support vfork.
> 
> Further I expect we would need a count rather than a bit that gets set
> and never gets cleared.  Or else even when the mm is no longer shared by
> vfork we will still think it is.

Yes, but does that actually matter all that much to warrant an
additional complexity? If somebody uses the funny threading model and
want to migrate then the failure will likely get noticed regardless
whether there is one or more processes sharing the mm. One failing while
other succeeding sounds confusing.

> Michal do you have an opinion on my previous patch?

I guess you mean http://lkml.kernel.org/r/87wovj8e1d.fsf_-_@xmission.com
Yes, I definitely plan to review this. I just need to find some time to
focus on this without being interrupted by many other small things.

> I just want to make certain that this fun work does not get all of the
> attention, and the bug fix actually gets reviewed.

Sure thing!
-- 
Michal Hocko
SUSE Labs