mm : Avoid candidate task is equal to current task

PATCH In review Peter Chiang 6 participants 26 messages Apr 16, 2014 – Apr 23, 2014

Subsystems (3)

MM core Scheduler exec and binfmt

lore

Share & tools

lore

Peter Chiang

author

2014-04-16 03:04 UTC #0 lore raw

PATCH 1/1 mm : Avoid candidate task is equal to current task

From: pchiang <[email protected]>

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

Signed-off-by pchiang <[email protected]>

---
 kernel/exit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/exit.c +1 -1

index 6ed6a1d..aa98422 100644
@@ -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);

Signature · 1 line

1.8.1.5

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:[email protected]]
Sent: Wednesday, April 16, 2014 11:03 AM
To: [email protected]; Peter Chiang; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Cc: [email protected]
Subject: [PATCH] mm : Avoid candidate task is equal to current task

From: pchiang <[email protected]>

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

Signed-off-by pchiang <[email protected]>

---
 kernel/exit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/exit.c +1 -1

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

Peter Chiang

author

2014-04-16 12:52 UTC #2 lore raw

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; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Cc: [email protected]
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 .

From: pchiang <[email protected]>

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

Signed-off-by pchiang <[email protected]>

---
 kernel/exit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/exit.c +1 -1

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

Oleg Nesterov

2014-04-16 13:58 UTC #3 lore raw

Quoted 4 lines · 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 .

Confused, please see below.

Quoted 2 lines

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 ?

kernel/exit.c retry: · line 404

Quoted 11 lines

kernel/exit.c +1 -1

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

Peter Chiang

author

2014-04-17 06:48 UTC #4 lore raw

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:[email protected]]
Sent: Wednesday, April 16, 2014 9:58 PM
To: Peter Chiang
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH] mm : Avoid candidate task is equal to current task

Quoted 4 lines · 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 .

Confused, please see below.

Quoted 2 lines

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 ?

kernel/exit.c retry: · line 404

Quoted 12 lines

kernel/exit.c index 6ed6a1d..aa98422 renamed from kernel/exit.c +0 -0

100644

kernel/exit.c +1 -1

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

Oleg Nesterov

2014-04-18 16:24 UTC #5 lore raw

Quoted 10 lines · Peter Chiang

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.

Oleg Nesterov

2014-04-18 17:26 UTC #6 lore raw

cover memcg: mm_update_next_owner() should skip kthreads

Quoted 3 lines · Oleg Nesterov

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.

Oleg Nesterov

2014-04-18 17:27 UTC #7 lore raw

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

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

---
 kernel/exit.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

kernel/exit.c +4 -6

index ad7183a..e270d2a 100644
@@ -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

Signature · 1 line

1.5.5.1

Oleg Nesterov

2014-04-18 17:27 UTC #8 lore raw

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

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

---
 kernel/exit.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

kernel/exit.c +9 -3

index e270d2a..429659c 100644
@@ -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);
 	/*

Signature · 1 line

1.5.5.1

Michal Hocko

Reviewed-by on the v1 series

2014-04-18 18:24 UTC #9 lore raw

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

In reply to Oleg Nesterov

Quoted 8 lines · Oleg Nesterov

Quoted 3 lines · Oleg Nesterov
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 <[email protected]>

Signature · 2 lines

Michal Hocko
SUSE Labs

Oleg Nesterov

2014-04-18 18:45 UTC #10 lore raw

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

Quoted 7 lines · Michal Hocko

Quoted 5 lines · Oleg Nesterov
Quoted 3 lines · Oleg Nesterov
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.

Quoted 6 lines

Quoted 3 lines
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?

Quoted 2 lines

Anyway, feel free to add
Reviewed-by Michal Hocko <[email protected]>

Thanks!

Oleg.

Oleg Nesterov

2014-04-18 19:13 UTC #11 lore raw

cover memcg: kill start_kernel()->mm_init_owner(init_mm)

Quoted 5 lines · Oleg Nesterov

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

include/linux/mm_types.h +1 -1

index 8967e20..de16272 100644
@@ -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

include/linux/sched.h +2 -2

index 82fe3da..570a325 100644
@@ -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)

init/Kconfig +0 -7

index 765018c..143119e 100644
@@ -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

kernel/exit.c +2 -2

index 429659c..e5c4668 100644
@@ -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

kernel/fork.c +2 -2

index 54a8d26..ed75cf5 100644
@@ -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.

Oleg Nesterov

2014-04-18 19:14 UTC #12 lore raw

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

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

---
 init/main.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

init/main.c +0 -1

index 3815895..17090bb 100644
@@ -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();

Signature · 1 line

1.5.5.1

Michal Hocko

Reviewed-by on v1 1/1

2014-04-22 13:34 UTC #13 lore raw

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

Quoted 5 lines · 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.

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.

Quoted 4 lines

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

Reviewed-by Michal Hocko <[email protected]>

Quoted 20 lines

---
 init/main.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

init/main.c +0 -1

index 3815895..17090bb 100644
@@ -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();

Signature · 1 line

1.5.5.1

Signature · 2 lines

Michal Hocko
SUSE Labs

Michal Hocko

Acked-by on the v1 series

2014-04-22 13:24 UTC #14 lore raw

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

In reply to Oleg Nesterov

Quoted 9 lines · Oleg Nesterov

Quoted 5 lines · Oleg Nesterov
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 <[email protected]>

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.

Quoted 117 lines

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

include/linux/mm_types.h +1 -1

index 8967e20..de16272 100644
@@ -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

include/linux/sched.h +2 -2

index 82fe3da..570a325 100644
@@ -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)

init/Kconfig +0 -7

index 765018c..143119e 100644
@@ -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

kernel/exit.c +2 -2

index 429659c..e5c4668 100644
@@ -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

kernel/fork.c +2 -2

index 54a8d26..ed75cf5 100644
@@ -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.

Signature · 2 lines

Michal Hocko
SUSE Labs

Oleg Nesterov

2014-04-22 16:15 UTC #15 lore raw

cover memcg: kill CONFIG_MM_OWNER

Quoted 9 lines · Michal Hocko

Quoted 3 lines · Oleg Nesterov
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 <[email protected]>

Thanks, included.

Quoted 3 lines

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.

Oleg Nesterov

2014-04-22 16:16 UTC #16 lore raw

PATCH 1/1 memcg: kill CONFIG_MM_OWNER

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

Acked-by Michal Hocko <[email protected]>

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

include/linux/mm_types.h +1 -1

index 8967e20..de16272 100644
@@ -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

include/linux/sched.h +2 -2

index 82fe3da..570a325 100644
@@ -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)

init/Kconfig +0 -7

index 765018c..143119e 100644
@@ -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

kernel/exit.c +2 -2

index 429659c..e5c4668 100644
@@ -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

kernel/fork.c +2 -2

index 54a8d26..ed75cf5 100644
@@ -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.

Signature · 1 line

1.5.5.1

Johannes Weiner

Acked-by on v1 1/1

2014-04-22 16:40 UTC #17 lore raw

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

Quoted 6 lines · 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 <[email protected]>
Acked-by Michal Hocko <[email protected]>

Acked-by Johannes Weiner <[email protected]>

Michal Hocko

2014-04-22 16:59 UTC #18 lore raw

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

In reply to Oleg Nesterov

Quoted 11 lines · Oleg Nesterov

Quoted 9 lines · Michal Hocko
Quoted 3 lines · Oleg Nesterov
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 <[email protected]>

Quoted 7 lines

Thanks, included.
Quoted 3 lines
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!

Signature · 2 lines

Michal Hocko
SUSE Labs

Michal Hocko

2014-04-22 10:52 UTC #19 lore raw

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

In reply to Oleg Nesterov

Quoted 15 lines · Oleg Nesterov

Quoted 7 lines · Michal Hocko
Quoted 5 lines · Oleg Nesterov
Quoted 3 lines · Oleg Nesterov
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.
Quoted 6 lines
Quoted 3 lines
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.

Quoted 6 lines

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.

Quoted 2 lines

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.

Signature · 2 lines

Michal Hocko
SUSE Labs

Michal Hocko

2014-04-22 13:21 UTC #20 lore raw

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

Quoted 1 line · Michal Hocko

On Fri 18-04-14 20:44:41, Oleg Nesterov wrote:

[...]

Quoted 5 lines

Quoted 2 lines
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.

Signature · 2 lines

Michal Hocko
SUSE Labs

Hugh Dickins

2014-04-22 21:36 UTC #21 lore raw

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

Quoted 9 lines · Michal Hocko

Quoted 1 line · Michal Hocko
On Fri 18-04-14 20:44:41, Oleg Nesterov wrote:
[...]
Quoted 5 lines
Quoted 2 lines
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.

Quoted 7 lines

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

Michal Hocko

2014-04-23 07:04 UTC #22 lore raw

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

Quoted 18 lines · Hugh Dickins

Quoted 9 lines · Michal Hocko
Quoted 1 line · Michal Hocko
On Fri 18-04-14 20:44:41, Oleg Nesterov wrote:
[...]
Quoted 5 lines
Quoted 2 lines
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.
Quoted 7 lines
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.

Quoted 2 lines

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!

Signature · 2 lines

Michal Hocko
SUSE Labs

Pavel Machek

2014-04-19 08:35 UTC #23 lore raw

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

In reply to Oleg Nesterov

Hi!

Quoted 6 lines

Quoted 2 lines
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

Signature · 2 lines

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

Oleg Nesterov

2014-04-19 18:14 UTC #24 lore raw

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

Hi,

Quoted 10 lines · Pavel Machek

Quoted 7 lines
Quoted 3 lines
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.

Quoted 4 lines

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.

Hugh Dickins

2014-04-19 21:25 UTC #25 lore raw

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

Quoted 5 lines · Oleg Nesterov

Quoted 4 lines · Pavel Machek
Quoted 4 lines
Quoted 3 lines
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.

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
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.

Quoted 9 lines

Quoted 5 lines
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