[PATCH] mm : Avoid candidate task is equal to current task

[PATCH] mm : Avoid candidate task is equal to current task

[Peter Chiang]

From: pchiang <pchiang@nvidia.com>

Fix kernel panic when finding a new owner for the mm
and the new owner is equal to current onwer

Signed-off-by: pchiang <pchiang@nvidia.com>
---
 kernel/exit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index 6ed6a1d..aa98422 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -399,7 +399,7 @@ retry:
 	 * here often
 	 */
 	do_each_thread(g, c) {
-		if (c->mm == mm)
+		if ((c != p) && (c->mm == mm))
 			goto assign_new_owner;
 	} while_each_thread(g, c);
 
-- 
1.8.1.5

RE: [PATCH] mm : Avoid candidate task is equal to current task

[Peter Chiang]

mm_update_next_owner() call from exit_mm() , and exit_mm()  change tsk->mm to NULL
If p==c , It seems to be  impossible that mm == c->mm  (tsk->mm) . Because mm is non-NULL and  c->mm is NULL . 


-----Original Message-----
From: Peter Chiang [mailto:pchiang@nvidia.com] 
Sent: Wednesday, April 16, 2014 11:03 AM
To: ccross@android.com; Peter Chiang; lizefan@huawei.com; akpm@linux-foundation.org; oleg@redhat.com; tj@kernel.org; pavel@ucw.cz; ebiederm@xmission.com; guillaume@morinfr.org
Cc: linux-kernel@vger.kernel.org
Subject: [PATCH] mm : Avoid candidate task is equal to current task

From: pchiang <pchiang@nvidia.com>

Fix kernel panic when finding a new owner for the mm and the new owner is equal to current onwer

Signed-off-by: pchiang <pchiang@nvidia.com>
---
 kernel/exit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/exit.c b/kernel/exit.c index 6ed6a1d..aa98422 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -399,7 +399,7 @@ retry:
 	 * here often
 	 */
 	do_each_thread(g, c) {
-		if (c->mm == mm)
+		if ((c != p) && (c->mm == mm))
 			goto assign_new_owner;
 	} while_each_thread(g, c);
 
--
1.8.1.5

RE: [PATCH] mm : Avoid candidate task is equal to current task

[Peter Chiang]

Is it possible  before exit_mm() set tsk->mm to NULL, tsk->mm has already been NULL ?

-----Original Message-----
From: Peter Chiang 
Sent: Wednesday, April 16, 2014 8:31 PM
To: Peter Chiang; ccross@android.com; lizefan@huawei.com; akpm@linux-foundation.org; oleg@redhat.com; tj@kernel.org; pavel@ucw.cz; ebiederm@xmission.com; guillaume@morinfr.org
Cc: linux-kernel@vger.kernel.org
Subject: RE: [PATCH] mm : Avoid candidate task is equal to current task

mm_update_next_owner() call from exit_mm() , and exit_mm()  change tsk->mm to NULL If p==c , It seems to be  impossible that mm == c->mm  (tsk->mm) . Because mm is non-NULL and  c->mm is NULL . 


-----Original Message-----
From: Peter Chiang [mailto:pchiang@nvidia.com]
Sent: Wednesday, April 16, 2014 11:03 AM
To: ccross@android.com; Peter Chiang; lizefan@huawei.com; akpm@linux-foundation.org; oleg@redhat.com; tj@kernel.org; pavel@ucw.cz; ebiederm@xmission.com; guillaume@morinfr.org
Cc: linux-kernel@vger.kernel.org
Subject: [PATCH] mm : Avoid candidate task is equal to current task

From: pchiang <pchiang@nvidia.com>

Fix kernel panic when finding a new owner for the mm and the new owner is equal to current onwer

Signed-off-by: pchiang <pchiang@nvidia.com>
---
 kernel/exit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/exit.c b/kernel/exit.c index 6ed6a1d..aa98422 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -399,7 +399,7 @@ retry:
 	 * here often
 	 */
 	do_each_thread(g, c) {
-		if (c->mm == mm)
+		if ((c != p) && (c->mm == mm))
 			goto assign_new_owner;
 	} while_each_thread(g, c);
 
--
1.8.1.5

Re: [PATCH] mm : Avoid candidate task is equal to current task

[Oleg Nesterov]

On 04/16, Peter Chiang wrote:
>
> mm_update_next_owner() call from exit_mm() , and exit_mm()  change tsk->mm
> to NULL If p==c , It seems to be  impossible that mm == c->mm  (tsk->mm) .
> Because mm is non-NULL and  c->mm is NULL .

Confused, please see below.

> Fix kernel panic when finding a new owner for the mm and the new owner is
> equal to current onwer

Did you actually observe the panic ?

> diff --git a/kernel/exit.c b/kernel/exit.c index 6ed6a1d..aa98422 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -399,7 +399,7 @@ retry:
>  	 * here often
>  	 */
>  	do_each_thread(g, c) {
> -		if (c->mm == mm)
> +		if ((c != p) && (c->mm == mm))
>  			goto assign_new_owner;
>  	} while_each_thread(g, c);

p == current. This is always called with p->mm == NULL and mm != NULL.

So, if c->mm == mm then at least c->mm != NULL, and this means that
c == p is not possible?

And it seems that this is exactly what you meant above. So why do you
think we need this change?

Oleg.

RE: [PATCH] mm : Avoid candidate task is equal to current task

[Peter Chiang]

You are right !!  It is not a right solution .

It is a speculation where the panic was.

[38261.652100] Call trace: 
[38261.654616] [<ffffffc000aa6fe0>] mm_update_next_owner+0x190/0x238 
[38261.660766] [<ffffffc000aa728c>] do_exit+0x204/0x924 
[38261.665790] [<ffffffc000aa7a1c>] do_group_exit+0x40/0xcc 
[38261.671169] [<ffffffc000ab59cc>] get_signal_to_deliver+0x218/0x57c 
[38261.677409] [<ffffffc000a87e6c>] do_signal+0x534/0x550 
[38261.682608] [<ffffffc000a88070>] do_notify_resume+0x20/0x58

-----Original Message-----
From: Oleg Nesterov [mailto:oleg@redhat.com] 
Sent: Wednesday, April 16, 2014 9:58 PM
To: Peter Chiang
Cc: ccross@android.com; lizefan@huawei.com; akpm@linux-foundation.org; tj@kernel.org; pavel@ucw.cz; ebiederm@xmission.com; guillaume@morinfr.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] mm : Avoid candidate task is equal to current task

On 04/16, Peter Chiang wrote:
>
> mm_update_next_owner() call from exit_mm() , and exit_mm()  change 
> tsk->mm to NULL If p==c , It seems to be  impossible that mm == c->mm  (tsk->mm) .
> Because mm is non-NULL and  c->mm is NULL .

Confused, please see below.

> Fix kernel panic when finding a new owner for the mm and the new owner 
> is equal to current onwer

Did you actually observe the panic ?

> diff --git a/kernel/exit.c b/kernel/exit.c index 6ed6a1d..aa98422 
> 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -399,7 +399,7 @@ retry:
>  	 * here often
>  	 */
>  	do_each_thread(g, c) {
> -		if (c->mm == mm)
> +		if ((c != p) && (c->mm == mm))
>  			goto assign_new_owner;
>  	} while_each_thread(g, c);

p == current. This is always called with p->mm == NULL and mm != NULL.

So, if c->mm == mm then at least c->mm != NULL, and this means that c == p is not possible?

And it seems that this is exactly what you meant above. So why do you think we need this change?

Oleg.

Re: [PATCH] mm : Avoid candidate task is equal to current task

[Oleg Nesterov]

On 04/17, Peter Chiang wrote:
>
> It is a speculation where the panic was.
>
> [38261.652100] Call trace:
> [38261.654616] [<ffffffc000aa6fe0>] mm_update_next_owner+0x190/0x238
> [38261.660766] [<ffffffc000aa728c>] do_exit+0x204/0x924
> [38261.665790] [<ffffffc000aa7a1c>] do_group_exit+0x40/0xcc
> [38261.671169] [<ffffffc000ab59cc>] get_signal_to_deliver+0x218/0x57c
> [38261.677409] [<ffffffc000a87e6c>] do_signal+0x534/0x550
> [38261.682608] [<ffffffc000a88070>] do_notify_resume+0x20/0x58

Could you show the full dmesg?

Could you look into asm/objdump/addr2line to figure out what this
mm_update_next_owner+0x190 means?

Is it reproducable?

Hmm. I seem to see a bug in this function, it can be fulled by use_mm,
but I am not sure this can explain the problem. I'll send a patch.

Oleg.

[PATCH 0/2] memcg: mm_update_next_owner() should skip kthreads

[Oleg Nesterov]

On 04/18, Oleg Nesterov wrote:
>
> Hmm. I seem to see a bug in this function, it can be fulled by use_mm,
> but I am not sure this can explain the problem. I'll send a patch.

Untested, please review. But it really looks "obviously wrong", and note
that unuse_mm() doesn't do mm_update_next_owner(). (just in case, do not
confuse it with unuse_mm() in mm/swapfile.c).

Oleg.

[PATCH 1/2] memcg: mm_update_next_owner() should skip kthreads

[Oleg Nesterov]

"Search through everything else" in mm_update_next_owner() can
hit a kthread which adopted this "mm" via use_mm(), it should
not be used as mm->owner. Add the PF_KTHREAD check.

While at it, change this code to use for_each_process_thread()
instead of deprecated do_each_thread/while_each_thread.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/exit.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index ad7183a..e270d2a 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -356,14 +356,12 @@ retry:
 	}
 
 	/*
-	 * Search through everything else. We should not get
-	 * here often
+	 * Search through everything else, we should not get here often.
 	 */
-	do_each_thread(g, c) {
-		if (c->mm == mm)
+	for_each_process_thread(g, c) {
+		if (!(c->flags & PF_KTHREAD) && c->mm == mm)
 			goto assign_new_owner;
-	} while_each_thread(g, c);
-
+	}
 	read_unlock(&tasklist_lock);
 	/*
 	 * We found no owner yet mm_users > 1: this implies that we are
-- 
1.5.5.1

[PATCH 2/2] memcg: optimize the "Search everything else" loop in mm_update_next_owner()

[Oleg Nesterov]

for_each_process_thread() is sub-optimal. All threads share the same
->mm, we can swicth to the next process once we found a thread with
->mm != NULL and ->mm != mm.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/exit.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index e270d2a..429659c 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -358,9 +358,15 @@ retry:
 	/*
 	 * Search through everything else, we should not get here often.
 	 */
-	for_each_process_thread(g, c) {
-		if (!(c->flags & PF_KTHREAD) && c->mm == mm)
-			goto assign_new_owner;
+	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);
 	/*
-- 
1.5.5.1

Re: [PATCH 0/2] memcg: mm_update_next_owner() should skip kthreads

[Michal Hocko]

On Fri 18-04-14 19:26:31, Oleg Nesterov wrote:
> On 04/18, Oleg Nesterov wrote:
> >
> > Hmm. I seem to see a bug in this function, it can be fulled by use_mm,
> > but I am not sure this can explain the problem. I'll send a patch.
> 
> Untested, please review. But it really looks "obviously wrong", and note
> that unuse_mm() doesn't do mm_update_next_owner(). (just in case, do not
> confuse it with unuse_mm() in mm/swapfile.c).

Both patches seem to be correct but I am missinng why they are marked as
memcg: when they are touching generic mm_update_next_owner path.

Anyway, feel free to add
Reviewed-by: Michal Hocko <mhocko@suse.cz>
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 0/2] memcg: mm_update_next_owner() should skip kthreads

[Oleg Nesterov]

On 04/18, Michal Hocko wrote:
>
> On Fri 18-04-14 19:26:31, Oleg Nesterov wrote:
> > On 04/18, Oleg Nesterov wrote:
> > >
> > > Hmm. I seem to see a bug in this function, it can be fulled by use_mm,
> > > but I am not sure this can explain the problem. I'll send a patch.
> >
> > Untested, please review. But it really looks "obviously wrong", and note
> > that unuse_mm() doesn't do mm_update_next_owner(). (just in case, do not
> > confuse it with unuse_mm() in mm/swapfile.c).
>
> Both patches seem to be correct but I am missinng why they are marked as
> memcg: when they are touching generic mm_update_next_owner path.

Well, this is because I didn't know which prefix should I use. I looked
at git-blame to see who changed this function, picked the random 733eda7ac
"memcg: clear mm->owner when last possible owner leaves" commit and copied
"memcg" from there.

OTOH, mm->owner is used by mm/memcontrol.c, so perhaps the prefix is fine?

I do not even understand why do we have CONFIG_MM_OWNER, perhaps it should
die?

> Anyway, feel free to add
> Reviewed-by: Michal Hocko <mhocko@suse.cz>

Thanks!

Oleg.

[PATCH 0/1] memcg: kill start_kernel()->mm_init_owner(init_mm)

[Oleg Nesterov]

On 04/18, Oleg Nesterov wrote:
>
> OTOH, mm->owner is used by mm/memcontrol.c, so perhaps the prefix is fine?
>
> I do not even understand why do we have CONFIG_MM_OWNER, perhaps it should
> die?

Seriously what do you think about the patch below? Afaics CONFIG_MM_OWNER
is pointless.

And I just noticed start_kernel()->mm_init_owner(). It looks simply wrong
even if this is harmless, or I missed something?

Could you review 1/1 as well?

Oleg.
---

diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 8967e20..de16272 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -406,7 +406,7 @@ struct mm_struct {
 	spinlock_t			ioctx_lock;
 	struct kioctx_table __rcu	*ioctx_table;
 #endif
-#ifdef CONFIG_MM_OWNER
+#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
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 82fe3da..570a325 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2947,7 +2947,7 @@ static inline void inc_syscw(struct task_struct *tsk)
 #define TASK_SIZE_OF(tsk)	TASK_SIZE
 #endif
 
-#ifdef CONFIG_MM_OWNER
+#ifdef CONFIG_MEMCG
 extern void mm_update_next_owner(struct mm_struct *mm);
 extern void mm_init_owner(struct mm_struct *mm, struct task_struct *p);
 #else
@@ -2958,7 +2958,7 @@ static inline void mm_update_next_owner(struct mm_struct *mm)
 static inline void mm_init_owner(struct mm_struct *mm, struct task_struct *p)
 {
 }
-#endif /* CONFIG_MM_OWNER */
+#endif /* CONFIG_MEMCG */
 
 static inline unsigned long task_rlimit(const struct task_struct *tsk,
 		unsigned int limit)
diff --git a/init/Kconfig b/init/Kconfig
index 765018c..143119e 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -933,7 +933,6 @@ config RESOURCE_COUNTERS
 config MEMCG
 	bool "Memory Resource Controller for Control Groups"
 	depends on RESOURCE_COUNTERS
-	select MM_OWNER
 	select EVENTFD
 	help
 	  Provides a memory resource controller that manages both anonymous
@@ -951,9 +950,6 @@ config MEMCG
 	  disable memory resource controller and you can avoid overheads.
 	  (and lose benefits of memory resource controller)
 
-	  This config option also selects MM_OWNER config option, which
-	  could in turn add some fork/exit overhead.
-
 config MEMCG_SWAP
 	bool "Memory Resource Controller Swap Extension"
 	depends on MEMCG && SWAP
@@ -1173,9 +1169,6 @@ config SCHED_AUTOGROUP
 	  desktop applications.  Task group autogeneration is currently based
 	  upon task session.
 
-config MM_OWNER
-	bool
-
 config SYSFS_DEPRECATED
 	bool "Enable deprecated sysfs features to support old userspace tools"
 	depends on SYSFS
diff --git a/kernel/exit.c b/kernel/exit.c
index 429659c..e5c4668 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -313,7 +313,7 @@ kill_orphaned_pgrp(struct task_struct *tsk, struct task_struct *parent)
 	}
 }
 
-#ifdef CONFIG_MM_OWNER
+#ifdef CONFIG_MEMCG
 /*
  * A task is exiting.   If it owned this mm, find a new owner for the mm.
  */
@@ -399,7 +399,7 @@ assign_new_owner:
 	task_unlock(c);
 	put_task_struct(c);
 }
-#endif /* CONFIG_MM_OWNER */
+#endif /* CONFIG_MEMCG */
 
 /*
  * Turn us into a lazy TLB process if we
diff --git a/kernel/fork.c b/kernel/fork.c
index 54a8d26..ed75cf5 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1099,12 +1099,12 @@ static void rt_mutex_init_task(struct task_struct *p)
 #endif
 }
 
-#ifdef CONFIG_MM_OWNER
+#ifdef CONFIG_MEMCG
 void mm_init_owner(struct mm_struct *mm, struct task_struct *p)
 {
 	mm->owner = p;
 }
-#endif /* CONFIG_MM_OWNER */
+#endif /* CONFIG_MEMCG */
 
 /*
  * Initialize POSIX timer handling for a single task.

[PATCH 1/1] memcg: kill start_kernel()->mm_init_owner(init_mm)

[Oleg Nesterov]

Remove start_kernel()->mm_init_owner(&init_mm, &init_task).

This doesn't really hurt but unnecessary and misleading. init_task
is the "swapper" thread == current, its ->mm is always NULL. And
init_mm can only be used as ->active_mm, not as ->mm.

mm_init_owner() has a single caller with this patch, perhaps it
should die. mm_init() can initialize ->owner under #ifdef.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 init/main.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/init/main.c b/init/main.c
index 3815895..17090bb 100644
--- a/init/main.c
+++ b/init/main.c
@@ -507,7 +507,6 @@ asmlinkage void __init start_kernel(void)
 	page_address_init();
 	pr_notice("%s", linux_banner);
 	setup_arch(&command_line);
-	mm_init_owner(&init_mm, &init_task);
 	mm_init_cpumask(&init_mm);
 	setup_command_line(command_line);
 	setup_nr_cpu_ids();
-- 
1.5.5.1

Re: [PATCH 0/2] memcg: mm_update_next_owner() should skip kthreads

[Pavel Machek]

Hi!

> > Hmm. I seem to see a bug in this function, it can be fulled by use_mm,
> > but I am not sure this can explain the problem. I'll send a patch.
> 
> Untested, please review. But it really looks "obviously wrong", and note
> that unuse_mm() doesn't do mm_update_next_owner(). (just in case, do not
> confuse it with unuse_mm() in mm/swapfile.c).

Having two functions, one exported, one static with same name -- that
sounds quite evil, right?

mmu_context.c: * unuse_mm
mmu_context.c:void unuse_mm(struct mm_struct *mm)
mmu_context.c:EXPORT_SYMBOL_GPL(unuse_mm);
swapfile.c:static int unuse_mm(struct mm_struct *mm,
swapfile.c:	      		      retval = unuse_mm(start_mm,
entry, page);
swapfile.c:					retval = unuse_mm(mm,
entry, page);
swapfile.c:		 * or while we dropped it in unuse_mm().  The
page might even

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 0/2] memcg: mm_update_next_owner() should skip kthreads

[Oleg Nesterov]

Hi,

On 04/19, Pavel Machek wrote:
>
> > > Hmm. I seem to see a bug in this function, it can be fulled by use_mm,
> > > but I am not sure this can explain the problem. I'll send a patch.
> >
> > Untested, please review. But it really looks "obviously wrong", and note
> > that unuse_mm() doesn't do mm_update_next_owner(). (just in case, do not
> > confuse it with unuse_mm() in mm/swapfile.c).
>
> Having two functions, one exported, one static with same name -- that
> sounds quite evil, right?

Yes, agreed.

> mmu_context.c: * unuse_mm
> mmu_context.c:void unuse_mm(struct mm_struct *mm)
> mmu_context.c:EXPORT_SYMBOL_GPL(unuse_mm);
> swapfile.c:static int unuse_mm(struct mm_struct *mm,

Yes, I was thinking about s/unuse_mm/unswap_mm/ change in swapfile.c,
but then we should probaly rename other "unuse" functions there, and
shmem_unuse/try_to_unuse are not static.

Oleg.

Re: [PATCH 0/2] memcg: mm_update_next_owner() should skip kthreads

[Hugh Dickins]

On Sat, 19 Apr 2014, Oleg Nesterov wrote:
> On 04/19, Pavel Machek wrote:
> >
> > > > Hmm. I seem to see a bug in this function, it can be fulled by use_mm,
> > > > but I am not sure this can explain the problem. I'll send a patch.
> > >
> > > Untested, please review. But it really looks "obviously wrong", and note
> > > that unuse_mm() doesn't do mm_update_next_owner(). (just in case, do not
> > > confuse it with unuse_mm() in mm/swapfile.c).
> >
> > Having two functions, one exported, one static with same name -- that
> > sounds quite evil, right?
> 
> Yes, agreed.

Doubly agreed; though it seems to have escaped causing confusion
for several years, so no great hurry to resolve it.

> 
> > mmu_context.c: * unuse_mm
> > mmu_context.c:void unuse_mm(struct mm_struct *mm)
> > mmu_context.c:EXPORT_SYMBOL_GPL(unuse_mm);
> > swapfile.c:static int unuse_mm(struct mm_struct *mm,
> 
> Yes, I was thinking about s/unuse_mm/unswap_mm/ change in swapfile.c,
> but then we should probaly rename other "unuse" functions there, and
> shmem_unuse/try_to_unuse are not static.

My preference is "swapoff" instead of "unuse" throughout there:
it saves having to explain that "unuse" implies "swapoff" each
time we send in a patch making a change to that code.

But I'd prefer you hold off for the moment: Kelley Nielsen is currently
preparing a patchset reworking try_to_unuse, and I suggested a few days
ago to include such renaming in that patchset.

Partly driven by s390's recent 45961722f8e3 "mm: add support for
discard of unused ptes" which adds further confusion with pte_unused().

Hugh

Re: [PATCH 0/2] memcg: mm_update_next_owner() should skip kthreads

[Michal Hocko]

On Fri 18-04-14 20:44:41, Oleg Nesterov wrote:
> On 04/18, Michal Hocko wrote:
> >
> > On Fri 18-04-14 19:26:31, Oleg Nesterov wrote:
> > > On 04/18, Oleg Nesterov wrote:
> > > >
> > > > Hmm. I seem to see a bug in this function, it can be fulled by use_mm,
> > > > but I am not sure this can explain the problem. I'll send a patch.
> > >
> > > Untested, please review. But it really looks "obviously wrong", and note
> > > that unuse_mm() doesn't do mm_update_next_owner(). (just in case, do not
> > > confuse it with unuse_mm() in mm/swapfile.c).
> >
> > Both patches seem to be correct but I am missinng why they are marked as
> > memcg: when they are touching generic mm_update_next_owner path.
> 
> Well, this is because I didn't know which prefix should I use. I looked
> at git-blame to see who changed this function, picked the random 733eda7ac
> "memcg: clear mm->owner when last possible owner leaves" commit and copied
> "memcg" from there.
> 
> OTOH, mm->owner is used by mm/memcontrol.c, so perhaps the prefix is fine?

OK, I didn't realize memcg is the only user.

> I do not even understand why do we have CONFIG_MM_OWNER, perhaps it should
> die?

I have to dig into history to check why it has been introduced in the
first place. It might be possible it is not relevant anymore.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 0/2] memcg: mm_update_next_owner() should skip kthreads

[Michal Hocko]

On Tue 22-04-14 12:52:28, Michal Hocko wrote:
> On Fri 18-04-14 20:44:41, Oleg Nesterov wrote:
[...]
> > I do not even understand why do we have CONFIG_MM_OWNER, perhaps it should
> > die?
> 
> I have to dig into history to check why it has been introduced in the
> first place. It might be possible it is not relevant anymore.

There didn't seem to be any other user of CONFIG_MM_OWNER outside of
MEMCG so it seems that a separate config option seems like an overkill.
Regarding the mm->owner itself it is hard to live without it at the
moment. Most of the charging places do charge the current task_struct
but there are some that rely on mm and we would need mm->task mapping.
The last obstacle would be threads migration but that one should go away
with unified hierarchy AFAIR.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 0/1] memcg: kill start_kernel()->mm_init_owner(init_mm)

[Michal Hocko]

On Fri 18-04-14 21:12:59, Oleg Nesterov wrote:
> On 04/18, Oleg Nesterov wrote:
> >
> > OTOH, mm->owner is used by mm/memcontrol.c, so perhaps the prefix is fine?
> >
> > I do not even understand why do we have CONFIG_MM_OWNER, perhaps it should
> > die?
> 
> Seriously what do you think about the patch below? Afaics CONFIG_MM_OWNER
> is pointless.

Yes, I do not see any reason for a separate config when it has a single
user which cannot work without it. So feel free to add my
Acked-by: Michal Hocko <mhocko@suse.cz>

to the patch with your s-o-b and a simple changelog explaining there the
config option didn't have any user outside of MEMCG which cannot work
without it.

> And I just noticed start_kernel()->mm_init_owner(). It looks simply wrong
> even if this is harmless, or I missed something?
> 
> Could you review 1/1 as well?
> 
> Oleg.
> ---
> 
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 8967e20..de16272 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -406,7 +406,7 @@ struct mm_struct {
>  	spinlock_t			ioctx_lock;
>  	struct kioctx_table __rcu	*ioctx_table;
>  #endif
> -#ifdef CONFIG_MM_OWNER
> +#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
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 82fe3da..570a325 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -2947,7 +2947,7 @@ static inline void inc_syscw(struct task_struct *tsk)
>  #define TASK_SIZE_OF(tsk)	TASK_SIZE
>  #endif
>  
> -#ifdef CONFIG_MM_OWNER
> +#ifdef CONFIG_MEMCG
>  extern void mm_update_next_owner(struct mm_struct *mm);
>  extern void mm_init_owner(struct mm_struct *mm, struct task_struct *p);
>  #else
> @@ -2958,7 +2958,7 @@ static inline void mm_update_next_owner(struct mm_struct *mm)
>  static inline void mm_init_owner(struct mm_struct *mm, struct task_struct *p)
>  {
>  }
> -#endif /* CONFIG_MM_OWNER */
> +#endif /* CONFIG_MEMCG */
>  
>  static inline unsigned long task_rlimit(const struct task_struct *tsk,
>  		unsigned int limit)
> diff --git a/init/Kconfig b/init/Kconfig
> index 765018c..143119e 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -933,7 +933,6 @@ config RESOURCE_COUNTERS
>  config MEMCG
>  	bool "Memory Resource Controller for Control Groups"
>  	depends on RESOURCE_COUNTERS
> -	select MM_OWNER
>  	select EVENTFD
>  	help
>  	  Provides a memory resource controller that manages both anonymous
> @@ -951,9 +950,6 @@ config MEMCG
>  	  disable memory resource controller and you can avoid overheads.
>  	  (and lose benefits of memory resource controller)
>  
> -	  This config option also selects MM_OWNER config option, which
> -	  could in turn add some fork/exit overhead.
> -
>  config MEMCG_SWAP
>  	bool "Memory Resource Controller Swap Extension"
>  	depends on MEMCG && SWAP
> @@ -1173,9 +1169,6 @@ config SCHED_AUTOGROUP
>  	  desktop applications.  Task group autogeneration is currently based
>  	  upon task session.
>  
> -config MM_OWNER
> -	bool
> -
>  config SYSFS_DEPRECATED
>  	bool "Enable deprecated sysfs features to support old userspace tools"
>  	depends on SYSFS
> diff --git a/kernel/exit.c b/kernel/exit.c
> index 429659c..e5c4668 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -313,7 +313,7 @@ kill_orphaned_pgrp(struct task_struct *tsk, struct task_struct *parent)
>  	}
>  }
>  
> -#ifdef CONFIG_MM_OWNER
> +#ifdef CONFIG_MEMCG
>  /*
>   * A task is exiting.   If it owned this mm, find a new owner for the mm.
>   */
> @@ -399,7 +399,7 @@ assign_new_owner:
>  	task_unlock(c);
>  	put_task_struct(c);
>  }
> -#endif /* CONFIG_MM_OWNER */
> +#endif /* CONFIG_MEMCG */
>  
>  /*
>   * Turn us into a lazy TLB process if we
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 54a8d26..ed75cf5 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1099,12 +1099,12 @@ static void rt_mutex_init_task(struct task_struct *p)
>  #endif
>  }
>  
> -#ifdef CONFIG_MM_OWNER
> +#ifdef CONFIG_MEMCG
>  void mm_init_owner(struct mm_struct *mm, struct task_struct *p)
>  {
>  	mm->owner = p;
>  }
> -#endif /* CONFIG_MM_OWNER */
> +#endif /* CONFIG_MEMCG */
>  
>  /*
>   * Initialize POSIX timer handling for a single task.
> 

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/1] memcg: kill start_kernel()->mm_init_owner(init_mm)

[Michal Hocko]

On Fri 18-04-14 21:13:22, Oleg Nesterov wrote:
> Remove start_kernel()->mm_init_owner(&init_mm, &init_task).
> 
> This doesn't really hurt but unnecessary and misleading. init_task
> is the "swapper" thread == current, its ->mm is always NULL. And
> init_mm can only be used as ->active_mm, not as ->mm.

OK, this should work because mem_cgroup_from_task can handle NULL
task so even if somebody provided init_mm it will work properly.

But to be honest, I have no idea why mm_init_owner was called here.

> mm_init_owner() has a single caller with this patch, perhaps it
> should die. mm_init() can initialize ->owner under #ifdef.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Reviewed-by: Michal Hocko <mhocko@suse.cz>

> ---
>  init/main.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/init/main.c b/init/main.c
> index 3815895..17090bb 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -507,7 +507,6 @@ asmlinkage void __init start_kernel(void)
>  	page_address_init();
>  	pr_notice("%s", linux_banner);
>  	setup_arch(&command_line);
> -	mm_init_owner(&init_mm, &init_task);
>  	mm_init_cpumask(&init_mm);
>  	setup_command_line(command_line);
>  	setup_nr_cpu_ids();
> -- 
> 1.5.5.1
> 
> 

-- 
Michal Hocko
SUSE Labs

[PATCH 0/1] memcg: kill CONFIG_MM_OWNER

[Oleg Nesterov]

On 04/22, Michal Hocko wrote:
>
> On Fri 18-04-14 21:12:59, Oleg Nesterov wrote:
> >
> > Seriously what do you think about the patch below? Afaics CONFIG_MM_OWNER
> > is pointless.
>
> Yes, I do not see any reason for a separate config when it has a single
> user which cannot work without it. So feel free to add my
> Acked-by: Michal Hocko <mhocko@suse.cz>

Thanks, included.

> to the patch with your s-o-b and a simple changelog explaining there the
> config option didn't have any user outside of MEMCG which cannot work
> without it.

OK, please see 1/1.

Oleg.

[PATCH 1/1] memcg: kill CONFIG_MM_OWNER

[Oleg Nesterov]

CONFIG_MM_OWNER makes no sense. It is not user-selectable, it is only
selected by CONFIG_MEMCG automatically. So we can kill this option in
init/Kconfig and do s/CONFIG_MM_OWNER/CONFIG_MEMCG/ globally.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Michal Hocko <mhocko@suse.cz>
---
 include/linux/mm_types.h |    2 +-
 include/linux/sched.h    |    4 ++--
 init/Kconfig             |    7 -------
 kernel/exit.c            |    4 ++--
 kernel/fork.c            |    4 ++--
 5 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 8967e20..de16272 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -406,7 +406,7 @@ struct mm_struct {
 	spinlock_t			ioctx_lock;
 	struct kioctx_table __rcu	*ioctx_table;
 #endif
-#ifdef CONFIG_MM_OWNER
+#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
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 82fe3da..570a325 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2947,7 +2947,7 @@ static inline void inc_syscw(struct task_struct *tsk)
 #define TASK_SIZE_OF(tsk)	TASK_SIZE
 #endif
 
-#ifdef CONFIG_MM_OWNER
+#ifdef CONFIG_MEMCG
 extern void mm_update_next_owner(struct mm_struct *mm);
 extern void mm_init_owner(struct mm_struct *mm, struct task_struct *p);
 #else
@@ -2958,7 +2958,7 @@ static inline void mm_update_next_owner(struct mm_struct *mm)
 static inline void mm_init_owner(struct mm_struct *mm, struct task_struct *p)
 {
 }
-#endif /* CONFIG_MM_OWNER */
+#endif /* CONFIG_MEMCG */
 
 static inline unsigned long task_rlimit(const struct task_struct *tsk,
 		unsigned int limit)
diff --git a/init/Kconfig b/init/Kconfig
index 765018c..143119e 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -933,7 +933,6 @@ config RESOURCE_COUNTERS
 config MEMCG
 	bool "Memory Resource Controller for Control Groups"
 	depends on RESOURCE_COUNTERS
-	select MM_OWNER
 	select EVENTFD
 	help
 	  Provides a memory resource controller that manages both anonymous
@@ -951,9 +950,6 @@ config MEMCG
 	  disable memory resource controller and you can avoid overheads.
 	  (and lose benefits of memory resource controller)
 
-	  This config option also selects MM_OWNER config option, which
-	  could in turn add some fork/exit overhead.
-
 config MEMCG_SWAP
 	bool "Memory Resource Controller Swap Extension"
 	depends on MEMCG && SWAP
@@ -1173,9 +1169,6 @@ config SCHED_AUTOGROUP
 	  desktop applications.  Task group autogeneration is currently based
 	  upon task session.
 
-config MM_OWNER
-	bool
-
 config SYSFS_DEPRECATED
 	bool "Enable deprecated sysfs features to support old userspace tools"
 	depends on SYSFS
diff --git a/kernel/exit.c b/kernel/exit.c
index 429659c..e5c4668 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -313,7 +313,7 @@ kill_orphaned_pgrp(struct task_struct *tsk, struct task_struct *parent)
 	}
 }
 
-#ifdef CONFIG_MM_OWNER
+#ifdef CONFIG_MEMCG
 /*
  * A task is exiting.   If it owned this mm, find a new owner for the mm.
  */
@@ -399,7 +399,7 @@ assign_new_owner:
 	task_unlock(c);
 	put_task_struct(c);
 }
-#endif /* CONFIG_MM_OWNER */
+#endif /* CONFIG_MEMCG */
 
 /*
  * Turn us into a lazy TLB process if we
diff --git a/kernel/fork.c b/kernel/fork.c
index 54a8d26..ed75cf5 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1099,12 +1099,12 @@ static void rt_mutex_init_task(struct task_struct *p)
 #endif
 }
 
-#ifdef CONFIG_MM_OWNER
+#ifdef CONFIG_MEMCG
 void mm_init_owner(struct mm_struct *mm, struct task_struct *p)
 {
 	mm->owner = p;
 }
-#endif /* CONFIG_MM_OWNER */
+#endif /* CONFIG_MEMCG */
 
 /*
  * Initialize POSIX timer handling for a single task.
-- 
1.5.5.1

Re: [PATCH 1/1] memcg: kill CONFIG_MM_OWNER

[Johannes Weiner]

On Tue, Apr 22, 2014 at 06:15:32PM +0200, Oleg Nesterov wrote:
> CONFIG_MM_OWNER makes no sense. It is not user-selectable, it is only
> selected by CONFIG_MEMCG automatically. So we can kill this option in
> init/Kconfig and do s/CONFIG_MM_OWNER/CONFIG_MEMCG/ globally.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> Acked-by: Michal Hocko <mhocko@suse.cz>

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Re: [PATCH 0/1] memcg: kill CONFIG_MM_OWNER

[Michal Hocko]

On Tue 22-04-14 18:15:13, Oleg Nesterov wrote:
> On 04/22, Michal Hocko wrote:
> >
> > On Fri 18-04-14 21:12:59, Oleg Nesterov wrote:
> > >
> > > Seriously what do you think about the patch below? Afaics CONFIG_MM_OWNER
> > > is pointless.
> >
> > Yes, I do not see any reason for a separate config when it has a single
> > user which cannot work without it. So feel free to add my
> > Acked-by: Michal Hocko <mhocko@suse.cz>
> 
> Thanks, included.
> 
> > to the patch with your s-o-b and a simple changelog explaining there the
> > config option didn't have any user outside of MEMCG which cannot work
> > without it.
> 
> OK, please see 1/1.

Good. Thanks!

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 0/2] memcg: mm_update_next_owner() should skip kthreads

[Hugh Dickins]

On Tue, 22 Apr 2014, Michal Hocko wrote:
> On Tue 22-04-14 12:52:28, Michal Hocko wrote:
> > On Fri 18-04-14 20:44:41, Oleg Nesterov wrote:
> [...]
> > > I do not even understand why do we have CONFIG_MM_OWNER, perhaps it should
> > > die?
> > 
> > I have to dig into history to check why it has been introduced in the
> > first place. It might be possible it is not relevant anymore.
> 
> There didn't seem to be any other user of CONFIG_MM_OWNER outside of
> MEMCG so it seems that a separate config option seems like an overkill.
> Regarding the mm->owner itself it is hard to live without it at the
> moment. Most of the charging places do charge the current task_struct
> but there are some that rely on mm and we would need mm->task mapping.
> The last obstacle would be threads migration but that one should go away
> with unified hierarchy AFAIR.

Balbir had another user for mm->owner in mmotm back in 2008, his
memrlimit controller; but that didn't make it through to mainline.

Hugh

Re: [PATCH 0/2] memcg: mm_update_next_owner() should skip kthreads

[Michal Hocko]

On Tue 22-04-14 14:35:14, Hugh Dickins wrote:
> On Tue, 22 Apr 2014, Michal Hocko wrote:
> > On Tue 22-04-14 12:52:28, Michal Hocko wrote:
> > > On Fri 18-04-14 20:44:41, Oleg Nesterov wrote:
> > [...]
> > > > I do not even understand why do we have CONFIG_MM_OWNER, perhaps it should
> > > > die?
> > > 
> > > I have to dig into history to check why it has been introduced in the
> > > first place. It might be possible it is not relevant anymore.
> > 
> > There didn't seem to be any other user of CONFIG_MM_OWNER outside of
> > MEMCG so it seems that a separate config option seems like an overkill.
> > Regarding the mm->owner itself it is hard to live without it at the
> > moment. Most of the charging places do charge the current task_struct
> > but there are some that rely on mm and we would need mm->task mapping.
> > The last obstacle would be threads migration but that one should go away
> > with unified hierarchy AFAIR.
> 
> Balbir had another user for mm->owner in mmotm back in 2008, his
> memrlimit controller; but that didn't make it through to mainline.

I see, thanks for the reference, Hugh!
-- 
Michal Hocko
SUSE Labs