[patch -mm 6/5] memcontrol: move mm_cgroup to header file

[patch -mm 6/5] memcontrol: move mm_cgroup to header file

[David Rientjes]

Inline functions must preceed their use, so mm_cgroup() should be defined
in linux/memcontrol.h.

include/linux/memcontrol.h:48: warning: 'mm_cgroup' declared inline after
	being called
include/linux/memcontrol.h:48: warning: previous declaration of
	'mm_cgroup' was here

Cc: Balbir Singh <balbir@linux.vnet.ibm.com>
Signed-off-by: David Rientjes <rientjes@google.com>
---
 include/linux/memcontrol.h |    8 ++++++--
 mm/memcontrol.c            |    5 -----
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -45,7 +45,11 @@ extern unsigned long mem_cgroup_isolate_pages(unsigned long nr_to_scan,
 extern void mem_cgroup_out_of_memory(struct mem_cgroup *mem, gfp_t gfp_mask);
 extern int mem_cgroup_cache_charge(struct page *page, struct mm_struct *mm,
 					gfp_t gfp_mask);
-extern struct mem_cgroup *mm_cgroup(struct mm_struct *mm);
+
+static inline struct mem_cgroup *mm_cgroup(const struct mm_struct *mm)
+{
+	return rcu_dereference(mm->mem_cgroup);
+}
 
 static inline void mem_cgroup_uncharge_page(struct page *page)
 {
@@ -108,7 +112,7 @@ static inline int mem_cgroup_cache_charge(struct page *page,
 	return 0;
 }
 
-static inline struct mem_cgroup *mm_cgroup(struct mm_struct *mm)
+static inline struct mem_cgroup *mm_cgroup(const struct mm_struct *mm)
 {
 	return NULL;
 }
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -110,11 +110,6 @@ struct mem_cgroup *mem_cgroup_from_task(struct task_struct *p)
 				struct mem_cgroup, css);
 }
 
-inline struct mem_cgroup *mm_cgroup(struct mm_struct *mm)
-{
-	return rcu_dereference(mm->mem_cgroup);
-}
-
 void mm_init_cgroup(struct mm_struct *mm, struct task_struct *p)
 {
 	struct mem_cgroup *mem;

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

[patch -mm 7/5] oom: filter tasklist dump by mem_cgroup

[David Rientjes]

If an OOM was triggered as a result a cgroup's memory controller, the
tasklist shall be filtered to exclude tasks that are not a member of the
same group.

Creates a helper function to return non-zero if a task is a member of a
mem_cgroup:

	int task_in_mem_cgroup(const struct task_struct *task,
			       const struct mem_cgroup *mem);

Cc: Christoph Lameter <clameter@sgi.com>
Cc: Balbir Singh <balbir@linux.vnet.ibm.com>
Signed-off-by: David Rientjes <rientjes@google.com>
---
 include/linux/memcontrol.h |   16 ++++++++++++++++
 mm/oom_kill.c              |   25 ++++++++++++++-----------
 2 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -52,6 +52,16 @@ static inline void mem_cgroup_uncharge_page(struct page *page)
 	mem_cgroup_uncharge(page_get_page_cgroup(page));
 }
 
+/*
+ * Returns non-zero if the task is in the cgroup; otherwise returns zero.
+ * Call with task_lock(task) held.
+ */
+static inline int task_in_mem_cgroup(const struct task_struct *task,
+				     const struct mem_cgroup *mem)
+{
+	return mem && task->mm && mm_cgroup(task->mm) == mem;
+}
+
 #else /* CONFIG_CGROUP_MEM_CONT */
 static inline void mm_init_cgroup(struct mm_struct *mm,
 					struct task_struct *p)
@@ -103,6 +113,12 @@ static inline struct mem_cgroup *mm_cgroup(struct mm_struct *mm)
 	return NULL;
 }
 
+static inline int task_in_mem_cgroup(const struct task_struct *task,
+				     const struct mem_cgroup *mem)
+{
+	return 1;
+}
+
 #endif /* CONFIG_CGROUP_MEM_CONT */
 
 #endif /* _LINUX_MEMCONTROL_H */
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -65,13 +65,10 @@ unsigned long badness(struct task_struct *p, unsigned long uptime,
 		task_unlock(p);
 		return 0;
 	}
-
-#ifdef CONFIG_CGROUP_MEM_CONT
-	if (mem != NULL && mm->mem_cgroup != mem) {
+	if (!task_in_mem_cgroup(p, mem)) {
 		task_unlock(p);
 		return 0;
 	}
-#endif
 
 	/*
 	 * The memory size of the process is the basis for the badness.
@@ -274,9 +271,12 @@ static struct task_struct *select_bad_process(unsigned long *ppoints,
  * State information includes task's pid, uid, tgid, vm size, rss, cpu, oom_adj
  * score, and name.
  *
+ * If the actual is non-NULL, only tasks that are a member of the mem_cgroup are
+ * shown.
+ *
  * Call with tasklist_lock read-locked.
  */
-static void dump_tasks(void)
+static void dump_tasks(const struct mem_cgroup *mem)
 {
 	struct task_struct *g, *p;
 
@@ -291,6 +291,8 @@ static void dump_tasks(void)
 			continue;
 
 		task_lock(p);
+		if (!task_in_mem_cgroup(p, mem))
+			continue;
 		printk(KERN_INFO "[%5d] %5d %5d %8lu %8lu %3d     %3d %s\n",
 		       p->pid, p->uid, p->tgid, p->mm->total_vm,
 		       get_mm_rss(p->mm), (int)task_cpu(p), p->oomkilladj,
@@ -376,7 +378,8 @@ static int oom_kill_task(struct task_struct *p)
 }
 
 static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
-			    unsigned long points, const char *message)
+			    unsigned long points, struct mem_cgroup *mem,
+			    const char *message)
 {
 	struct task_struct *c;
 
@@ -387,7 +390,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
 		dump_stack();
 		show_mem();
 		if (sysctl_oom_dump_tasks)
-			dump_tasks();
+			dump_tasks(mem);
 	}
 
 	/*
@@ -428,7 +431,7 @@ retry:
 	if (!p)
 		p = current;
 
-	if (oom_kill_process(p, gfp_mask, 0, points,
+	if (oom_kill_process(p, gfp_mask, 0, points, mem,
 				"Memory cgroup out of memory"))
 		goto retry;
 out:
@@ -534,7 +537,7 @@ void out_of_memory(struct zonelist *zonelist, gfp_t gfp_mask, int order)
 
 	switch (constraint) {
 	case CONSTRAINT_MEMORY_POLICY:
-		oom_kill_process(current, gfp_mask, order, points,
+		oom_kill_process(current, gfp_mask, order, points, NULL,
 				"No available memory (MPOL_BIND)");
 		break;
 
@@ -544,7 +547,7 @@ void out_of_memory(struct zonelist *zonelist, gfp_t gfp_mask, int order)
 		/* Fall-through */
 	case CONSTRAINT_CPUSET:
 		if (sysctl_oom_kill_allocating_task) {
-			oom_kill_process(current, gfp_mask, order, points,
+			oom_kill_process(current, gfp_mask, order, points, NULL,
 					"Out of memory (oom_kill_allocating_task)");
 			break;
 		}
@@ -564,7 +567,7 @@ retry:
 			panic("Out of memory and no killable processes...\n");
 		}
 
-		if (oom_kill_process(p, points, gfp_mask, order,
+		if (oom_kill_process(p, points, gfp_mask, order, NULL,
 				     "Out of memory"))
 			goto retry;
 

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

Re: [patch -mm 7/5] oom: filter tasklist dump by mem_cgroup

[Balbir Singh]

David Rientjes wrote:
> If an OOM was triggered as a result a cgroup's memory controller, the
> tasklist shall be filtered to exclude tasks that are not a member of the
> same group.
> 
> Creates a helper function to return non-zero if a task is a member of a
> mem_cgroup:
> 
> 	int task_in_mem_cgroup(const struct task_struct *task,
> 			       const struct mem_cgroup *mem);
> 
> Cc: Christoph Lameter <clameter@sgi.com>
> Cc: Balbir Singh <balbir@linux.vnet.ibm.com>
> Signed-off-by: David Rientjes <rientjes@google.com>

Thanks for doing this. The number of parameters to OOM kill
have grown, may at the time of the next addition of parameter,
we should consider using a structure similar to scan_control
and pass the structure instead of all the parameters.


-- 
	Warm Regards,
	Balbir Singh
	Linux Technology Center
	IBM, ISTL

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

Re: [patch -mm 7/5] oom: filter tasklist dump by mem_cgroup

[David Rientjes]

On Tue, 25 Sep 2007, Balbir Singh wrote:

> > If an OOM was triggered as a result a cgroup's memory controller, the
> > tasklist shall be filtered to exclude tasks that are not a member of the
> > same group.
> > 
> > Creates a helper function to return non-zero if a task is a member of a
> > mem_cgroup:
> > 
> > 	int task_in_mem_cgroup(const struct task_struct *task,
> > 			       const struct mem_cgroup *mem);
> > 
> > Cc: Christoph Lameter <clameter@sgi.com>
> > Cc: Balbir Singh <balbir@linux.vnet.ibm.com>
> > Signed-off-by: David Rientjes <rientjes@google.com>
> 
> Thanks for doing this. The number of parameters to OOM kill
> have grown, may at the time of the next addition of parameter,
> we should consider using a structure similar to scan_control
> and pass the structure instead of all the parameters.
> 

I mentioned in the description of patch #5 in this set that the kernel 
will probably eventually want a generic tasklist dumping interface that 
allows users to specify what they want displayed for each task, even 
though that's going to introduce a large number of new flags like 
DUMP_PID, DUMP_TOTAL_VM_SIZE, etc.

It would be trivial to include a callback function to do the filtering for 
such a tasklist dumping interface that returns non-zero to display a task 
and zero otherwise.

So now our interface prototype looks like this:

	void dump_tasks(int (*filter)(const struct task_struct *),
		        unsigned long flags)

That's simple enough, but the work in converting other tasklist dumps over 
to using this interface and the number of flags this mechanism would 
require may not be so popular.  But, I agree, it's something that the 
kernel should have.

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

Re: [patch -mm 7/5] oom: filter tasklist dump by mem_cgroup

[Paul Menage]

It would be nice to be able to do the same thing for cpuset
membership, in the event that cpusets are active and the memory
controller is not.

Paul

On 9/25/07, David Rientjes <rientjes@google.com> wrote:
> If an OOM was triggered as a result a cgroup's memory controller, the
> tasklist shall be filtered to exclude tasks that are not a member of the
> same group.
>
> Creates a helper function to return non-zero if a task is a member of a
> mem_cgroup:
>
>         int task_in_mem_cgroup(const struct task_struct *task,
>                                const struct mem_cgroup *mem);
>
> Cc: Christoph Lameter <clameter@sgi.com>
> Cc: Balbir Singh <balbir@linux.vnet.ibm.com>
> Signed-off-by: David Rientjes <rientjes@google.com>
> ---
>  include/linux/memcontrol.h |   16 ++++++++++++++++
>  mm/oom_kill.c              |   25 ++++++++++++++-----------
>  2 files changed, 30 insertions(+), 11 deletions(-)
>
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -52,6 +52,16 @@ static inline void mem_cgroup_uncharge_page(struct page *page)
>         mem_cgroup_uncharge(page_get_page_cgroup(page));
>  }
>
> +/*
> + * Returns non-zero if the task is in the cgroup; otherwise returns zero.
> + * Call with task_lock(task) held.
> + */
> +static inline int task_in_mem_cgroup(const struct task_struct *task,
> +                                    const struct mem_cgroup *mem)
> +{
> +       return mem && task->mm && mm_cgroup(task->mm) == mem;
> +}
> +
>  #else /* CONFIG_CGROUP_MEM_CONT */
>  static inline void mm_init_cgroup(struct mm_struct *mm,
>                                         struct task_struct *p)
> @@ -103,6 +113,12 @@ static inline struct mem_cgroup *mm_cgroup(struct mm_struct *mm)
>         return NULL;
>  }
>
> +static inline int task_in_mem_cgroup(const struct task_struct *task,
> +                                    const struct mem_cgroup *mem)
> +{
> +       return 1;
> +}
> +
>  #endif /* CONFIG_CGROUP_MEM_CONT */
>
>  #endif /* _LINUX_MEMCONTROL_H */
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -65,13 +65,10 @@ unsigned long badness(struct task_struct *p, unsigned long uptime,
>                 task_unlock(p);
>                 return 0;
>         }
> -
> -#ifdef CONFIG_CGROUP_MEM_CONT
> -       if (mem != NULL && mm->mem_cgroup != mem) {
> +       if (!task_in_mem_cgroup(p, mem)) {
>                 task_unlock(p);
>                 return 0;
>         }
> -#endif
>
>         /*
>          * The memory size of the process is the basis for the badness.
> @@ -274,9 +271,12 @@ static struct task_struct *select_bad_process(unsigned long *ppoints,
>   * State information includes task's pid, uid, tgid, vm size, rss, cpu, oom_adj
>   * score, and name.
>   *
> + * If the actual is non-NULL, only tasks that are a member of the mem_cgroup are
> + * shown.
> + *
>   * Call with tasklist_lock read-locked.
>   */
> -static void dump_tasks(void)
> +static void dump_tasks(const struct mem_cgroup *mem)
>  {
>         struct task_struct *g, *p;
>
> @@ -291,6 +291,8 @@ static void dump_tasks(void)
>                         continue;
>
>                 task_lock(p);
> +               if (!task_in_mem_cgroup(p, mem))
> +                       continue;
>                 printk(KERN_INFO "[%5d] %5d %5d %8lu %8lu %3d     %3d %s\n",
>                        p->pid, p->uid, p->tgid, p->mm->total_vm,
>                        get_mm_rss(p->mm), (int)task_cpu(p), p->oomkilladj,
> @@ -376,7 +378,8 @@ static int oom_kill_task(struct task_struct *p)
>  }
>
>  static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
> -                           unsigned long points, const char *message)
> +                           unsigned long points, struct mem_cgroup *mem,
> +                           const char *message)
>  {
>         struct task_struct *c;
>
> @@ -387,7 +390,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
>                 dump_stack();
>                 show_mem();
>                 if (sysctl_oom_dump_tasks)
> -                       dump_tasks();
> +                       dump_tasks(mem);
>         }
>
>         /*
> @@ -428,7 +431,7 @@ retry:
>         if (!p)
>                 p = current;
>
> -       if (oom_kill_process(p, gfp_mask, 0, points,
> +       if (oom_kill_process(p, gfp_mask, 0, points, mem,
>                                 "Memory cgroup out of memory"))
>                 goto retry;
>  out:
> @@ -534,7 +537,7 @@ void out_of_memory(struct zonelist *zonelist, gfp_t gfp_mask, int order)
>
>         switch (constraint) {
>         case CONSTRAINT_MEMORY_POLICY:
> -               oom_kill_process(current, gfp_mask, order, points,
> +               oom_kill_process(current, gfp_mask, order, points, NULL,
>                                 "No available memory (MPOL_BIND)");
>                 break;
>
> @@ -544,7 +547,7 @@ void out_of_memory(struct zonelist *zonelist, gfp_t gfp_mask, int order)
>                 /* Fall-through */
>         case CONSTRAINT_CPUSET:
>                 if (sysctl_oom_kill_allocating_task) {
> -                       oom_kill_process(current, gfp_mask, order, points,
> +                       oom_kill_process(current, gfp_mask, order, points, NULL,
>                                         "Out of memory (oom_kill_allocating_task)");
>                         break;
>                 }
> @@ -564,7 +567,7 @@ retry:
>                         panic("Out of memory and no killable processes...\n");
>                 }
>
> -               if (oom_kill_process(p, points, gfp_mask, order,
> +               if (oom_kill_process(p, points, gfp_mask, order, NULL,
>                                      "Out of memory"))
>                         goto retry;
>
>
> --
> 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>
>

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

Re: [patch -mm 7/5] oom: filter tasklist dump by mem_cgroup

[Christoph Lameter]

On Tue, 25 Sep 2007, Paul Menage wrote:

> It would be nice to be able to do the same thing for cpuset
> membership, in the event that cpusets are active and the memory
> controller is not.

Maybe come up with some generic scheme that works for all types of memory 
controllers? cpusets is now a type of memory controller right?

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

Re: [patch -mm 7/5] oom: filter tasklist dump by mem_cgroup

[Paul Menage]

On 9/25/07, Christoph Lameter <clameter@sgi.com> wrote:
> On Tue, 25 Sep 2007, Paul Menage wrote:
>
> > It would be nice to be able to do the same thing for cpuset
> > membership, in the event that cpusets are active and the memory
> > controller is not.
>
> Maybe come up with some generic scheme that works for all types of memory
> controllers? cpusets is now a type of memory controller right?

Kind of, just the way it's always been. It's just a very different
model to Balbir's memory controller.

Incidentally, I'm considering splitting cpusets into two cgroup
subsystems, cpuset and memset, so that they can be more independent.
Mounting the old "cpuset" filesystem type would still get both of them
as before, so it would be backwards compatible.

Paul

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

Re: [patch -mm 7/5] oom: filter tasklist dump by mem_cgroup

[Paul Jackson]

On 9/25/07, David Rientjes <rientjes@google.com> wrote:
> If an OOM was triggered as a result a cgroup's memory controller, the
> tasklist shall be filtered to exclude tasks that are not a member of the
> same group.

Paul M replied:
> It would be nice to be able to do the same thing for cpuset
> membership, in the event that cpusets are active and the memory
> controller is not.

But cpusets can overlap.  For those configurations where we use
CONSTRAINT_CPUSET, I guess this doesn't matter, as we just shoot the
current task.  But what about configurations using overlapping cpusets
but not CONSTRAINT_CPUSET?

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.925.600.0401

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

Re: [patch -mm 7/5] oom: filter tasklist dump by mem_cgroup

[David Rientjes]

On Tue, 25 Sep 2007, Paul Jackson wrote:

> > It would be nice to be able to do the same thing for cpuset
> > membership, in the event that cpusets are active and the memory
> > controller is not.
> 
> But cpusets can overlap.  For those configurations where we use
> CONSTRAINT_CPUSET, I guess this doesn't matter, as we just shoot the
> current task.  But what about configurations using overlapping cpusets
> but not CONSTRAINT_CPUSET?
> 

CONSTRAINT_CPUSET isn't as simple as just killing current anymore in -mm.  
For that behavior, you need

	echo 1 > /proc/sys/vm/oom_kill_allocating_task

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

Re: [patch -mm 7/5] oom: filter tasklist dump by mem_cgroup

[Paul Jackson]

pj wrote:
> current task.  But what about configurations using overlapping cpusets
> but not CONSTRAINT_CPUSET?

David replied:
> CONSTRAINT_CPUSET isn't as simple as just killing current anymore in -mm.  
> For that behavior, you need
> 
> 	echo 1 > /proc/sys/vm/oom_kill_allocating_task

True.

... but what about configs with overlappnig cpusets that don't set
oom_kill_allocating_tasks ?

To connect back this back to the original point:

On 9/25/07, David Rientjes <rientjes@google.com> wrote:
> If an OOM was triggered as a result a cgroup's memory controller, the
> tasklist shall be filtered to exclude tasks that are not a member of the
> same group.

I would think that excluding tasks not in the same cpuset (if that's what
"not a member of the same group" would mean here) wouldn't be the right
thing to do, if the cpusets had overlapping mems_allowed and if we had
not set oom_kill_allocating_task.

... or am I still exposing my ignorance ??

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.925.600.0401

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

Re: [patch -mm 7/5] oom: filter tasklist dump by mem_cgroup

[David Rientjes]

On Tue, 25 Sep 2007, Paul Jackson wrote:

> > CONSTRAINT_CPUSET isn't as simple as just killing current anymore in -mm.  
> > For that behavior, you need
> > 
> > 	echo 1 > /proc/sys/vm/oom_kill_allocating_task
> 
> True.
> 
> ... but what about configs with overlappnig cpusets that don't set
> oom_kill_allocating_tasks ?
> 

The OOM killer in -mm no longer checks cpuset_excl_nodes_overlap() to 
select an overlapping task and, in fact, that function has been removed 
entirely from kernel/cpuset.c.

If oom_kill_allocating_tasks is zero (which it is by default), the 
tasklist is scanned and each task is checked for intersection with 
current's mems_allowed (task->mems_allowed, not dereferencing 
task->cpuset).  If it doesn't intersect, its "badness" score is divided by 
eight.

It is not necessarily eliminated from being a kill candidate, however, 
because it may have allocated memory on nodes that are not in 
task->mems_allowed in the past (such as GFP_ATOMIC allocations, tasks 
moved between cpusets, or cpusets with adjusted mems).

> > If an OOM was triggered as a result a cgroup's memory controller, the
> > tasklist shall be filtered to exclude tasks that are not a member of the
> > same group.
> 
> I would think that excluding tasks not in the same cpuset (if that's what
> "not a member of the same group" would mean here) wouldn't be the right
> thing to do, if the cpusets had overlapping mems_allowed and if we had
> not set oom_kill_allocating_task.
> 

Yes, absolutely.

I think Paul Menage is talking about filtering tasks that are not a member 
of the same cpuset because we're more familiar with mem_exclusive cpusets.  
So I think his suggestion was initially to filter based on overlapping 
mems_allowed instead, which makes sense.

	void dump_tasks(const struct mem_cgroup *mem)
	{
		struct task_struct *g, *p;

		do_each_thread(g, p) {
			...

			if (!task_in_mem_cgroup(p, mem)
				continue;
			if (!cpuset_mems_allowed_intersects(current, p))
				continue;

			/* show the task information */

		} while_each_thread(g, 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>

Re: [patch -mm 7/5] oom: filter tasklist dump by mem_cgroup

[David Rientjes]

On Tue, 25 Sep 2007, David Rientjes wrote:

> 	void dump_tasks(const struct mem_cgroup *mem)
> 	{
> 		struct task_struct *g, *p;
> 
> 		do_each_thread(g, p) {
> 			...
> 
> 			if (!task_in_mem_cgroup(p, mem)
> 				continue;
> 			if (!cpuset_mems_allowed_intersects(current, p))
> 				continue;
> 
> 			/* show the task information */
> 
> 		} while_each_thread(g, p);
> 	}
> 

By the way, the only reason I didn't code it like this was because tasks 
that overlap nodes in mems_allowed with the OOM-triggering task aren't 
necessarily excluded from being OOM killed, as I mentioned.  In other 
words, coding it like the above opens up the possibility of filtering the 
task that ends up getting killed.  Not a good idea.

Tasks that aren't in the same mem_cgroup, however, are filtered from the 
dump because they are explicitly excluded from being a target.  The check 
for that is actually misplaced and currently appears in badness() when it 
should appear in select_bad_process().

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

Re: [patch -mm 7/5] oom: filter tasklist dump by mem_cgroup

[Paul Jackson]

> The OOM killer in -mm no longer checks cpuset_excl_nodes_overlap() to 
> select an overlapping task and, in fact, that function has been removed 
> entirely from kernel/cpuset.c.
> 
> If oom_kill_allocating_tasks is zero (which it is by default), the 
> tasklist is scanned and each task is checked for intersection with 
> current's mems_allowed (task->mems_allowed, not dereferencing 
> task->cpuset).  If it doesn't intersect, its "badness" score is divided by 
> eight.

Yes - I recall seeing that change go by recently.  Seemed good to me.


> Yes, absolutely.
> 
> I think Paul Menage is talking about filtering tasks that are not a member 
> of the same cpuset because we're more familiar with mem_exclusive cpusets.  
> So I think his suggestion was initially to filter based on overlapping 
> mems_allowed instead, which makes sense.
> 
> 	void dump_tasks(const struct mem_cgroup *mem)

As Paul M realized in his reply shortly ago, I missed the simple and
essential detail that we were discussing the dump routine.

It makes more sense now - thanks.

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.925.600.0401

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

Re: [patch -mm 7/5] oom: filter tasklist dump by mem_cgroup

[Paul Menage]

On 9/25/07, Paul Jackson <pj@sgi.com> wrote:
> > It would be nice to be able to do the same thing for cpuset
> > membership, in the event that cpusets are active and the memory
> > controller is not.
>
> But cpusets can overlap.  For those configurations where we use
> CONSTRAINT_CPUSET, I guess this doesn't matter, as we just shoot the
> current task.  But what about configurations using overlapping cpusets
> but not CONSTRAINT_CPUSET?

You could print any tasks that share memory nodes (in their cpuset)
with the OOMing task.

Paul

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

Re: [patch -mm 7/5] oom: filter tasklist dump by mem_cgroup

[Paul Jackson]

Paul M wrote:
> You could print any tasks that share memory nodes (in their cpuset)
> with the OOMing task.

Huh?  I'm mystified.  Why would printing tasks help avoid choosing
the wrong task to oom kill?

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.925.600.0401

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

Re: [patch -mm 7/5] oom: filter tasklist dump by mem_cgroup

[Paul Menage]

On 9/25/07, Paul Jackson <pj@sgi.com> wrote:
> Paul M wrote:
> > You could print any tasks that share memory nodes (in their cpuset)
> > with the OOMing task.
>
> Huh?  I'm mystified.  Why would printing tasks help avoid choosing
> the wrong task to oom kill?

This whole dump is purely for debugging, to help figure out
post-mortem why the OOM occurred. By default the kernel prints all
tasks; this is an attempt to strip out irrelevant tasks.

Paul

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

Re: [patch -mm 7/5] oom: filter tasklist dump by mem_cgroup

[Paul Jackson]

> This whole dump is purely for debugging, to help figure out
> post-mortem why the OOM occurred. By default the kernel prints all
> tasks; this is an attempt to strip out irrelevant tasks.

aha - thanks

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.925.600.0401

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

Re: [patch -mm 6/5] memcontrol: move mm_cgroup to header file

[Balbir Singh]

David Rientjes wrote:
> Inline functions must preceed their use, so mm_cgroup() should be defined
> in linux/memcontrol.h.
> 
> include/linux/memcontrol.h:48: warning: 'mm_cgroup' declared inline after
> 	being called
> include/linux/memcontrol.h:48: warning: previous declaration of
> 	'mm_cgroup' was here
> 
> Cc: Balbir Singh <balbir@linux.vnet.ibm.com>
> Signed-off-by: David Rientjes <rientjes@google.com>

Is this a new warning or have you seen this earlier. I don't see the
warning in any of the versions upto 2.6.23-rc7-mm1. I'll check
the compilation output again and of-course 2.6.23-rc8-mm1


-- 
	Warm Regards,
	Balbir Singh
	Linux Technology Center
	IBM, ISTL

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

Re: [patch -mm 6/5] memcontrol: move mm_cgroup to header file

[David Rientjes]

On Tue, 25 Sep 2007, Balbir Singh wrote:

> > Inline functions must preceed their use, so mm_cgroup() should be defined
> > in linux/memcontrol.h.
> > 
> > include/linux/memcontrol.h:48: warning: 'mm_cgroup' declared inline after
> > 	being called
> > include/linux/memcontrol.h:48: warning: previous declaration of
> > 	'mm_cgroup' was here
> > 
> > Cc: Balbir Singh <balbir@linux.vnet.ibm.com>
> > Signed-off-by: David Rientjes <rientjes@google.com>
> 
> Is this a new warning or have you seen this earlier. I don't see the
> warning in any of the versions upto 2.6.23-rc7-mm1. I'll check
> the compilation output again and of-course 2.6.23-rc8-mm1
> 

It was produced when I implemented the filtering for the task dump with 
respect to cgroups, that's why this fix is included in a series that 
applies to the OOM killer.

Inline functions always need to preceed their use, otherwise the compiler 
can't inline them and they become normal functions.  So the quick rule is 
that inline functions are always "static inline," which this one was not.  
Those functions are always available in only one source file or declared 
in a header (and usually included before source file code) so that you 
don't need to worry about the declaration and use ordering.

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