exit: Make unlikely case in mm_update_next_owner() more scalable

(quoted diff) +3 -1

@@ -464,18 +464,15 @@ void mm_update_next_owner(struct mm_struct *mm)
 	return;
 
 assign_new_owner:
-	BUG_ON(c == p);
 	get_task_struct(c);
+	read_unlock(&tasklist_lock);
+	BUG_ON(c == p);
+

The patch finalizes the series and makes mm_update_next_owner() to iterate over task list using RCU instead of tasklist_lock. This is possible because of rules of inheritance of mm: it may be propagated to a child only, while only kernel thread can obtain someone else's mm via use_mm().

Also, all new tasks are added to tail of tasks list or threads list. The only exception is transfer_pid() in de_thread(), when group leader is replaced by another thread. But transfer_pid() is called in case of successful exec only, where new mm is allocated, so it can't be interesting for mm_update_next_owner().

This patch uses alloc_pid() as a memory barrier, and it's possible since it contains two or more spin_lock()/spin_unlock() pairs. Single pair does not imply a barrier, while two pairs do imply that.

There are three barriers:

1)for_each_process(g) copy_process()

                                   p->mm = mm
    smp_rmb();                     smp_wmb() implied by alloc_pid()
    if (g->flags & PF_KTHREAD)     list_add_tail_rcu(&p->tasks, &init_task.tasks)

2)for_each_thread(g, c) copy_process()

                                   p->mm = mm
    smp_rmb();                     smp_wmb() implied by alloc_pid()
    tmp = READ_ONCE(c->mm)         list_add_tail_rcu(&p->thread_node, ...)

3)for_each_thread(g, c) copy_process()

                                   list_add_tail_rcu(&p->thread_node, ...)
    p->mm != NULL check          do_exit()
    smp_rmb()                      smp_mb();
    get next thread in loop      p->mm = NULL

This patch may be useful for machines with many processes executing. I regulary observe mm_update_next_owner() executing on one of the cpus in crash dumps (not related to this function) on big machines. Even if iteration over task list looks as unlikely situation, it regularity grows with the growth of containers/processes numbers.

Signed-off-by Kirill Tkhai <[email protected]>

---
 kernel/exit.c |   39 +++++++++++++++++++++++++++++++++++----
 kernel/fork.c |    1 +
 kernel/pid.c  |    5 ++++-
 3 files changed, 40 insertions(+), 5 deletions(-)

kernel/exit.c +35 -4

index 40f734ed1193..7ce4cdf96a64 100644
@@ -406,6 +406,8 @@ kill_orphaned_pgrp(struct task_struct *tsk, struct task_struct *parent)
 void mm_update_next_owner(struct mm_struct *mm)
 {
 	struct task_struct *c, *g, *p = current;
+	struct mm_struct *tmp;
+	struct list_head *n;
 
 retry:
 	/*

@@ -440,21 +442,49 @@ void mm_update_next_owner(struct mm_struct *mm)
 		if (c->mm == mm)
 			goto new_owner;
 	}
+	read_unlock(&tasklist_lock);
 
 	/*
 	 * Search through everything else, we should not get here often.
 	 */
+	rcu_read_lock();
 	for_each_process(g) {
+		/*
+		 * g->signal, g->mm and g->flags initialization of a just
+		 * created task must not reorder with linking the task to
+		 * tasks list. Pairs with smp_mb() implied by alloc_pid().
+		 */
+		smp_rmb();
 		if (g->flags & PF_KTHREAD)
 			continue;
 		for_each_thread(g, c) {
-			if (c->mm == mm)
-				goto new_owner;
-			if (c->mm)
+			/*
+			 * Make visible mm of iterated thread.
+			 * Pairs with smp_mb() implied by alloc_pid().
+			 */
+			if (c != g)
+				smp_rmb();
+			tmp = READ_ONCE(c->mm);
+			if (tmp == mm)
+				goto new_owner_nolock;
+			if (likely(tmp))
 				break;
+			n = READ_ONCE(c->thread_node.next);
+			/*
+			 * All mm are NULL, so iterated threads already exited.
+			 * Make sure we see their children.
+			 * Pairs with smp_mb() in do_exit().
+			 */
+			if (n == &g->signal->thread_head)
+				smp_rmb();
 		}
+		/*
+		 * Children of exited thread group are visible due to the above
+		 * smp_rmb(). Threads with mm != NULL can't create a child with
+		 * the mm we're looking for. So, no additional smp_rmb() needed.
+		 */
 	}
-	read_unlock(&tasklist_lock);
+	rcu_read_unlock();
 	/*
 	 * We found no owner yet mm_users > 1: this implies that we are
 	 * most likely racing with swapoff (try_to_unuse()) or /proc or

@@ -466,6 +496,7 @@ void mm_update_next_owner(struct mm_struct *mm)
 new_owner:
 	rcu_read_lock();
 	read_unlock(&tasklist_lock);
+new_owner_nolock:
 	BUG_ON(c == p);
 
 	/*

kernel/fork.c +1 -0

index a5d21c42acfc..2032d4657546 100644
@@ -1805,6 +1805,7 @@ static __latent_entropy struct task_struct *copy_process(
 		goto bad_fork_cleanup_io;
 
 	if (pid != &init_struct_pid) {
+		/* Successfuly returned, this function imply smp_mb() */
 		pid = alloc_pid(p->nsproxy->pid_ns_for_children);
 		if (IS_ERR(pid)) {
 			retval = PTR_ERR(pid);

kernel/pid.c +0 -0

index 157fe4b19971..cb96473aa058 100644

kernel/pid.c +1 -0

+ */
 struct pid *alloc_pid(struct pid_namespace *ns)
 {
 	struct pid *pid;

Hi Kirill,

Quoted 140 lines · Kirill Tkhai

The patch finalizes the series and makes mm_update_next_owner() to iterate over task list using RCU instead of tasklist_lock. This is possible because of rules of inheritance of mm: it may be propagated to a child only, while only kernel thread can obtain someone else's mm via use_mm().

Also, all new tasks are added to tail of tasks list or threads list. The only exception is transfer_pid() in de_thread(), when group leader is replaced by another thread. But transfer_pid() is called in case of successful exec only, where new mm is allocated, so it can't be interesting for mm_update_next_owner().

This patch uses alloc_pid() as a memory barrier, and it's possible since it contains two or more spin_lock()/spin_unlock() pairs. Single pair does not imply a barrier, while two pairs do imply that.

There are three barriers:

1)for_each_process(g) copy_process()

                                   p->mm = mm
    smp_rmb();                     smp_wmb() implied by alloc_pid()
    if (g->flags & PF_KTHREAD)     list_add_tail_rcu(&p->tasks, &init_task.tasks)

2)for_each_thread(g, c) copy_process()

                                   p->mm = mm
    smp_rmb();                     smp_wmb() implied by alloc_pid()
    tmp = READ_ONCE(c->mm)         list_add_tail_rcu(&p->thread_node, ...)

3)for_each_thread(g, c) copy_process()

                                   list_add_tail_rcu(&p->thread_node, ...)
    p->mm != NULL check          do_exit()
    smp_rmb()                      smp_mb();
    get next thread in loop      p->mm = NULL

This patch may be useful for machines with many processes executing. I regulary observe mm_update_next_owner() executing on one of the cpus in crash dumps (not related to this function) on big machines. Even if iteration over task list looks as unlikely situation, it regularity grows with the growth of containers/processes numbers.

Signed-off-by Kirill Tkhai <[email protected]>

---
 kernel/exit.c |   39 +++++++++++++++++++++++++++++++++++----
 kernel/fork.c |    1 +
 kernel/pid.c  |    5 ++++-
 3 files changed, 40 insertions(+), 5 deletions(-)

kernel/exit.c +35 -4

index 40f734ed1193..7ce4cdf96a64 100644
@@ -406,6 +406,8 @@ kill_orphaned_pgrp(struct task_struct *tsk, struct task_struct *parent)
 void mm_update_next_owner(struct mm_struct *mm)
 {
 	struct task_struct *c, *g, *p = current;
+	struct mm_struct *tmp;
+	struct list_head *n;
 
 retry:
 	/*

@@ -440,21 +442,49 @@ void mm_update_next_owner(struct mm_struct *mm)
 		if (c->mm == mm)
 			goto new_owner;
 	}
+	read_unlock(&tasklist_lock);
 
 	/*
 	 * Search through everything else, we should not get here often.
 	 */
+	rcu_read_lock();
 	for_each_process(g) {
+		/*
+		 * g->signal, g->mm and g->flags initialization of a just
+		 * created task must not reorder with linking the task to
+		 * tasks list. Pairs with smp_mb() implied by alloc_pid().
+		 */
+		smp_rmb();
 		if (g->flags & PF_KTHREAD)
 			continue;
 		for_each_thread(g, c) {
-			if (c->mm == mm)
-				goto new_owner;
-			if (c->mm)
+			/*
+			 * Make visible mm of iterated thread.
+			 * Pairs with smp_mb() implied by alloc_pid().
+			 */
+			if (c != g)
+				smp_rmb();
+			tmp = READ_ONCE(c->mm);
+			if (tmp == mm)
+				goto new_owner_nolock;
+			if (likely(tmp))
 				break;
+			n = READ_ONCE(c->thread_node.next);
+			/*
+			 * All mm are NULL, so iterated threads already exited.
+			 * Make sure we see their children.
+			 * Pairs with smp_mb() in do_exit().
+			 */
+			if (n == &g->signal->thread_head)
+				smp_rmb();
 		}
+		/*
+		 * Children of exited thread group are visible due to the above
+		 * smp_rmb(). Threads with mm != NULL can't create a child with
+		 * the mm we're looking for. So, no additional smp_rmb() needed.
+		 */
 	}
-	read_unlock(&tasklist_lock);
+	rcu_read_unlock();
 	/*
 	 * We found no owner yet mm_users > 1: this implies that we are
 	 * most likely racing with swapoff (try_to_unuse()) or /proc or

@@ -466,6 +496,7 @@ void mm_update_next_owner(struct mm_struct *mm)
 new_owner:
 	rcu_read_lock();
 	read_unlock(&tasklist_lock);
+new_owner_nolock:
 	BUG_ON(c == p);
 
 	/*

kernel/fork.c +1 -0

index a5d21c42acfc..2032d4657546 100644
@@ -1805,6 +1805,7 @@ static __latent_entropy struct task_struct *copy_process(
 		goto bad_fork_cleanup_io;
 
 	if (pid != &init_struct_pid) {
+		/* Successfuly returned, this function imply smp_mb() */
 		pid = alloc_pid(p->nsproxy->pid_ns_for_children);
 		if (IS_ERR(pid)) {
 			retval = PTR_ERR(pid);

kernel/pid.c +0 -0

index 157fe4b19971..cb96473aa058 100644

kernel/pid.c void free_pid(struct pid *pid) · line 160

Quoted 7 lines

kernel/pid.c +3 -1

@@ -155,7 +155,10 @@ void free_pid(struct pid *pid)
 
 	call_rcu(&pid->rcu, delayed_put_pid);
 }
-
+/*
+ * This function contains at least two sequential spin_lock()/spin_unlock(),
+ * and together they imply full memory barrier.

Mmh, it's possible that I am misunderstanding this statement but it does not seem quite correct to me; a counter-example would be provided by the test at "tools/memory-model/litmus-tests/SB+mbonceonces.litmus" (replace either of the smp_mb() with the sequence:

   spin_lock(s); spin_unlock(s); spin_lock(s); spin_unlock(s); ).

BTW, your commit message suggests that your case would work with "imply an smp_wmb()". This implication should hold "w.r.t. current implementa- tions". We (LKMM people) discussed changes to the LKMM to make it hold in LKMM but such changes are still in our TODO list as of today...

In the patch I used the logic, that the below code:

	x = A;
	spin_lock();
	spin_unlock();
	spin_lock();
	spin_unlock();
	y = B;

cannot reorder much than:

	spin_lock();
	x = A;		<- this can't become visible later, that spin_unlock()
	spin_unlock();
	spin_lock();
	y = B;		<- this can't become visible earlier, than spin_lock()
	spin_unlock();

Is there a problem?

Quoted 18 lines · Kirill Tkhai

In the patch I used the logic, that the below code:

	x = A;
	spin_lock();
	spin_unlock();
	spin_lock();
	spin_unlock();
	y = B;

cannot reorder much than:

	spin_lock();
	x = A;		<- this can't become visible later, that spin_unlock()
	spin_unlock();
	spin_lock();
	y = B;		<- this can't become visible earlier, than spin_lock()
	spin_unlock();

Quoted 1 line

Is there a problem?

The two stores will be ordered, but only at the strength of an smp_wmb(). The above construct does not imply smp_mb(). The difference is observable on real hardware (Power).

Quoted 10 lines

Quoted 10 lines

1)for_each_process(g) copy_process()

                                   p->mm = mm
    smp_rmb();                     smp_wmb() implied by alloc_pid()
    if (g->flags & PF_KTHREAD)     list_add_tail_rcu(&p->tasks, &init_task.tasks)

2)for_each_thread(g, c) copy_process()

                                   p->mm = mm
    smp_rmb();                     smp_wmb() implied by alloc_pid()
    tmp = READ_ONCE(c->mm)         list_add_tail_rcu(&p->thread_node, ...)

Quoted 6 lines · Kirill Tkhai

Quoted 6 lines

Quoted 6 lines

1)for_each_process(g) copy_process()

                                   p->mm = mm
    smp_rmb();                     smp_wmb() implied by alloc_pid()
    if (g->flags & PF_KTHREAD)     list_add_tail_rcu(&p->tasks, &init_task.tasks)

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

2)for_each_thread(g, c) copy_process()

                                   p->mm = mm
    smp_rmb();                     smp_wmb() implied by alloc_pid()
    tmp = READ_ONCE(c->mm)         list_add_tail_rcu(&p->thread_node, ...)

For these two; what's the purpose of the smp_rmb()? which loads are ordered?

On 26.04.2018 15:35, Andrea Parri wrote:

Quoted 6 lines

Mmh, it's possible that I am misunderstanding this statement but it does not seem quite correct to me; a counter-example would be provided by the test at "tools/memory-model/litmus-tests/SB+mbonceonces.litmus" (replace either of the smp_mb() with the sequence:

Quoted 6 lines

   spin_lock(s); spin_unlock(s); spin_lock(s); spin_unlock(s); ).

BTW, your commit message suggests that your case would work with "imply an smp_wmb()". This implication should hold "w.r.t. current implementa- tions". We (LKMM people) discussed changes to the LKMM to make it hold in LKMM but such changes are still in our TODO list as of today...

I'm not close to LKMM, so the test you referenced is not clear for me.

Does LKMM show the real hardware behavior? Or there are added the most cases, and work is still in progress?

In the patch I used the logic, that the below code:

	x = A;
	spin_lock();
	spin_unlock();
	spin_lock();
	spin_unlock();
	y = B;

cannot reorder much than:

	spin_lock();
	x = A;		<- this can't become visible later, that spin_unlock()
	spin_unlock();
	spin_lock();
	y = B;		<- this can't become visible earlier, than spin_lock()
	spin_unlock();

Is there a problem?

Kirill

Quoted 1 line · Kirill Tkhai

On 26.04.2018 15:35, Andrea Parri wrote:

[...]

Quoted 6 lines

Quoted 6 lines

Mmh, it's possible that I am misunderstanding this statement but it does not seem quite correct to me; a counter-example would be provided by the test at "tools/memory-model/litmus-tests/SB+mbonceonces.litmus" (replace either of the smp_mb() with the sequence:

Quoted 8 lines

Quoted 6 lines

   spin_lock(s); spin_unlock(s); spin_lock(s); spin_unlock(s); ).

BTW, your commit message suggests that your case would work with "imply an smp_wmb()". This implication should hold "w.r.t. current implementa- tions". We (LKMM people) discussed changes to the LKMM to make it hold in LKMM but such changes are still in our TODO list as of today...

I'm not close to LKMM, so the test you referenced is not clear for me.

The test could be concisely described by:

   {initially: x=y=0; }

   Thread0	Thread1

   x = 1;	y = 1;
   MB		MB
   r0 = y;	r1 = x;

   Can r0,r1 be both 0 after joining?

The answer to the question is -No-; however, if you replaced any of the MB with the locking sequence described above, then the answer is -Yes-:
full fences on both sides are required to forbid that state and this is something that the locking sequences won't be able to provide (think at the implementation of these primitives for powerpc, for example).

Quoted 2 lines

Does LKMM show the real hardware behavior? Or there are added the most cases, and work is still in progress?

Very roughly speaking, LKMM is an "envelope" of the underlying hardware memory models/architectures supported by the Linux kernel which in turn may not coincide with the observable behavior on a given implementation /processor of that architecture. Also, LKMM doesn't aim to be a "tight" envelope. I'd refer to the documentation within "tools/memory-model/"; please let me know if I can provide further info.

Quoted 12 lines

In the patch I used the logic, that the below code:

	x = A;
	spin_lock();
	spin_unlock();
	spin_lock();
	spin_unlock();
	y = B;

cannot reorder much than:

Quoted 8 lines

	spin_lock();
	x = A;		<- this can't become visible later, that spin_unlock()
	spin_unlock();
	spin_lock();
	y = B;		<- this can't become visible earlier, than spin_lock()
	spin_unlock();

Is there a problem?

As mentioned in the previous email, if smp_wmb() is what you're looking for then this should be fine (considering current implementations; LKMM will likely be there soon...).

BTW, the behavior in question has been recently discussed on the list; c.f., for example, the test "unlock-lock-write-ordering" described in:

http://lkml.kernel.org/r/[email protected]

as well as

0123f4d76ca63b7b895f40089be0ce4809e392d8 ("riscv/spinlock: Strengthen implementations with fences")

Andrea

This function searches for a new mm owner in children and siblings, and then iterates over all processes in the system in unlikely case. Despite the case is unlikely, its probability growths with the number of processes in the system. The time, spent on iterations, also growths. I regulary observe mm_update_next_owner() in crash dumps (not related to this function) of the nodes with many processes (20K+), so it looks like it's not so unlikely case.

then I suspect that we should be better off reconsidering mm->owner and try to come up with something more clever. I've had a patch to remove owner few years back.

Quoted 7 lines · Kirill Tkhai

This function searches for a new mm owner in children and siblings, and then iterates over all processes in the system in unlikely case. Despite the case is unlikely, its probability growths with the number of processes in the system. The time, spent on iterations, also growths. I regulary observe mm_update_next_owner() in crash dumps (not related to this function) of the nodes with many processes (20K+), so it looks like it's not so unlikely case.

Did you manage to find the pattern that forces mm_update_next_owner to slow paths? This really shouldn't trigger very often. If we can fallback easily then I suspect that we should be better off reconsidering mm->owner and try to come up with something more clever. I've had a patch to remove owner few years back. It needed some work to finish but maybe that would be a better than try to make non-scalable thing suck less.

We can rework this simply by adding a list of tasks to mm.

I've had a patch to remove owner few years back. It needed some work to finish but maybe that would be a better than try to make non-scalable thing suck less.

Quoted 3 lines · Michal Hocko

I've had a patch to remove owner few years back. It needed some work to finish but maybe that would be a better than try to make non-scalable thing suck less.

I have a question. Would it be reasonable to just have a mm->memcg? That would appear to be the simplest solution to the problem.

Quoted 6 lines · Eric W. Biederman

Quoted 3 lines · Michal Hocko

I've had a patch to remove owner few years back. It needed some work to finish but maybe that would be a better than try to make non-scalable thing suck less.

Quoted 2 lines

I have a question. Would it be reasonable to just have a mm->memcg? That would appear to be the simplest solution to the problem.

I do not remember details. Have to re-read the whole thing again. Hope to get to this soon but with the current jet lag and backlog from the LSFMM I rather not promis anything. Going with mm->memcg would be the most simple of course but I have a very vague recollection that it was not possible. Maybe I misremember...

Quoted 7 lines · Michal Hocko

Quoted 6 lines · Eric W. Biederman

Quoted 3 lines · Michal Hocko

I've had a patch to remove owner few years back. It needed some work to finish but maybe that would be a better than try to make non-scalable thing suck less.

Quoted 8 lines

Quoted 2 lines

I have a question. Would it be reasonable to just have a mm->memcg? That would appear to be the simplest solution to the problem.

I do not remember details. Have to re-read the whole thing again. Hope to get to this soon but with the current jet lag and backlog from the LSFMM I rather not promis anything. Going with mm->memcg would be the most simple of course but I have a very vague recollection that it was not possible. Maybe I misremember...

Just for the record, the last version where I've tried to remove owner
was posted here: http://lkml.kernel.org/r/[email protected]

I didn't get to remember details yet, but the primary problem was the task migration between cgroups and the nasty case when different thread grounds share the mm. At some point I just suggested to not care about semantic of these weird threads all that much. We can either migrate all tasks sharing the mm struct or just keep the inconsistency.

Anyway, removing this ugliness would be so cool!

Kirill Tkhai do you think you would be able adapt Michal Hoko's old patch at https://marc.info/?l=linux-kernel&m=143635857131756&w=2
that replaces mm->owner with mm->memcg?

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 <[email protected]>

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 fs/exec.c                  |  1 -
 include/linux/memcontrol.h | 11 ++++--
 include/linux/mm_types.h   | 12 +------
 include/linux/sched/mm.h   |  8 -----
 kernel/exit.c              | 89 ----------------------------------------------
 kernel/fork.c              | 17 +++++++--
 mm/memcontrol.c            | 86 ++++++++++++++++++++++++++++++++++----------
 7 files changed, 90 insertions(+), 134 deletions(-)

fs/exec.c +0 -1

index 183059c427b9..a8be9318d1a8 100644
@@ -1040,7 +1040,6 @@ static int exec_mmap(struct mm_struct *mm)
 		up_read(&old_mm->mmap_sem);
 		BUG_ON(active_mm != old_mm);
 		setmax_mm_hiwater_rss(&tsk->signal->maxrss, old_mm);
-		mm_update_next_owner(old_mm);
 		mmput(old_mm);
 		return 0;
 	}

include/linux/memcontrol.h +8 -3

index d99b71bc2c66..147e04bfcaee 100644
@@ -341,7 +341,6 @@ static inline struct lruvec *mem_cgroup_lruvec(struct pglist_data *pgdat,
 struct lruvec *mem_cgroup_page_lruvec(struct page *, struct pglist_data *);
 
 bool task_in_mem_cgroup(struct task_struct *task, struct mem_cgroup *memcg);
-struct mem_cgroup *mem_cgroup_from_task(struct task_struct *p);
 
 static inline
 struct mem_cgroup *mem_cgroup_from_css(struct cgroup_subsys_state *css){

@@ -402,6 +401,8 @@ static inline bool mem_cgroup_is_descendant(struct mem_cgroup *memcg,
 	return cgroup_is_descendant(memcg->css.cgroup, root->css.cgroup);
 }
 
+void mm_update_memcg(struct mm_struct *mm, struct mem_cgroup *new);
+
 static inline bool mm_match_cgroup(struct mm_struct *mm,
 				   struct mem_cgroup *memcg)
 {

@@ -409,7 +410,7 @@ static inline bool mm_match_cgroup(struct mm_struct *mm,
 	bool match = false;
 
 	rcu_read_lock();
-	task_memcg = mem_cgroup_from_task(rcu_dereference(mm->owner));
+	task_memcg = rcu_dereference(mm->memcg);
 	if (task_memcg)
 		match = mem_cgroup_is_descendant(task_memcg, memcg);
 	rcu_read_unlock();

@@ -693,7 +694,7 @@ static inline void count_memcg_event_mm(struct mm_struct *mm,
 		return;
 
 	rcu_read_lock();
-	memcg = mem_cgroup_from_task(rcu_dereference(mm->owner));
+	memcg = rcu_dereference(mm->memcg);
 	if (likely(memcg)) {
 		count_memcg_events(memcg, idx, 1);
 		if (idx == OOM_KILL)

@@ -781,6 +782,10 @@ static inline struct lruvec *mem_cgroup_page_lruvec(struct page *page,
 	return &pgdat->lruvec;
 }
 
+static inline void mm_update_memcg(struct mm_struct *mm, struct mem_cgroup *new)
+{
+}
+
 static inline bool mm_match_cgroup(struct mm_struct *mm,
 		struct mem_cgroup *memcg)
 {

include/linux/mm_types.h +1 -11

index 21612347d311..ea5efd40a5d1 100644
@@ -443,17 +443,7 @@ struct mm_struct {
 	struct kioctx_table __rcu	*ioctx_table;
 #endif
 #ifdef CONFIG_MEMCG
-	/*
-	 * "owner" points to a task that is regarded as the canonical
-	 * user/owner of this mm. All of the following must be true in
-	 * order for it to be changed:
-	 *
-	 * current == mm->owner
-	 * current->mm != mm
-	 * new_owner->mm == mm
-	 * new_owner->alloc_lock is held
-	 */
-	struct task_struct __rcu *owner;
+	struct mem_cgroup __rcu	*memcg;
 #endif
 	struct user_namespace *user_ns;
 

include/linux/sched/mm.h +0 -8

index 2c570cd934af..cc8e68d36fc2 100644
@@ -95,14 +95,6 @@ extern struct mm_struct *mm_access(struct task_struct *task, unsigned int mode);
 /* Remove the current tasks stale references to the old mm_struct */
 extern void mm_release(struct task_struct *, struct mm_struct *);
 
-#ifdef CONFIG_MEMCG
-extern void mm_update_next_owner(struct mm_struct *mm);
-#else
-static inline void mm_update_next_owner(struct mm_struct *mm)
-{
-}
-#endif /* CONFIG_MEMCG */
-
 #ifdef CONFIG_MMU
 extern void arch_pick_mmap_layout(struct mm_struct *mm,
 				  struct rlimit *rlim_stack);

kernel/exit.c +0 -89

index c3c7ac560114..be967d2da0ce 100644
@@ -399,94 +399,6 @@ kill_orphaned_pgrp(struct task_struct *tsk, struct task_struct *parent)
 	}
 }
 
-#ifdef CONFIG_MEMCG
-/*
- * A task is exiting.   If it owned this mm, find a new owner for the mm.
- */
-void mm_update_next_owner(struct mm_struct *mm)
-{
-	struct task_struct *c, *g, *p = current;
-
-retry:
-	/*
-	 * If the exiting or execing task is not the owner, it's
-	 * someone else's problem.
-	 */
-	if (mm->owner != p)
-		return;
-	/*
-	 * The current owner is exiting/execing and there are no other
-	 * candidates.  Do not leave the mm pointing to a possibly
-	 * freed task structure.
-	 */
-	if (atomic_read(&mm->mm_users) <= 1) {
-		mm->owner = NULL;
-		return;
-	}
-
-	read_lock(&tasklist_lock);
-	/*
-	 * Search in the children
-	 */
-	list_for_each_entry(c, &p->children, sibling) {
-		if (c->mm == mm)
-			goto assign_new_owner;
-	}
-
-	/*
-	 * Search in the siblings
-	 */
-	list_for_each_entry(c, &p->real_parent->children, sibling) {
-		if (c->mm == mm)
-			goto assign_new_owner;
-	}
-
-	/*
-	 * Search through everything else, we should not get here often.
-	 */
-	for_each_process(g) {
-		if (g->flags & PF_KTHREAD)
-			continue;
-		for_each_thread(g, c) {
-			if (c->mm == mm)
-				goto assign_new_owner;
-			if (c->mm)
-				break;
-		}
-	}
-	read_unlock(&tasklist_lock);
-	/*
-	 * We found no owner yet mm_users > 1: this implies that we are
-	 * most likely racing with swapoff (try_to_unuse()) or /proc or
-	 * ptrace or page migration (get_task_mm()).  Mark owner as NULL.
-	 */
-	mm->owner = NULL;
-	return;
-
-assign_new_owner:
-	BUG_ON(c == p);
-	get_task_struct(c);
-	/*
-	 * The task_lock protects c->mm from changing.
-	 * We always want mm->owner->mm == mm
-	 */
-	task_lock(c);
-	/*
-	 * Delay read_unlock() till we have the task_lock()
-	 * to ensure that c does not slip away underneath us
-	 */
-	read_unlock(&tasklist_lock);
-	if (c->mm != mm) {
-		task_unlock(c);
-		put_task_struct(c);
-		goto retry;
-	}
-	mm->owner = c;
-	task_unlock(c);
-	put_task_struct(c);
-}
-#endif /* CONFIG_MEMCG */
-
 /*
  * Turn us into a lazy TLB process if we
  * aren't already..

@@ -540,7 +452,6 @@ static void exit_mm(void)
 	up_read(&mm->mmap_sem);
 	enter_lazy_tlb(mm, current);
 	task_unlock(current);
-	mm_update_next_owner(mm);
 	mmput(mm);
 	if (test_thread_flag(TIF_MEMDIE))
 		exit_oom_victim();

kernel/fork.c +14 -3

index a5d21c42acfc..f284acf22aad 100644
@@ -868,10 +868,19 @@ static void mm_init_aio(struct mm_struct *mm)
 #endif
 }
 
-static void mm_init_owner(struct mm_struct *mm, struct task_struct *p)
+static void mm_init_memcg(struct mm_struct *mm)
 {
 #ifdef CONFIG_MEMCG
-	mm->owner = p;
+	struct cgroup_subsys_state *css;
+
+	/* Ensure mm->memcg is initialized */
+	mm->memcg = NULL;
+
+	rcu_read_lock();
+	css = task_css(current, memory_cgrp_id);
+	if (css && css_tryget(css))
+		mm_update_memcg(mm, mem_cgroup_from_css(css));
+	rcu_read_unlock();
 #endif
 }
 

@@ -901,7 +910,7 @@ static struct mm_struct *mm_init(struct mm_struct *mm, struct task_struct *p,
 	spin_lock_init(&mm->page_table_lock);
 	mm_init_cpumask(mm);
 	mm_init_aio(mm);
-	mm_init_owner(mm, p);
+	mm_init_memcg(mm);
 	RCU_INIT_POINTER(mm->exe_file, NULL);
 	mmu_notifier_mm_init(mm);
 	hmm_mm_init(mm);

@@ -931,6 +940,7 @@ static struct mm_struct *mm_init(struct mm_struct *mm, struct task_struct *p,
 fail_nocontext:
 	mm_free_pgd(mm);
 fail_nopgd:
+	mm_update_memcg(mm, NULL);
 	free_mm(mm);
 	return NULL;
 }

@@ -968,6 +978,7 @@ static inline void __mmput(struct mm_struct *mm)
 	}
 	if (mm->binfmt)
 		module_put(mm->binfmt->module);
+	mm_update_memcg(mm, NULL);
 	mmdrop(mm);
 }
 

mm/memcontrol.c +67 -19

index 2bd3df3d101a..5dce8a7fa65b 100644
@@ -664,20 +664,6 @@ static void memcg_check_events(struct mem_cgroup *memcg, struct page *page)
 	}
 }
 
-struct mem_cgroup *mem_cgroup_from_task(struct task_struct *p)
-{
-	/*
-	 * mm_update_next_owner() may clear mm->owner to NULL
-	 * if it races with swapoff, page migration, etc.
-	 * So this can be called with p == NULL.
-	 */
-	if (unlikely(!p))
-		return NULL;
-
-	return mem_cgroup_from_css(task_css(p, memory_cgrp_id));
-}
-EXPORT_SYMBOL(mem_cgroup_from_task);
-
 static struct mem_cgroup *get_mem_cgroup_from_mm(struct mm_struct *mm)
 {
 	struct mem_cgroup *memcg = NULL;

@@ -692,7 +678,7 @@ static struct mem_cgroup *get_mem_cgroup_from_mm(struct mm_struct *mm)
 		if (unlikely(!mm))
 			memcg = root_mem_cgroup;
 		else {
-			memcg = mem_cgroup_from_task(rcu_dereference(mm->owner));
+			memcg = rcu_dereference(mm->memcg);
 			if (unlikely(!memcg))
 				memcg = root_mem_cgroup;
 		}

@@ -1011,7 +997,7 @@ bool task_in_mem_cgroup(struct task_struct *task, struct mem_cgroup *memcg)
 		 * killed to prevent needlessly killing additional tasks.
 		 */
 		rcu_read_lock();
-		task_memcg = mem_cgroup_from_task(task);
+		task_memcg = mem_cgroup_from_css(task_css(task, memory_cgrp_id));
 		css_get(&task_memcg->css);
 		rcu_read_unlock();
 	}

@@ -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);

@@ -4859,6 +4846,59 @@ static int mem_cgroup_can_attach(struct cgroup_taskset *tset)
 	return ret;
 }
 
+/**
+ * mm_update_memcg - Update the memory cgroup of a mm_struct
+ * @mm: mm struct
+ * @new: new memory cgroup value
+ *
+ * Called whenever mm->memcg needs to change.   Consumes a reference
+ * to new (unless new is NULL).   The reference to the old memory
+ * cgroup is decreased.
+ */
+void mm_update_memcg(struct mm_struct *mm, struct mem_cgroup *new)
+{
+	/* This is the only place where mm->memcg is changed */
+	struct mem_cgroup *old;
+
+	old = xchg(&mm->memcg, new);
+	if (old)
+		css_put(&old->css);
+}
+
+static void task_update_memcg(struct task_struct *tsk, struct mem_cgroup *new)
+{
+	struct mm_struct *mm;
+	task_lock(tsk);
+	mm = tsk->mm;
+	if (mm && !(tsk->flags & PF_KTHREAD))
+		mm_update_memcg(mm, new);
+	task_unlock(tsk);
+}
+
+static void mem_cgroup_attach(struct cgroup_taskset *tset)
+{
+	struct cgroup_subsys_state *css;
+	struct task_struct *tsk;
+
+	cgroup_taskset_for_each(tsk, css, tset) {
+		struct mem_cgroup *new = mem_cgroup_from_css(css);
+		css_get(css);
+		task_update_memcg(tsk, new);
+	}
+}
+
+static void mem_cgroup_fork(struct task_struct *tsk)
+{
+	struct cgroup_subsys_state *css;
+
+	rcu_read_lock();
+	css = task_css(tsk, memory_cgrp_id);
+	if (css && css_tryget(css))
+		task_update_memcg(tsk, mem_cgroup_from_css(css));
+	rcu_read_unlock();
+}
+
+
 static void mem_cgroup_cancel_attach(struct cgroup_taskset *tset)
 {
 	if (mc.to)

@@ -5027,6 +5067,12 @@ static int mem_cgroup_can_attach(struct cgroup_taskset *tset)
 {
 	return 0;
 }
+static void mem_cgroup_attach(struct cgroup_taskset *tset)
+{
+}
+static void mem_cgroup_fork(struct task_struct *task)
+{
+}
 static void mem_cgroup_cancel_attach(struct cgroup_taskset *tset)
 {
 }

@@ -5335,8 +5381,10 @@ struct cgroup_subsys memory_cgrp_subsys = {
 	.css_free = mem_cgroup_css_free,
 	.css_reset = mem_cgroup_css_reset,
 	.can_attach = mem_cgroup_can_attach,
+	.attach = mem_cgroup_attach,
 	.cancel_attach = mem_cgroup_cancel_attach,
 	.post_attach = mem_cgroup_move_task,
+	.fork = mem_cgroup_fork,
 	.bind = mem_cgroup_bind,
 	.dfl_cftypes = memory_files,
 	.legacy_cftypes = mem_cgroup_legacy_files,

@@ -5769,7 +5817,7 @@ void mem_cgroup_sk_alloc(struct sock *sk)
 	}
 
 	rcu_read_lock();
-	memcg = mem_cgroup_from_task(current);
+	memcg = mem_cgroup_from_css(task_css(current, memory_cgrp_id));
 	if (memcg == root_mem_cgroup)
 		goto out;
 	if (!cgroup_subsys_on_dfl(memory_cgrp_subsys) && !memcg->tcpmem_active)

Signature · 1 line

2.14.1

[CC johannes and Tejun as well. I am sorry but my backlog is so huge I will not get to this week.]

Quoted 46 lines · Eric W. Biederman

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 <[email protected]>

Signed-off-by "Eric W. Biederman" <[email protected]>

Quoted 9 lines

mm/memcontrol.c +1 -1

@@ -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/memcontrol.c static int mem_cgroup_can_attach(struct cgroup_taskset *tset) · line 4828

Quoted 10 lines

mm/memcontrol.c +2 -1

 	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);

Hi Eric,

Quoted 50 lines · Michal Hocko

[CC johannes and Tejun as well. I am sorry but my backlog is so huge I will not get to this week.]

Quoted 46 lines · Eric W. Biederman

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 <[email protected]>

Signed-off-by "Eric W. Biederman" <[email protected]>

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.

Quoted 9 lines

Quoted 9 lines

mm/memcontrol.c +1 -1

@@ -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);
 

Quoted 10 lines

mm/memcontrol.c static int mem_cgroup_can_attach(struct cgroup_taskset *tset) · line 4828

Quoted 10 lines

mm/memcontrol.c +2 -1

 	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.

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.

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

Aside from that, this looks great to me. For the fixed version:

Acked-by Johannes Weiner <[email protected]>

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.

Quoted 36 lines · On Wed, 02 May 2018 14:21:35 -0500 [email protected] (Eric W. Biederman)…

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.

Quoted 6 lines

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.

Cleanliness nit: I'm not sure that the removal and open-coding of mem_cgroup_from_task() actually improved things. Should we restore it?

mm/memcontrol.c renamed from mm/memcontrol.c~memcg-replace-mm-owner-with-mm-memcg-fix +8 -3

@@ -664,6 +664,11 @@ static void memcg_check_events(struct me
 	}
 }
 
+static inline struct mem_cgroup *mem_cgroup_from_task(struct task_struct *p)
+{
+	return mem_cgroup_from_css(task_css(p, memory_cgrp_id));
+}
+
 struct mem_cgroup *get_mem_cgroup_from_mm(struct mm_struct *mm)
 {
 	struct mem_cgroup *memcg = NULL;

@@ -1011,7 +1016,7 @@ bool task_in_mem_cgroup(struct task_stru
 		 * killed to prevent needlessly killing additional tasks.
 		 */
 		rcu_read_lock();
-		task_memcg = mem_cgroup_from_css(task_css(task, memory_cgrp_id));
+		task_memcg = mem_cgroup_from_task(task);
 		css_get(&task_memcg->css);
 		rcu_read_unlock();
 	}

@@ -4829,7 +4834,7 @@ static int mem_cgroup_can_attach(struct
 	if (!move_flags)
 		return 0;
 
-	from = mem_cgroup_from_css(task_css(p, memory_cgrp_id));
+	from = mem_cgroup_from_task(p);
 
 	VM_BUG_ON(from == memcg);
 

@@ -5887,7 +5892,7 @@ void mem_cgroup_sk_alloc(struct sock *sk
 	}
 
 	rcu_read_lock();
-	memcg = mem_cgroup_from_css(task_css(current, memory_cgrp_id));
+	memcg = mem_cgroup_from_task(current);
 	if (memcg == root_mem_cgroup)
 		goto out;
 	if (!cgroup_subsys_on_dfl(memory_cgrp_subsys) && !memcg->tcpmem_active)

_

mm/memcontrol.c static int mem_cgroup_can_attach(struct cgroup_taskset *tset) · line 4899

Quoted 11 lines · Eric W. Biederman

mm/memcontrol.c +10 -0

+static void mem_cgroup_fork(struct task_struct *tsk)
+{
+	struct cgroup_subsys_state *css;
+
+	rcu_read_lock();
+	css = task_css(tsk, memory_cgrp_id);
+	if (css && css_tryget(css))
+		task_update_memcg(tsk, mem_cgroup_from_css(css));
+	rcu_read_unlock();
+}

Why do we need it?

The child's mm->memcg was already initialized by mm_init_memcg() and we can't race with migrate until cgroup_threadgroup_change_end() ?

Quoted 15 lines · Oleg Nesterov

mm/memcontrol.c static int mem_cgroup_can_attach(struct cgroup_taskset *tset) · line 4899

Quoted 11 lines · Eric W. Biederman

mm/memcontrol.c +10 -0

+static void mem_cgroup_fork(struct task_struct *tsk)
+{
+	struct cgroup_subsys_state *css;
+
+	rcu_read_lock();
+	css = task_css(tsk, memory_cgrp_id);
+	if (css && css_tryget(css))
+		task_update_memcg(tsk, mem_cgroup_from_css(css));
+	rcu_read_unlock();
+}

Why do we need it?

Quoted 2 lines

The child's mm->memcg was already initialized by mm_init_memcg() and we can't race with migrate until cgroup_threadgroup_change_end() ?

I admit I missed the cgroup_threadgroup_change_begin cgroup_threadgroup_change_end pair in fs fork. In this case it doesn't matter because mm_init_memcg is called from:

copy_mm
dup_mm
mm_init

And copy_mm is called before we call cgroup_threadgroup_change_begin. So the race remains.

We could move move cgroup_threadgroup_change_begin earlier, to remove the need for mem_cgroup_fork. But I have not analyzed that.

Quoted 25 lines · Eric W. Biederman

Quoted 15 lines · Oleg Nesterov

mm/memcontrol.c static int mem_cgroup_can_attach(struct cgroup_taskset *tset) · line 4899

Quoted 11 lines · Eric W. Biederman

mm/memcontrol.c +10 -0

+static void mem_cgroup_fork(struct task_struct *tsk)
+{
+	struct cgroup_subsys_state *css;
+
+	rcu_read_lock();
+	css = task_css(tsk, memory_cgrp_id);
+	if (css && css_tryget(css))
+		task_update_memcg(tsk, mem_cgroup_from_css(css));
+	rcu_read_unlock();
+}

Why do we need it?

Quoted 2 lines

The child's mm->memcg was already initialized by mm_init_memcg() and we can't race with migrate until cgroup_threadgroup_change_end() ?

I admit I missed the cgroup_threadgroup_change_begin cgroup_threadgroup_change_end pair in fs fork. In this case it doesn't matter because mm_init_memcg is called from:

Quoted 6 lines

copy_mm
dup_mm
mm_init

And copy_mm is called before we call cgroup_threadgroup_change_begin. So the race remains.

Ah yes, you are right.

Quoted 2 lines

We could move move cgroup_threadgroup_change_begin earlier, to remove the need for mem_cgroup_fork. But I have not analyzed that.

No, cgroup_threadgroup_change_begin() was called early and this was wrong, see 568ac888215c7fb2fabe8ea739b00ec3c1f5d440. Actually there were more problems, say copy_net() could deadlock because cleanup_net() does do_wait() with net_mutex held.

OK, what about exec() ? mm_init_memcg() initializes bprm->mm->memcg early in bprm_mm_init(). What if the execing task migrates before exec_mmap() ?

Quoted 2 lines · Oleg Nesterov

OK, what about exec() ? mm_init_memcg() initializes bprm->mm->memcg early in bprm_mm_init(). What if the execing task migrates before exec_mmap() ?

We need the the cgroup when the mm is initialized. That way we have the cgroup information when initializing the mm.

I don't know if a lock preventing changing the cgroup in exec or just a little bit of code in exec_mmap to ensure mm->memcg is properly set is the better approach.

This does look like a very good place for an incremental patch to close that race.

Quoted 8 lines · Eric W. Biederman

Quoted 2 lines · Oleg Nesterov

OK, what about exec() ? mm_init_memcg() initializes bprm->mm->memcg early in bprm_mm_init(). What if the execing task migrates before exec_mmap() ?

We need the the cgroup when the mm is initialized. That way we have the cgroup information when initializing the mm.

Yes, we need to initialize new_mm->memcg but iiuc mostly for the error path,

Quoted 3 lines

I don't know if a lock preventing changing the cgroup in exec or just a little bit of code in exec_mmap to ensure mm->memcg is properly set is the better approach.

I'd vote for the change in exec_mmap(). This way mm_init_memcg() can just nullify mm->memcg.

Quoted 2 lines · Oleg Nesterov

I'd vote for the change in exec_mmap(). This way mm_init_memcg() can just nullify mm->memcg.

There is at least one common path where we need the memory control group properly initialized so memory allocations don't escape the memory control group.

do_execveat_common
copy_strings
get_arg_page
get_user_pages_remote
__get_user_pages_locked
__get_user_pages
faultin_page
handle_mm_fault
count_memcg_event_mm __handle_mm_fault
handle_pte_fault
do_anonymous_page
mem_cgroup_try_charge

I am surprised I can't easily find more. Apparently in load_elf_binary
we call elf_mmap after set_new_exec and install_exec_creds, making a gracefull recovery from elf_mmap failures impossible.

In any case we most definitely need the memory control group properly setup before exec_mmap.

Quoted 28 lines · Eric W. Biederman

Quoted 2 lines · Oleg Nesterov

I'd vote for the change in exec_mmap(). This way mm_init_memcg() can just nullify mm->memcg.

There is at least one common path where we need the memory control group properly initialized so memory allocations don't escape the memory control group.

do_execveat_common
copy_strings
get_arg_page
get_user_pages_remote
__get_user_pages_locked
__get_user_pages
faultin_page
handle_mm_fault
count_memcg_event_mm __handle_mm_fault
handle_pte_fault
do_anonymous_page
mem_cgroup_try_charge

I am surprised I can't easily find more. Apparently in load_elf_binary
we call elf_mmap after set_new_exec and install_exec_creds, making a gracefull recovery from elf_mmap failures impossible.

Quoted 2 lines

In any case we most definitely need the memory control group properly setup before exec_mmap.

Confused ...

new_mm->memcg has no effect until exec_mmap(), why it can't be NULL ?

and why do you think mem_cgroup_try_charge() can use the wrong memcg in this case?

I am getting the following for nommu with MEMCG=y randconfig:
kernel/fork.o: In function `mm_init':
fork.c:(.text+0x948): undefined reference to mm_update_memcg' kernel/fork.o: In function mmput':
fork.c:(.text+0x119c): undefined reference to `mm_update_memcg'
make: *** [vmlinux] Error 1

fs/exec.c static int exec_mmap(struct mm_struct *mm) · line 1050

Quoted 11 lines · Eric W. Biederman

fs/exec.c +2 -0

@@ -1044,6 +1044,8 @@ static int exec_mmap(struct mm_struct *mm)
 		return 0;
 	}
 	mmdrop(active_mm);
+	/* The tsk may have migrated before the new mm was attached */
+	mm_sync_memcg_from_task(tsk);
 	return 0;
 }

call_usermodehelper(). perhaps it makes sense to change flush_old_exec() to clear PF_KTHREAD/etc before exec_mmap().

Andrew can you pick up these two fixes as well.

These address the issues Michal Hocko and Oleg Nesterov noticed.

Quoted 4 lines · Eric W. Biederman

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?

Quoted 4 lines · Eric W. Biederman

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?

Quoted 4 lines · Michal Hocko

Quoted 3 lines · Eric W. Biederman

Andrew can you pick up these two fixes as well.

Quoted 5 lines

Quoted 1 line

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

Quoted 11 lines · Michal Hocko

Quoted 9 lines · Michal Hocko

Quoted 4 lines · Eric W. Biederman

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?

Quoted 1 line

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.

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.

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

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.

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:

I would really prefer and appreciate a repost with all the fixes folded in.

Quoted 2 lines · On Thu, 24 May 2018 13:10:02 +0200 Michal Hocko <[email protected]> wrote:

I would really prefer and appreciate a repost with all the fixes folded in.

[1/2]

From: "Eric W. Biederman" <[email protected]>
Subject: memcg: replace mm->owner with mm->memcg

Recently it was reported that mm_update_next_owner could get into cases where it was executing its 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.

During 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.

[[email protected]: update to work when !CONFIG_MMU]

Link http://lkml.kernel.org/r/[email protected] open here

[[email protected]: close race between migration and installing bprm->mm as mm]

Link http://lkml.kernel.org/r/[email protected] open here

Link http://lkml.kernel.org/r/[email protected] open here

Fixes cf475ad28ac3 ("cgroups: add an owner to the mm_struct")

Signed-off-by "Eric W. Biederman" <[email protected]>

Reported-by Kirill Tkhai <[email protected]>

Acked-by Johannes Weiner <[email protected]>

Cc Michal Hocko <[email protected]>

Cc "Kirill A. Shutemov" <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Oleg Nesterov <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

---

 fs/exec.c                  |    3 -
 include/linux/memcontrol.h |   16 +++++-
 include/linux/mm_types.h   |   12 ----
 include/linux/sched/mm.h   |    8 ---
 kernel/exit.c              |   89 -----------------------------------
 kernel/fork.c              |   17 +++++-
 mm/debug.c                 |    4 -
 mm/memcontrol.c            |   81 +++++++++++++++++++++++--------
 8 files changed, 93 insertions(+), 137 deletions(-)

diff -puN fs/exec.c~memcg-replace-mm-owner-with-mm-memcg fs/exec.c

fs/exec.c renamed from fs/exec.c~memcg-replace-mm-owner-with-mm-memcg +2 -1

@@ -1040,11 +1040,12 @@ static int exec_mmap(struct mm_struct *m
 		up_read(&old_mm->mmap_sem);
 		BUG_ON(active_mm != old_mm);
 		setmax_mm_hiwater_rss(&tsk->signal->maxrss, old_mm);
-		mm_update_next_owner(old_mm);
 		mmput(old_mm);
 		return 0;
 	}
 	mmdrop(active_mm);
+	/* The tsk may have migrated before the new mm was attached */
+	mm_sync_memcg_from_task(tsk);
 	return 0;
 }
 

diff -puN include/linux/memcontrol.h~memcg-replace-mm-owner-with-mm-memcg include/linux/memcontrol.h

include/linux/memcontrol.h renamed from include/linux/memcontrol.h~memcg-replace-mm-owner-with-mm-memcg +13 -3

@@ -345,7 +345,6 @@ out:
 struct lruvec *mem_cgroup_page_lruvec(struct page *, struct pglist_data *);
 
 bool task_in_mem_cgroup(struct task_struct *task, struct mem_cgroup *memcg);
-struct mem_cgroup *mem_cgroup_from_task(struct task_struct *p);
 
 struct mem_cgroup *get_mem_cgroup_from_mm(struct mm_struct *mm);
 

@@ -408,6 +407,9 @@ static inline bool mem_cgroup_is_descend
 	return cgroup_is_descendant(memcg->css.cgroup, root->css.cgroup);
 }
 
+void mm_update_memcg(struct mm_struct *mm, struct mem_cgroup *new);
+void mm_sync_memcg_from_task(struct task_struct *tsk);
+
 static inline bool mm_match_cgroup(struct mm_struct *mm,
 				   struct mem_cgroup *memcg)
 {

@@ -415,7 +417,7 @@ static inline bool mm_match_cgroup(struc
 	bool match = false;
 
 	rcu_read_lock();
-	task_memcg = mem_cgroup_from_task(rcu_dereference(mm->owner));
+	task_memcg = rcu_dereference(mm->memcg);
 	if (task_memcg)
 		match = mem_cgroup_is_descendant(task_memcg, memcg);
 	rcu_read_unlock();

@@ -699,7 +701,7 @@ static inline void count_memcg_event_mm(
 		return;
 
 	rcu_read_lock();
-	memcg = mem_cgroup_from_task(rcu_dereference(mm->owner));
+	memcg = rcu_dereference(mm->memcg);
 	if (likely(memcg)) {
 		count_memcg_events(memcg, idx, 1);
 		if (idx == OOM_KILL)

@@ -787,6 +789,14 @@ static inline struct lruvec *mem_cgroup_
 	return &pgdat->lruvec;
 }
 
+static inline void mm_update_memcg(struct mm_struct *mm, struct mem_cgroup *new)
+{
+}
+
+static inline void mm_sync_memcg_from_task(struct task_struct *tsk)
+{
+}
+
 static inline bool mm_match_cgroup(struct mm_struct *mm,
 		struct mem_cgroup *memcg)
 {

diff -puN include/linux/mm_types.h~memcg-replace-mm-owner-with-mm-memcg include/linux/mm_types.h

include/linux/mm_types.h renamed from include/linux/mm_types.h~memcg-replace-mm-owner-with-mm-memcg +1 -11

@@ -445,17 +445,7 @@ struct mm_struct {
 	struct kioctx_table __rcu	*ioctx_table;
 #endif
 #ifdef CONFIG_MEMCG
-	/*
-	 * "owner" points to a task that is regarded as the canonical
-	 * user/owner of this mm. All of the following must be true in
-	 * order for it to be changed:
-	 *
-	 * current == mm->owner
-	 * current->mm != mm
-	 * new_owner->mm == mm
-	 * new_owner->alloc_lock is held
-	 */
-	struct task_struct __rcu *owner;
+	struct mem_cgroup __rcu	*memcg;
 #endif
 	struct user_namespace *user_ns;
 

diff -puN include/linux/sched/mm.h~memcg-replace-mm-owner-with-mm-memcg include/linux/sched/mm.h

include/linux/sched/mm.h renamed from include/linux/sched/mm.h~memcg-replace-mm-owner-with-mm-memcg +0 -8

@@ -95,14 +95,6 @@ extern struct mm_struct *mm_access(struc
 /* Remove the current tasks stale references to the old mm_struct */
 extern void mm_release(struct task_struct *, struct mm_struct *);
 
-#ifdef CONFIG_MEMCG
-extern void mm_update_next_owner(struct mm_struct *mm);
-#else
-static inline void mm_update_next_owner(struct mm_struct *mm)
-{
-}
-#endif /* CONFIG_MEMCG */
-
 #ifdef CONFIG_MMU
 extern void arch_pick_mmap_layout(struct mm_struct *mm,
 				  struct rlimit *rlim_stack);

diff -puN kernel/exit.c~memcg-replace-mm-owner-with-mm-memcg kernel/exit.c

kernel/exit.c renamed from kernel/exit.c~memcg-replace-mm-owner-with-mm-memcg +0 -89

@@ -399,94 +399,6 @@ kill_orphaned_pgrp(struct task_struct *t
 	}
 }
 
-#ifdef CONFIG_MEMCG
-/*
- * A task is exiting.   If it owned this mm, find a new owner for the mm.
- */
-void mm_update_next_owner(struct mm_struct *mm)
-{
-	struct task_struct *c, *g, *p = current;
-
-retry:
-	/*
-	 * If the exiting or execing task is not the owner, it's
-	 * someone else's problem.
-	 */
-	if (mm->owner != p)
-		return;
-	/*
-	 * The current owner is exiting/execing and there are no other
-	 * candidates.  Do not leave the mm pointing to a possibly
-	 * freed task structure.
-	 */
-	if (atomic_read(&mm->mm_users) <= 1) {
-		mm->owner = NULL;
-		return;
-	}
-
-	read_lock(&tasklist_lock);
-	/*
-	 * Search in the children
-	 */
-	list_for_each_entry(c, &p->children, sibling) {
-		if (c->mm == mm)
-			goto assign_new_owner;
-	}
-
-	/*
-	 * Search in the siblings
-	 */
-	list_for_each_entry(c, &p->real_parent->children, sibling) {
-		if (c->mm == mm)
-			goto assign_new_owner;
-	}
-
-	/*
-	 * Search through everything else, we should not get here often.
-	 */
-	for_each_process(g) {
-		if (g->flags & PF_KTHREAD)
-			continue;
-		for_each_thread(g, c) {
-			if (c->mm == mm)
-				goto assign_new_owner;
-			if (c->mm)
-				break;
-		}
-	}
-	read_unlock(&tasklist_lock);
-	/*
-	 * We found no owner yet mm_users > 1: this implies that we are
-	 * most likely racing with swapoff (try_to_unuse()) or /proc or
-	 * ptrace or page migration (get_task_mm()).  Mark owner as NULL.
-	 */
-	mm->owner = NULL;
-	return;
-
-assign_new_owner:
-	BUG_ON(c == p);
-	get_task_struct(c);
-	/*
-	 * The task_lock protects c->mm from changing.
-	 * We always want mm->owner->mm == mm
-	 */
-	task_lock(c);
-	/*
-	 * Delay read_unlock() till we have the task_lock()
-	 * to ensure that c does not slip away underneath us
-	 */
-	read_unlock(&tasklist_lock);
-	if (c->mm != mm) {
-		task_unlock(c);
-		put_task_struct(c);
-		goto retry;
-	}
-	mm->owner = c;
-	task_unlock(c);
-	put_task_struct(c);
-}
-#endif /* CONFIG_MEMCG */
-
 /*
  * Turn us into a lazy TLB process if we
  * aren't already..

@@ -540,7 +452,6 @@ static void exit_mm(void)
 	up_read(&mm->mmap_sem);
 	enter_lazy_tlb(mm, current);
 	task_unlock(current);
-	mm_update_next_owner(mm);
 	mmput(mm);
 	if (test_thread_flag(TIF_MEMDIE))
 		exit_oom_victim();

diff -puN kernel/fork.c~memcg-replace-mm-owner-with-mm-memcg kernel/fork.c

kernel/fork.c renamed from kernel/fork.c~memcg-replace-mm-owner-with-mm-memcg +14 -3

@@ -878,10 +878,19 @@ static void mm_init_aio(struct mm_struct
 #endif
 }
 
-static void mm_init_owner(struct mm_struct *mm, struct task_struct *p)
+static void mm_init_memcg(struct mm_struct *mm)
 {
 #ifdef CONFIG_MEMCG
-	mm->owner = p;
+	struct cgroup_subsys_state *css;
+
+	/* Ensure mm->memcg is initialized */
+	mm->memcg = NULL;
+
+	rcu_read_lock();
+	css = task_css(current, memory_cgrp_id);
+	if (css && css_tryget(css))
+		mm_update_memcg(mm, mem_cgroup_from_css(css));
+	rcu_read_unlock();
 #endif
 }
 

@@ -912,7 +921,7 @@ static struct mm_struct *mm_init(struct
 	spin_lock_init(&mm->arg_lock);
 	mm_init_cpumask(mm);
 	mm_init_aio(mm);
-	mm_init_owner(mm, p);
+	mm_init_memcg(mm);
 	RCU_INIT_POINTER(mm->exe_file, NULL);
 	mmu_notifier_mm_init(mm);
 	hmm_mm_init(mm);

@@ -942,6 +951,7 @@ static struct mm_struct *mm_init(struct
 fail_nocontext:
 	mm_free_pgd(mm);
 fail_nopgd:
+	mm_update_memcg(mm, NULL);
 	free_mm(mm);
 	return NULL;
 }

@@ -979,6 +989,7 @@ static inline void __mmput(struct mm_str
 	}
 	if (mm->binfmt)
 		module_put(mm->binfmt->module);
+	mm_update_memcg(mm, NULL);
 	mmdrop(mm);
 }
 

diff -puN mm/debug.c~memcg-replace-mm-owner-with-mm-memcg mm/debug.c

mm/debug.c renamed from mm/debug.c~memcg-replace-mm-owner-with-mm-memcg +2 -2

@@ -116,7 +116,7 @@ void dump_mm(const struct mm_struct *mm)
 		"ioctx_table %px\n"
 #endif
 #ifdef CONFIG_MEMCG
-		"owner %px "
+		"memcg %px "
 #endif
 		"exe_file %px\n"
 #ifdef CONFIG_MMU_NOTIFIER

@@ -147,7 +147,7 @@ void dump_mm(const struct mm_struct *mm)
 		mm->ioctx_table,
 #endif
 #ifdef CONFIG_MEMCG
-		mm->owner,
+		mm->memcg,
 #endif
 		mm->exe_file,
 #ifdef CONFIG_MMU_NOTIFIER

diff -puN mm/memcontrol.c~memcg-replace-mm-owner-with-mm-memcg mm/memcontrol.c

mm/memcontrol.c renamed from mm/memcontrol.c~memcg-replace-mm-owner-with-mm-memcg +61 -20

@@ -664,20 +664,6 @@ static void memcg_check_events(struct me
 	}
 }
 
-struct mem_cgroup *mem_cgroup_from_task(struct task_struct *p)
-{
-	/*
-	 * mm_update_next_owner() may clear mm->owner to NULL
-	 * if it races with swapoff, page migration, etc.
-	 * So this can be called with p == NULL.
-	 */
-	if (unlikely(!p))
-		return NULL;
-
-	return mem_cgroup_from_css(task_css(p, memory_cgrp_id));
-}
-EXPORT_SYMBOL(mem_cgroup_from_task);
-
 struct mem_cgroup *get_mem_cgroup_from_mm(struct mm_struct *mm)
 {
 	struct mem_cgroup *memcg = NULL;

@@ -692,7 +678,7 @@ struct mem_cgroup *get_mem_cgroup_from_m
 		if (unlikely(!mm))
 			memcg = root_mem_cgroup;
 		else {
-			memcg = mem_cgroup_from_task(rcu_dereference(mm->owner));
+			memcg = rcu_dereference(mm->memcg);
 			if (unlikely(!memcg))
 				memcg = root_mem_cgroup;
 		}

@@ -1025,7 +1011,7 @@ bool task_in_mem_cgroup(struct task_stru
 		 * killed to prevent needlessly killing additional tasks.
 		 */
 		rcu_read_lock();
-		task_memcg = mem_cgroup_from_task(task);
+		task_memcg = mem_cgroup_from_css(task_css(task, memory_cgrp_id));
 		css_get(&task_memcg->css);
 		rcu_read_unlock();
 	}

@@ -4850,15 +4836,16 @@ static int mem_cgroup_can_attach(struct
 	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) {
+
+	/* We move charges except for creative uses of CLONE_VM */
+	if (mm->memcg == from) {
 		VM_BUG_ON(mc.from);
 		VM_BUG_ON(mc.to);
 		VM_BUG_ON(mc.precharge);

@@ -5058,6 +5045,58 @@ static void mem_cgroup_move_task(void)
 }
 #endif
 
+/**
+ * mm_update_memcg - Update the memory cgroup of a mm_struct
+ * @mm: mm struct
+ * @new: new memory cgroup value
+ *
+ * Called whenever mm->memcg needs to change.   Consumes a reference
+ * to new (unless new is NULL).   The reference to the old memory
+ * cgroup is decreased.
+ */
+void mm_update_memcg(struct mm_struct *mm, struct mem_cgroup *new)
+{
+	/* This is the only place where mm->memcg is changed */
+	struct mem_cgroup *old;
+
+	old = xchg(&mm->memcg, new);
+	if (old)
+		css_put(&old->css);
+}
+
+static void task_update_memcg(struct task_struct *tsk, struct mem_cgroup *new)
+{
+	struct mm_struct *mm;
+	task_lock(tsk);
+	mm = tsk->mm;
+	if (mm && !(tsk->flags & PF_KTHREAD))
+		mm_update_memcg(mm, new);
+	task_unlock(tsk);
+}
+
+static void mem_cgroup_attach(struct cgroup_taskset *tset)
+{
+	struct cgroup_subsys_state *css;
+	struct task_struct *tsk;
+
+	cgroup_taskset_for_each(tsk, css, tset) {
+		struct mem_cgroup *new = mem_cgroup_from_css(css);
+		css_get(css);
+		task_update_memcg(tsk, new);
+	}
+}
+
+void mm_sync_memcg_from_task(struct task_struct *tsk)
+{
+	struct cgroup_subsys_state *css;
+
+	rcu_read_lock();
+	css = task_css(tsk, memory_cgrp_id);
+	if (css && css_tryget(css))
+		task_update_memcg(tsk, mem_cgroup_from_css(css));
+	rcu_read_unlock();
+}
+
 /*
  * Cgroup retains root cgroups across [un]mount cycles making it necessary
  * to verify whether we're attached to the default hierarchy on each mount

@@ -5358,8 +5397,10 @@ struct cgroup_subsys memory_cgrp_subsys
 	.css_free = mem_cgroup_css_free,
 	.css_reset = mem_cgroup_css_reset,
 	.can_attach = mem_cgroup_can_attach,
+	.attach = mem_cgroup_attach,
 	.cancel_attach = mem_cgroup_cancel_attach,
 	.post_attach = mem_cgroup_move_task,
+	.fork = mm_sync_memcg_from_task,
 	.bind = mem_cgroup_bind,
 	.dfl_cftypes = memory_files,
 	.legacy_cftypes = mem_cgroup_legacy_files,

@@ -5846,7 +5887,7 @@ void mem_cgroup_sk_alloc(struct sock *sk
 	}
 
 	rcu_read_lock();
-	memcg = mem_cgroup_from_task(current);
+	memcg = mem_cgroup_from_css(task_css(current, memory_cgrp_id));
 	if (memcg == root_mem_cgroup)
 		goto out;
 	if (!cgroup_subsys_on_dfl(memory_cgrp_subsys) && !memcg->tcpmem_active)

_

Quoted 2 lines · On Thu, 24 May 2018 13:10:02 +0200 Michal Hocko <[email protected]> wrote:

I would really prefer and appreciate a repost with all the fixes folded in.

[1/2]

kernel/fork.c renamed from kernel/fork.c~memcg-replace-mm-owner-with-mm-memcg +1 -2

-static void mm_init_owner(struct mm_struct *mm, struct task_struct *p)
+static void mm_init_memcg(struct mm_struct *mm)
 {
 #ifdef CONFIG_MEMCG
-	mm->owner = p;