* [PATCH] use find_lock_task_mm in memory cgroups oom
@ 2010-06-15 6:24 KAMEZAWA Hiroyuki
2010-06-15 9:59 ` Minchan Kim
0 siblings, 1 reply; 5+ messages in thread
From: KAMEZAWA Hiroyuki @ 2010-06-15 6:24 UTC (permalink / raw
To: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org, nishimura@mxp.nes.nec.co.jp,
balbir@linux.vnet.ibm.com, Oleg Nesterov,
akpm@linux-foundation.org
based on oom-introduce-find_lock_task_mm-to-fix-mm-false-positives.patch
tested on mm-of-the-moment snapshot 2010-06-11-16-40.
==
From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
When the OOM killer scans task, it check a task is under memcg or
not when it's called via memcg's context.
But, as Oleg pointed out, a thread group leader may have NULL ->mm
and task_in_mem_cgroup() may do wrong decision. We have to use
find_lock_task_mm() in memcg as generic OOM-Killer does.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
Cc: Balbir Singh <balbir@linux.vnet.ibm.com>
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
include/linux/oom.h | 2 ++
mm/memcontrol.c | 10 +++++++---
mm/oom_kill.c | 8 ++++++--
3 files changed, 15 insertions(+), 5 deletions(-)
Index: mmotm-2.6.35-0611/include/linux/oom.h
===================================================================
--- mmotm-2.6.35-0611.orig/include/linux/oom.h
+++ mmotm-2.6.35-0611/include/linux/oom.h
@@ -45,6 +45,8 @@ static inline void oom_killer_enable(voi
oom_killer_disabled = false;
}
+extern struct task_struct *find_lock_task_mm(struct task_struct *p);
+
/* sysctls */
extern int sysctl_oom_dump_tasks;
extern int sysctl_oom_kill_allocating_task;
Index: mmotm-2.6.35-0611/mm/memcontrol.c
===================================================================
--- mmotm-2.6.35-0611.orig/mm/memcontrol.c
+++ mmotm-2.6.35-0611/mm/memcontrol.c
@@ -47,6 +47,7 @@
#include <linux/mm_inline.h>
#include <linux/page_cgroup.h>
#include <linux/cpu.h>
+#include <linux/oom.h>
#include "internal.h"
#include <asm/uaccess.h>
@@ -838,10 +839,13 @@ int task_in_mem_cgroup(struct task_struc
{
int ret;
struct mem_cgroup *curr = NULL;
+ struct task_struct *p;
- task_lock(task);
- curr = try_get_mem_cgroup_from_mm(task->mm);
- task_unlock(task);
+ p = find_lock_task_mm(task);
+ if (!p)
+ return 0;
+ curr = try_get_mem_cgroup_from_mm(p->mm);
+ task_unlock(p);
if (!curr)
return 0;
/*
Index: mmotm-2.6.35-0611/mm/oom_kill.c
===================================================================
--- mmotm-2.6.35-0611.orig/mm/oom_kill.c
+++ mmotm-2.6.35-0611/mm/oom_kill.c
@@ -81,13 +81,17 @@ static bool has_intersects_mems_allowed(
}
#endif /* CONFIG_NUMA */
-/*
+/**
+ * find_lock_task_mm - Checking a process which a task belongs to has valid mm
+ * and return a locked task which has a valid pointer to mm.
+ *
+ * @p: the task of a process to be checked.
* The process p may have detached its own ->mm while exiting or through
* use_mm(), but one or more of its subthreads may still have a valid
* pointer. Return p, or any of its subthreads with a valid ->mm, with
* task_lock() held.
*/
-static struct task_struct *find_lock_task_mm(struct task_struct *p)
+struct task_struct *find_lock_task_mm(struct task_struct *p)
{
struct task_struct *t = p;
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] use find_lock_task_mm in memory cgroups oom
2010-06-15 6:24 [PATCH] use find_lock_task_mm in memory cgroups oom KAMEZAWA Hiroyuki
@ 2010-06-15 9:59 ` Minchan Kim
2010-06-16 0:03 ` [PATCH] use find_lock_task_mm in memory cgroups oom v2 KAMEZAWA Hiroyuki
0 siblings, 1 reply; 5+ messages in thread
From: Minchan Kim @ 2010-06-15 9:59 UTC (permalink / raw
To: KAMEZAWA Hiroyuki
Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org,
nishimura@mxp.nes.nec.co.jp, balbir@linux.vnet.ibm.com,
Oleg Nesterov, akpm@linux-foundation.org
Hi, Kame.
On Tue, Jun 15, 2010 at 3:24 PM, KAMEZAWA Hiroyuki
<kamezawa.hiroyu@jp.fujitsu.com> wrote:
>
> based on oom-introduce-find_lock_task_mm-to-fix-mm-false-positives.patch
> tested on mm-of-the-moment snapshot 2010-06-11-16-40.
>
> ==
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
>
> When the OOM killer scans task, it check a task is under memcg or
> not when it's called via memcg's context.
>
> But, as Oleg pointed out, a thread group leader may have NULL ->mm
> and task_in_mem_cgroup() may do wrong decision. We have to use
> find_lock_task_mm() in memcg as generic OOM-Killer does.
>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> Cc: Balbir Singh <balbir@linux.vnet.ibm.com>
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Reviewed-by: Minchan Kim <minchan.kim@gmail.com>
I have a trivial comment below.
> ---
> include/linux/oom.h | 2 ++
> mm/memcontrol.c | 10 +++++++---
> mm/oom_kill.c | 8 ++++++--
> 3 files changed, 15 insertions(+), 5 deletions(-)
>
> Index: mmotm-2.6.35-0611/include/linux/oom.h
> ===================================================================
> --- mmotm-2.6.35-0611.orig/include/linux/oom.h
> +++ mmotm-2.6.35-0611/include/linux/oom.h
> @@ -45,6 +45,8 @@ static inline void oom_killer_enable(voi
> oom_killer_disabled = false;
> }
>
> +extern struct task_struct *find_lock_task_mm(struct task_struct *p);
> +
> /* sysctls */
> extern int sysctl_oom_dump_tasks;
> extern int sysctl_oom_kill_allocating_task;
> Index: mmotm-2.6.35-0611/mm/memcontrol.c
> ===================================================================
> --- mmotm-2.6.35-0611.orig/mm/memcontrol.c
> +++ mmotm-2.6.35-0611/mm/memcontrol.c
> @@ -47,6 +47,7 @@
> #include <linux/mm_inline.h>
> #include <linux/page_cgroup.h>
> #include <linux/cpu.h>
> +#include <linux/oom.h>
> #include "internal.h"
>
> #include <asm/uaccess.h>
> @@ -838,10 +839,13 @@ int task_in_mem_cgroup(struct task_struc
> {
> int ret;
> struct mem_cgroup *curr = NULL;
> + struct task_struct *p;
>
> - task_lock(task);
> - curr = try_get_mem_cgroup_from_mm(task->mm);
> - task_unlock(task);
> + p = find_lock_task_mm(task);
> + if (!p)
> + return 0;
> + curr = try_get_mem_cgroup_from_mm(p->mm);
> + task_unlock(p);
> if (!curr)
> return 0;
> /*
> Index: mmotm-2.6.35-0611/mm/oom_kill.c
> ===================================================================
> --- mmotm-2.6.35-0611.orig/mm/oom_kill.c
> +++ mmotm-2.6.35-0611/mm/oom_kill.c
> @@ -81,13 +81,17 @@ static bool has_intersects_mems_allowed(
> }
> #endif /* CONFIG_NUMA */
>
> -/*
> +/**
> + * find_lock_task_mm - Checking a process which a task belongs to has valid mm
> + * and return a locked task which has a valid pointer to mm.
> + *
This comment should have been another patch.
BTW, below comment uses "subthread" word.
Personally it's easy to understand function's goal to me. :)
How about following as?
Checking a process which has any subthread with vaild mm
....
> + * @p: the task of a process to be checked.
> * The process p may have detached its own ->mm while exiting or through
> * use_mm(), but one or more of its subthreads may still have a valid
> * pointer. Return p, or any of its subthreads with a valid ->mm, with
> * task_lock() held.
> */
> -static struct task_struct *find_lock_task_mm(struct task_struct *p)
> +struct task_struct *find_lock_task_mm(struct task_struct *p)
> {
> struct task_struct *t = p;
>
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>
--
Kind regards,
Minchan Kim
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] use find_lock_task_mm in memory cgroups oom v2
2010-06-15 9:59 ` Minchan Kim
@ 2010-06-16 0:03 ` KAMEZAWA Hiroyuki
2010-06-16 4:42 ` Balbir Singh
0 siblings, 1 reply; 5+ messages in thread
From: KAMEZAWA Hiroyuki @ 2010-06-16 0:03 UTC (permalink / raw
To: Minchan Kim
Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org,
nishimura@mxp.nes.nec.co.jp, balbir@linux.vnet.ibm.com,
Oleg Nesterov, akpm@linux-foundation.org
On Tue, 15 Jun 2010 18:59:25 +0900
Minchan Kim <minchan.kim@gmail.com> wrote:
> > -/*
> > +/**
> > + * find_lock_task_mm - Checking a process which a task belongs to has valid mm
> > + * and return a locked task which has a valid pointer to mm.
> > + *
>
> This comment should have been another patch.
> BTW, below comment uses "subthread" word.
> Personally it's easy to understand function's goal to me. :)
>
> How about following as?
> Checking a process which has any subthread with vaild mm
> ....
>
Sure. thank you. v2 is here. I removed unnecessary parts.
==
From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
When the OOM killer scans task, it check a task is under memcg or
not when it's called via memcg's context.
But, as Oleg pointed out, a thread group leader may have NULL ->mm
and task_in_mem_cgroup() may do wrong decision. We have to use
find_lock_task_mm() in memcg as generic OOM-Killer does.
Changelog:
- removed unnecessary changes in comments.
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
Cc: Balbir Singh <balbir@linux.vnet.ibm.com>
Reviewed-by: Minchan Kim <minchan.kim@gmail.com>
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
include/linux/oom.h | 2 ++
mm/memcontrol.c | 10 +++++++---
mm/oom_kill.c | 2 +-
3 files changed, 10 insertions(+), 4 deletions(-)
Index: mmotm-2.6.35-0611/include/linux/oom.h
===================================================================
--- mmotm-2.6.35-0611.orig/include/linux/oom.h
+++ mmotm-2.6.35-0611/include/linux/oom.h
@@ -45,6 +45,8 @@ static inline void oom_killer_enable(voi
oom_killer_disabled = false;
}
+extern struct task_struct *find_lock_task_mm(struct task_struct *p);
+
/* sysctls */
extern int sysctl_oom_dump_tasks;
extern int sysctl_oom_kill_allocating_task;
Index: mmotm-2.6.35-0611/mm/memcontrol.c
===================================================================
--- mmotm-2.6.35-0611.orig/mm/memcontrol.c
+++ mmotm-2.6.35-0611/mm/memcontrol.c
@@ -47,6 +47,7 @@
#include <linux/mm_inline.h>
#include <linux/page_cgroup.h>
#include <linux/cpu.h>
+#include <linux/oom.h>
#include "internal.h"
#include <asm/uaccess.h>
@@ -838,10 +839,13 @@ int task_in_mem_cgroup(struct task_struc
{
int ret;
struct mem_cgroup *curr = NULL;
+ struct task_struct *p;
- task_lock(task);
- curr = try_get_mem_cgroup_from_mm(task->mm);
- task_unlock(task);
+ p = find_lock_task_mm(task);
+ if (!p)
+ return 0;
+ curr = try_get_mem_cgroup_from_mm(p->mm);
+ task_unlock(p);
if (!curr)
return 0;
/*
Index: mmotm-2.6.35-0611/mm/oom_kill.c
===================================================================
--- mmotm-2.6.35-0611.orig/mm/oom_kill.c
+++ mmotm-2.6.35-0611/mm/oom_kill.c
@@ -87,7 +87,7 @@ static bool has_intersects_mems_allowed(
* pointer. Return p, or any of its subthreads with a valid ->mm, with
* task_lock() held.
*/
-static struct task_struct *find_lock_task_mm(struct task_struct *p)
+struct task_struct *find_lock_task_mm(struct task_struct *p)
{
struct task_struct *t = p;
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] use find_lock_task_mm in memory cgroups oom v2
2010-06-16 0:03 ` [PATCH] use find_lock_task_mm in memory cgroups oom v2 KAMEZAWA Hiroyuki
@ 2010-06-16 4:42 ` Balbir Singh
2010-06-16 4:51 ` KAMEZAWA Hiroyuki
0 siblings, 1 reply; 5+ messages in thread
From: Balbir Singh @ 2010-06-16 4:42 UTC (permalink / raw
To: KAMEZAWA Hiroyuki
Cc: Minchan Kim, linux-mm@kvack.org, linux-kernel@vger.kernel.org,
nishimura@mxp.nes.nec.co.jp, Oleg Nesterov,
akpm@linux-foundation.org
On Wed, Jun 16, 2010 at 5:33 AM, KAMEZAWA Hiroyuki
<kamezawa.hiroyu@jp.fujitsu.com> wrote:
> On Tue, 15 Jun 2010 18:59:25 +0900
> Minchan Kim <minchan.kim@gmail.com> wrote:
>
>> > -/*
>> > +/**
>> > + * find_lock_task_mm - Checking a process which a task belongs to has valid mm
>> > + * and return a locked task which has a valid pointer to mm.
>> > + *
>>
>> This comment should have been another patch.
>> BTW, below comment uses "subthread" word.
>> Personally it's easy to understand function's goal to me. :)
>>
>> How about following as?
>> Checking a process which has any subthread with vaild mm
>> ....
>>
> Sure. thank you. v2 is here. I removed unnecessary parts.
>
> ==
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
>
> When the OOM killer scans task, it check a task is under memcg or
> not when it's called via memcg's context.
>
> But, as Oleg pointed out, a thread group leader may have NULL ->mm
> and task_in_mem_cgroup() may do wrong decision. We have to use
> find_lock_task_mm() in memcg as generic OOM-Killer does.
>
> Changelog:
> - removed unnecessary changes in comments.
>
mm->owner solves the same problem, but unfortunately we have task
based selection in OOM killer, so we need this patch. It is quite
ironic that we find the mm from the task and then eventually the task
back from mm->owner and then the mem cgroup. If we already know the mm
from oom_kill.c, I think we can change the function to work off of
that. mm->owner->cgroup..no?
Balbir
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] use find_lock_task_mm in memory cgroups oom v2
2010-06-16 4:42 ` Balbir Singh
@ 2010-06-16 4:51 ` KAMEZAWA Hiroyuki
0 siblings, 0 replies; 5+ messages in thread
From: KAMEZAWA Hiroyuki @ 2010-06-16 4:51 UTC (permalink / raw
To: Balbir Singh
Cc: Minchan Kim, linux-mm@kvack.org, linux-kernel@vger.kernel.org,
nishimura@mxp.nes.nec.co.jp, Oleg Nesterov,
akpm@linux-foundation.org
On Wed, 16 Jun 2010 10:12:20 +0530
Balbir Singh <balbir@linux.vnet.ibm.com> wrote:
> On Wed, Jun 16, 2010 at 5:33 AM, KAMEZAWA Hiroyuki
> <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> > On Tue, 15 Jun 2010 18:59:25 +0900
> > Minchan Kim <minchan.kim@gmail.com> wrote:
> >
> >> > -/*
> >> > +/**
> >> > + * find_lock_task_mm - Checking a process which a task belongs to has valid mm
> >> > + * and return a locked task which has a valid pointer to mm.
> >> > + *
> >>
> >> This comment should have been another patch.
> >> BTW, below comment uses "subthread" word.
> >> Personally it's easy to understand function's goal to me. :)
> >>
> >> How about following as?
> >> Checking a process which has any subthread with vaild mm
> >> ....
> >>
> > Sure. thank you. v2 is here. I removed unnecessary parts.
> >
> > ==
> > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> >
> > When the OOM killer scans task, it check a task is under memcg or
> > not when it's called via memcg's context.
> >
> > But, as Oleg pointed out, a thread group leader may have NULL ->mm
> > and task_in_mem_cgroup() may do wrong decision. We have to use
> > find_lock_task_mm() in memcg as generic OOM-Killer does.
> >
> > Changelog:
> > - removed unnecessary changes in comments.
> >
>
> mm->owner solves the same problem, but unfortunately we have task
> based selection in OOM killer, so we need this patch. It is quite
> ironic that we find the mm from the task and then eventually the task
> back from mm->owner and then the mem cgroup. If we already know the mm
> from oom_kill.c, I think we can change the function to work off of
> that. mm->owner->cgroup..no?
>
There is no function as for_each_mm(). There is only for_each_process().
And, generally, there is no way to get a task via mm_struct.
To send signal etc. we need task.
(mm_owner is an option but not always configured and there will be
complicated problem as vfork() etc.)
I think oom_kill.c has to depend on thread, not mm, unfortunately.
Thanks,
-Kame
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-06-16 4:55 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-06-15 6:24 [PATCH] use find_lock_task_mm in memory cgroups oom KAMEZAWA Hiroyuki
2010-06-15 9:59 ` Minchan Kim
2010-06-16 0:03 ` [PATCH] use find_lock_task_mm in memory cgroups oom v2 KAMEZAWA Hiroyuki
2010-06-16 4:42 ` Balbir Singh
2010-06-16 4:51 ` KAMEZAWA Hiroyuki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).