[-mm patch] Show memcg information during OOM

[-mm patch] Show memcg information during OOM

[Balbir Singh]

Hi, All,

I found the following patch useful while debugging the memory
controller. It adds additional information if memcg invoked the OOM.

Comments, Suggestions?

From: Balbir Singh <balbir@linux.vnet.ibm.com>

Description: Add RSS and swap to OOM output from memcg

This patch displays memcg values like failcnt, usage and limit
when an OOM occurs due to memcg.

Signed-off-by: Balbir Singh <balbir@linux.vnet.ibm.com>
---

 include/linux/memcontrol.h |    5 +++++
 mm/memcontrol.c            |   15 +++++++++++++++
 mm/oom_kill.c              |    1 +
 3 files changed, 21 insertions(+), 0 deletions(-)


diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 326f45c..2ce1737 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -104,6 +104,7 @@ struct zone_reclaim_stat *mem_cgroup_get_reclaim_stat(struct mem_cgroup *memcg,
 						      struct zone *zone);
 struct zone_reclaim_stat*
 mem_cgroup_get_reclaim_stat_from_page(struct page *page);
+extern void mem_cgroup_print_mem_info(struct mem_cgroup *memcg);
 
 #ifdef CONFIG_CGROUP_MEM_RES_CTLR_SWAP
 extern int do_swap_account;
@@ -270,6 +271,10 @@ mem_cgroup_get_reclaim_stat_from_page(struct page *page)
 	return NULL;
 }
 
+void mem_cgroup_print_mem_info(struct mem_cgroup *memcg)
+{
+}
+
 #endif /* CONFIG_CGROUP_MEM_CONT */
 
 #endif /* _LINUX_MEMCONTROL_H */
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 8e4be9c..75eae85 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -813,6 +813,21 @@ bool mem_cgroup_oom_called(struct task_struct *task)
 	rcu_read_unlock();
 	return ret;
 }
+
+void mem_cgroup_print_mem_info(struct mem_cgroup *memcg)
+{
+	printk(KERN_WARNING "Memory cgroups's name %s\n",
+		memcg->css.cgroup->dentry->d_name.name);
+	printk(KERN_WARNING "Memory cgroup RSS : usage %llu, limit %llu"
+		" failcnt %llu\n", res_counter_read_u64(&memcg->res, RES_USAGE),
+		res_counter_read_u64(&memcg->res, RES_LIMIT),
+		res_counter_read_u64(&memcg->res, RES_FAILCNT));
+	printk(KERN_WARNING "Memory cgroup swap: usage %llu, limit %llu "
+		"failcnt %llu\n", res_counter_read_u64(&memcg->res, RES_USAGE),
+		res_counter_read_u64(&memcg->res, RES_LIMIT),
+		res_counter_read_u64(&memcg->res, RES_FAILCNT));
+}
+
 /*
  * Unlike exported interface, "oom" parameter is added. if oom==true,
  * oom-killer can be invoked.
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index d3b9bac..b8e53ae 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -392,6 +392,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
 			current->comm, gfp_mask, order, current->oomkilladj);
 		task_lock(current);
 		cpuset_print_task_mems_allowed(current);
+		mem_cgroup_print_mem_info(mem);
 		task_unlock(current);
 		dump_stack();
 		show_mem();

-- 
	Balbir

--
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: [-mm patch] Show memcg information during OOM

[KOSAKI Motohiro]

Hi

> +void mem_cgroup_print_mem_info(struct mem_cgroup *memcg)
> +{
> +	printk(KERN_WARNING "Memory cgroups's name %s\n",
> +		memcg->css.cgroup->dentry->d_name.name);
> +	printk(KERN_WARNING "Memory cgroup RSS : usage %llu, limit %llu"
> +		" failcnt %llu\n", res_counter_read_u64(&memcg->res, RES_USAGE),
> +		res_counter_read_u64(&memcg->res, RES_LIMIT),
> +		res_counter_read_u64(&memcg->res, RES_FAILCNT));
> +	printk(KERN_WARNING "Memory cgroup swap: usage %llu, limit %llu "
> +		"failcnt %llu\n", res_counter_read_u64(&memcg->res, RES_USAGE),
> +		res_counter_read_u64(&memcg->res, RES_LIMIT),
> +		res_counter_read_u64(&memcg->res, RES_FAILCNT));

s/res/memsw/ ?

and, I don't like the name of "Memory cgroup RSS" and "Memory cgroup swap".
it seems misleading. memcg->res doesn't only count count rss, but also cache.
memcg->memsw doesn't only count swap, but also memory.

otherthing, I think it is good patch for me :)


> +}
> +
>  /*
>   * Unlike exported interface, "oom" parameter is added. if oom==true,
>   * oom-killer can be invoked.
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index d3b9bac..b8e53ae 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -392,6 +392,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
>  			current->comm, gfp_mask, order, current->oomkilladj);
>  		task_lock(current);
>  		cpuset_print_task_mems_allowed(current);
> +		mem_cgroup_print_mem_info(mem);
>  		task_unlock(current);
>  		dump_stack();
>  		show_mem();
> 
> -- 
> 	Balbir
> 
> --
> 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: [-mm patch] Show memcg information during OOM

[Johannes Weiner]

On Mon, Feb 02, 2009 at 06:22:40PM +0530, Balbir Singh wrote:
> Hi, All,
> 
> I found the following patch useful while debugging the memory
> controller. It adds additional information if memcg invoked the OOM.
> 
> Comments, Suggestions?
> 
> From: Balbir Singh <balbir@linux.vnet.ibm.com>
> 
> Description: Add RSS and swap to OOM output from memcg
> 
> This patch displays memcg values like failcnt, usage and limit
> when an OOM occurs due to memcg.
> 
> Signed-off-by: Balbir Singh <balbir@linux.vnet.ibm.com>
> ---
> 
>  include/linux/memcontrol.h |    5 +++++
>  mm/memcontrol.c            |   15 +++++++++++++++
>  mm/oom_kill.c              |    1 +
>  3 files changed, 21 insertions(+), 0 deletions(-)
> 
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 326f45c..2ce1737 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -104,6 +104,7 @@ struct zone_reclaim_stat *mem_cgroup_get_reclaim_stat(struct mem_cgroup *memcg,
>  						      struct zone *zone);
>  struct zone_reclaim_stat*
>  mem_cgroup_get_reclaim_stat_from_page(struct page *page);
> +extern void mem_cgroup_print_mem_info(struct mem_cgroup *memcg);
>  
>  #ifdef CONFIG_CGROUP_MEM_RES_CTLR_SWAP
>  extern int do_swap_account;
> @@ -270,6 +271,10 @@ mem_cgroup_get_reclaim_stat_from_page(struct page *page)
>  	return NULL;
>  }
>  
> +void mem_cgroup_print_mem_info(struct mem_cgroup *memcg)
> +{
> +}
> +
>  #endif /* CONFIG_CGROUP_MEM_CONT */
>  
>  #endif /* _LINUX_MEMCONTROL_H */
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 8e4be9c..75eae85 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -813,6 +813,21 @@ bool mem_cgroup_oom_called(struct task_struct *task)
>  	rcu_read_unlock();
>  	return ret;
>  }
> +
> +void mem_cgroup_print_mem_info(struct mem_cgroup *memcg)
> +{
> +	printk(KERN_WARNING "Memory cgroups's name %s\n",
> +		memcg->css.cgroup->dentry->d_name.name);
> +	printk(KERN_WARNING "Memory cgroup RSS : usage %llu, limit %llu"
> +		" failcnt %llu\n", res_counter_read_u64(&memcg->res, RES_USAGE),
> +		res_counter_read_u64(&memcg->res, RES_LIMIT),
> +		res_counter_read_u64(&memcg->res, RES_FAILCNT));
> +	printk(KERN_WARNING "Memory cgroup swap: usage %llu, limit %llu "
> +		"failcnt %llu\n", res_counter_read_u64(&memcg->res, RES_USAGE),
> +		res_counter_read_u64(&memcg->res, RES_LIMIT),
> +		res_counter_read_u64(&memcg->res, RES_FAILCNT));
> +}
> +
>  /*
>   * Unlike exported interface, "oom" parameter is added. if oom==true,
>   * oom-killer can be invoked.
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index d3b9bac..b8e53ae 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -392,6 +392,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
>  			current->comm, gfp_mask, order, current->oomkilladj);
>  		task_lock(current);
>  		cpuset_print_task_mems_allowed(current);
> +		mem_cgroup_print_mem_info(mem);

mem is only !NULL when we come from mem_cgroup_out_of_memory().  This
crashes otherwise in mem_cgroup_print_mem_info(), no?

	Hannes

--
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: [-mm patch] Show memcg information during OOM

[Balbir Singh]

* Balbir Singh <balbir@linux.vnet.ibm.com> [2009-02-02 18:22:40]:

> Hi, All,
> 
> I found the following patch useful while debugging the memory
> controller. It adds additional information if memcg invoked the OOM.
> 
> Comments, Suggestions?
>


Description: Add RSS and swap to OOM output from memcg

From: Balbir Singh <balbir@linux.vnet.ibm.com>

This patch displays memcg values like failcnt, usage and limit
when an OOM occurs due to memcg.

Signed-off-by: Balbir Singh <balbir@linux.vnet.ibm.com>
---

 include/linux/memcontrol.h |    5 +++++
 mm/memcontrol.c            |   16 ++++++++++++++++
 mm/oom_kill.c              |    1 +
 3 files changed, 22 insertions(+), 0 deletions(-)


diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 326f45c..2ce1737 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -104,6 +104,7 @@ struct zone_reclaim_stat *mem_cgroup_get_reclaim_stat(struct mem_cgroup *memcg,
 						      struct zone *zone);
 struct zone_reclaim_stat*
 mem_cgroup_get_reclaim_stat_from_page(struct page *page);
+extern void mem_cgroup_print_mem_info(struct mem_cgroup *memcg);
 
 #ifdef CONFIG_CGROUP_MEM_RES_CTLR_SWAP
 extern int do_swap_account;
@@ -270,6 +271,10 @@ mem_cgroup_get_reclaim_stat_from_page(struct page *page)
 	return NULL;
 }
 
+void mem_cgroup_print_mem_info(struct mem_cgroup *memcg)
+{
+}
+
 #endif /* CONFIG_CGROUP_MEM_CONT */
 
 #endif /* _LINUX_MEMCONTROL_H */
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 8e4be9c..d134a1d 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -813,6 +813,22 @@ bool mem_cgroup_oom_called(struct task_struct *task)
 	rcu_read_unlock();
 	return ret;
 }
+
+void mem_cgroup_print_mem_info(struct mem_cgroup *memcg)
+{
+	printk(KERN_WARNING "Memory cgroups's name %s\n",
+		memcg->css.cgroup->dentry->d_name.name);
+	printk(KERN_WARNING "Memory cgroup RSS : usage %llu, limit %llu"
+		" failcnt %llu\n", res_counter_read_u64(&memcg->res, RES_USAGE),
+		res_counter_read_u64(&memcg->res, RES_LIMIT),
+		res_counter_read_u64(&memcg->res, RES_FAILCNT));
+	printk(KERN_WARNING "Memory cgroup swap: usage %llu, limit %llu "
+		"failcnt %llu\n",
+		res_counter_read_u64(&memcg->memsw, RES_USAGE),
+		res_counter_read_u64(&memcg->memsw, RES_LIMIT),
+		res_counter_read_u64(&memcg->memsw, RES_FAILCNT));
+}
+
 /*
  * Unlike exported interface, "oom" parameter is added. if oom==true,
  * oom-killer can be invoked.
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index d3b9bac..b8e53ae 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -392,6 +392,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
 			current->comm, gfp_mask, order, current->oomkilladj);
 		task_lock(current);
 		cpuset_print_task_mems_allowed(current);
+		mem_cgroup_print_mem_info(mem);
 		task_unlock(current);
 		dump_stack();
 		show_mem();

-- 
	Balbir

--
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: [-mm patch] Show memcg information during OOM

[Balbir Singh]

* KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> [2009-02-02 21:59:34]:

> Hi
> 
> > +void mem_cgroup_print_mem_info(struct mem_cgroup *memcg)
> > +{
> > +	printk(KERN_WARNING "Memory cgroups's name %s\n",
> > +		memcg->css.cgroup->dentry->d_name.name);
> > +	printk(KERN_WARNING "Memory cgroup RSS : usage %llu, limit %llu"
> > +		" failcnt %llu\n", res_counter_read_u64(&memcg->res, RES_USAGE),
> > +		res_counter_read_u64(&memcg->res, RES_LIMIT),
> > +		res_counter_read_u64(&memcg->res, RES_FAILCNT));
> > +	printk(KERN_WARNING "Memory cgroup swap: usage %llu, limit %llu "
> > +		"failcnt %llu\n", res_counter_read_u64(&memcg->res, RES_USAGE),
> > +		res_counter_read_u64(&memcg->res, RES_LIMIT),
> > +		res_counter_read_u64(&memcg->res, RES_FAILCNT));
> 
> s/res/memsw/ ?
> 
> and, I don't like the name of "Memory cgroup RSS" and "Memory cgroup swap".
> it seems misleading. memcg->res doesn't only count count rss, but also cache.
> memcg->memsw doesn't only count swap, but also memory.
> 
> otherthing, I think it is good patch for me :)
>

Good point, I fixed the res/memsw problem and sent out a patch. About
the RSS thing, not sure what to call it, just calling it memory can be
confusing, may be I should call it

Memory and Memory+swap

Thanks for the review 

-- 
	Balbir

--
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: [-mm patch] Show memcg information during OOM

[Balbir Singh]

* KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> [2009-02-02 21:59:34]:

> Hi
> 
> > +void mem_cgroup_print_mem_info(struct mem_cgroup *memcg)
> > +{
> > +	printk(KERN_WARNING "Memory cgroups's name %s\n",
> > +		memcg->css.cgroup->dentry->d_name.name);
> > +	printk(KERN_WARNING "Memory cgroup RSS : usage %llu, limit %llu"
> > +		" failcnt %llu\n", res_counter_read_u64(&memcg->res, RES_USAGE),
> > +		res_counter_read_u64(&memcg->res, RES_LIMIT),
> > +		res_counter_read_u64(&memcg->res, RES_FAILCNT));
> > +	printk(KERN_WARNING "Memory cgroup swap: usage %llu, limit %llu "
> > +		"failcnt %llu\n", res_counter_read_u64(&memcg->res, RES_USAGE),
> > +		res_counter_read_u64(&memcg->res, RES_LIMIT),
> > +		res_counter_read_u64(&memcg->res, RES_FAILCNT));
>

Thanks! How does this look

Description: Add RSS and swap to OOM output from memcg

From: Balbir Singh <balbir@linux.vnet.ibm.com>

This patch displays memcg values like failcnt, usage and limit
when an OOM occurs due to memcg.

Thanks go out to Johannes Weiner <hannes@cmpxchg.org> and
KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> for review.

Signed-off-by: Balbir Singh <balbir@linux.vnet.ibm.com>
---

 include/linux/memcontrol.h |    5 +++++
 mm/memcontrol.c            |   19 +++++++++++++++++++
 mm/oom_kill.c              |    1 +
 3 files changed, 25 insertions(+), 0 deletions(-)


diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 326f45c..2ce1737 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -104,6 +104,7 @@ struct zone_reclaim_stat *mem_cgroup_get_reclaim_stat(struct mem_cgroup *memcg,
 						      struct zone *zone);
 struct zone_reclaim_stat*
 mem_cgroup_get_reclaim_stat_from_page(struct page *page);
+extern void mem_cgroup_print_mem_info(struct mem_cgroup *memcg);
 
 #ifdef CONFIG_CGROUP_MEM_RES_CTLR_SWAP
 extern int do_swap_account;
@@ -270,6 +271,10 @@ mem_cgroup_get_reclaim_stat_from_page(struct page *page)
 	return NULL;
 }
 
+void mem_cgroup_print_mem_info(struct mem_cgroup *memcg)
+{
+}
+
 #endif /* CONFIG_CGROUP_MEM_CONT */
 
 #endif /* _LINUX_MEMCONTROL_H */
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 8e4be9c..954b0d5 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -813,6 +813,25 @@ bool mem_cgroup_oom_called(struct task_struct *task)
 	rcu_read_unlock();
 	return ret;
 }
+
+void mem_cgroup_print_mem_info(struct mem_cgroup *memcg)
+{
+	if (!memcg)
+		return;
+
+	printk(KERN_WARNING "Memory cgroups's name %s\n",
+		memcg->css.cgroup->dentry->d_name.name);
+	printk(KERN_WARNING "Cgroup memory: usage %llu, limit %llu"
+		" failcnt %llu\n", res_counter_read_u64(&memcg->res, RES_USAGE),
+		res_counter_read_u64(&memcg->res, RES_LIMIT),
+		res_counter_read_u64(&memcg->res, RES_FAILCNT));
+	printk(KERN_WARNING "Cgroup memory+swap: usage %llu, limit %llu "
+		"failcnt %llu\n",
+		res_counter_read_u64(&memcg->memsw, RES_USAGE),
+		res_counter_read_u64(&memcg->memsw, RES_LIMIT),
+		res_counter_read_u64(&memcg->memsw, RES_FAILCNT));
+}
+
 /*
  * Unlike exported interface, "oom" parameter is added. if oom==true,
  * oom-killer can be invoked.
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index d3b9bac..b8e53ae 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -392,6 +392,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
 			current->comm, gfp_mask, order, current->oomkilladj);
 		task_lock(current);
 		cpuset_print_task_mems_allowed(current);
+		mem_cgroup_print_mem_info(mem);
 		task_unlock(current);
 		dump_stack();
 		show_mem();

-- 
	Balbir

--
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: [-mm patch] Show memcg information during OOM

[David Rientjes]

On Mon, 2 Feb 2009, Balbir Singh wrote:

> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 8e4be9c..954b0d5 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -813,6 +813,25 @@ bool mem_cgroup_oom_called(struct task_struct *task)
>  	rcu_read_unlock();
>  	return ret;
>  }
> +
> +void mem_cgroup_print_mem_info(struct mem_cgroup *memcg)
> +{
> +	if (!memcg)
> +		return;
> +
> +	printk(KERN_WARNING "Memory cgroups's name %s\n",
> +		memcg->css.cgroup->dentry->d_name.name);
> +	printk(KERN_WARNING "Cgroup memory: usage %llu, limit %llu"
> +		" failcnt %llu\n", res_counter_read_u64(&memcg->res, RES_USAGE),
> +		res_counter_read_u64(&memcg->res, RES_LIMIT),
> +		res_counter_read_u64(&memcg->res, RES_FAILCNT));
> +	printk(KERN_WARNING "Cgroup memory+swap: usage %llu, limit %llu "
> +		"failcnt %llu\n",
> +		res_counter_read_u64(&memcg->memsw, RES_USAGE),
> +		res_counter_read_u64(&memcg->memsw, RES_LIMIT),
> +		res_counter_read_u64(&memcg->memsw, RES_FAILCNT));
> +}
> +
>  /*
>   * Unlike exported interface, "oom" parameter is added. if oom==true,
>   * oom-killer can be invoked.

I think you'd want a less critical log level for these messages such as 
KERN_INFO.

--
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: [-mm patch] Show memcg information during OOM

[Balbir Singh]

* David Rientjes <rientjes@google.com> [2009-02-02 12:37:54]:

> On Mon, 2 Feb 2009, Balbir Singh wrote:
> 
> 
> I think you'd want a less critical log level for these messages such as 
> KERN_INFO.

David, I'd agree, but since we are under printk_ratelimit() and this
is a not-so-common path, does the log level matter much? If it does, I
don't mind using KERN_INFO.

-- 
	Balbir

--
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: [-mm patch] Show memcg information during OOM

[David Rientjes]

On Tue, 3 Feb 2009, Balbir Singh wrote:

> David, I'd agree, but since we are under printk_ratelimit() and this
> is a not-so-common path, does the log level matter much? If it does, I
> don't mind using KERN_INFO.
> 

It matters for parsing dmesg output; the only KERN_WARNING message from 
the oom killer is normally the header.  There's a couple extra ones for 
error conditions (that could certainly be changed to KERN_ERR), but only 
in very rare circumstances.

As defined by include/linux/kernel.h:

	#define KERN_WARNING	"<4>"	/* warning conditions			*/
	...
	#define KERN_INFO	"<6>"	/* informational			*/

The meminfo you are printing falls under the "informational" category, no?

While you're there, it might also be helpful to make another change 
that would also help in parsing the output:

> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 8e4be9c..954b0d5 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -813,6 +813,25 @@ bool mem_cgroup_oom_called(struct task_struct *task)
>  	rcu_read_unlock();
>  	return ret;
>  }
> +
> +void mem_cgroup_print_mem_info(struct mem_cgroup *memcg)
> +{
> +	if (!memcg)
> +		return;
> +
> +	printk(KERN_WARNING "Memory cgroups's name %s\n",
> +		memcg->css.cgroup->dentry->d_name.name);

This should be "cgroup's", but I don't think you want to print this on a 
line by itself since the only system-wide synchronization here is a 
read-lock on tasklist_lock and there could be two separate memcg's that 
are oom.

So it's quite possible, though unlikely, that two seperate oom events 
would have these messages merged together in the ring buffer, which would 
make parsing impossible.

I think you probably want to add the name to each line you print, such as:

> +	printk(KERN_WARNING "Cgroup memory: usage %llu, limit %llu"
> +		" failcnt %llu\n", res_counter_read_u64(&memcg->res, RES_USAGE),
> +		res_counter_read_u64(&memcg->res, RES_LIMIT),
> +		res_counter_read_u64(&memcg->res, RES_FAILCNT));

	const char *name = memcg->css.cgroup->dentry->d_name.name;

	printk(KERN_INFO "Cgroup %s memory: usage %llu, limit %llu"
			" failcount %llu\n", name, ...);

> +	printk(KERN_WARNING "Cgroup memory+swap: usage %llu, limit %llu "
> +		"failcnt %llu\n",
> +		res_counter_read_u64(&memcg->memsw, RES_USAGE),
> +		res_counter_read_u64(&memcg->memsw, RES_LIMIT),
> +		res_counter_read_u64(&memcg->memsw, RES_FAILCNT));

and

	printk(KERN_INFO "Cgroup %s memory+swap: usage %llu, limit %llu "
		"failcnt %llu\n", name, ...);

> +}

--
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: [-mm patch] Show memcg information during OOM

[KAMEZAWA Hiroyuki]

On Mon, 2 Feb 2009 19:38:49 +0530
Balbir Singh <balbir@linux.vnet.ibm.com> wrote:

> * Balbir Singh <balbir@linux.vnet.ibm.com> [2009-02-02 18:22:40]:
> 
> > Hi, All,
> > 
> > I found the following patch useful while debugging the memory
> > controller. It adds additional information if memcg invoked the OOM.
> > 
> > Comments, Suggestions?
> >
> 
> 
> Description: Add RSS and swap to OOM output from memcg
> 
> From: Balbir Singh <balbir@linux.vnet.ibm.com>
> 

> This patch displays memcg values like failcnt, usage and limit
> when an OOM occurs due to memcg.
> 

please use "KB" not bytes in OOM killer information.

And the most important information is dropped..
Even if you show information, the most important information that
"where I am and where we hit limit ?" is not coverred.
Could you consider some way to show full-path ?

  OOM-Killer:
  Task in /memory/xxx/yyy/zzz is killed by
  Limit of /memory/xxxx
  RSS Limit :     Usage:     Failcnt....
  RSS+SWAP Limit: ....



Thanks,
-Kame

--
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: [-mm patch] Show memcg information during OOM

[Li Zefan]

> +void mem_cgroup_print_mem_info(struct mem_cgroup *memcg)
> +{
> +	printk(KERN_WARNING "Memory cgroups's name %s\n",
> +		memcg->css.cgroup->dentry->d_name.name);
> +	printk(KERN_WARNING "Memory cgroup RSS : usage %llu, limit %llu"
> +		" failcnt %llu\n", res_counter_read_u64(&memcg->res, RES_USAGE),

", failcnt %llu\n" ?

> +		res_counter_read_u64(&memcg->res, RES_LIMIT),
> +		res_counter_read_u64(&memcg->res, RES_FAILCNT));
> +	printk(KERN_WARNING "Memory cgroup swap: usage %llu, limit %llu "
> +		"failcnt %llu\n",

", failcnt %llu\n" ?

> +		res_counter_read_u64(&memcg->memsw, RES_USAGE),
> +		res_counter_read_u64(&memcg->memsw, RES_LIMIT),
> +		res_counter_read_u64(&memcg->memsw, RES_FAILCNT));
> +}
> +
>  /*
>   * Unlike exported interface, "oom" parameter is added. if oom==true,
>   * oom-killer can be invoked.
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index d3b9bac..b8e53ae 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -392,6 +392,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
>  			current->comm, gfp_mask, order, current->oomkilladj);
>  		task_lock(current);
>  		cpuset_print_task_mems_allowed(current);
> +		mem_cgroup_print_mem_info(mem);

I think this can be put outside the task lock. The lock is used to call task_cs() safely in
cpuset_print_task_mems_allowed().

>  		task_unlock(current);
>  		dump_stack();
>  		show_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>

Re: [-mm patch] Show memcg information during OOM

[Daisuke Nishimura]

On Mon, 2 Feb 2009 14:45:06 +0100, Johannes Weiner <hannes@cmpxchg.org> wrote:
> On Mon, Feb 02, 2009 at 06:22:40PM +0530, Balbir Singh wrote:
> > Hi, All,
> > 
> > I found the following patch useful while debugging the memory
> > controller. It adds additional information if memcg invoked the OOM.
> > 
> > Comments, Suggestions?
> > 
> > From: Balbir Singh <balbir@linux.vnet.ibm.com>
> > 
> > Description: Add RSS and swap to OOM output from memcg
> > 
> > This patch displays memcg values like failcnt, usage and limit
> > when an OOM occurs due to memcg.
> > 
> > Signed-off-by: Balbir Singh <balbir@linux.vnet.ibm.com>
> > ---
> > 
> >  include/linux/memcontrol.h |    5 +++++
> >  mm/memcontrol.c            |   15 +++++++++++++++
> >  mm/oom_kill.c              |    1 +
> >  3 files changed, 21 insertions(+), 0 deletions(-)
> > 
> > 
> > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> > index 326f45c..2ce1737 100644
> > --- a/include/linux/memcontrol.h
> > +++ b/include/linux/memcontrol.h
> > @@ -104,6 +104,7 @@ struct zone_reclaim_stat *mem_cgroup_get_reclaim_stat(struct mem_cgroup *memcg,
> >  						      struct zone *zone);
> >  struct zone_reclaim_stat*
> >  mem_cgroup_get_reclaim_stat_from_page(struct page *page);
> > +extern void mem_cgroup_print_mem_info(struct mem_cgroup *memcg);
> >  
> >  #ifdef CONFIG_CGROUP_MEM_RES_CTLR_SWAP
> >  extern int do_swap_account;
> > @@ -270,6 +271,10 @@ mem_cgroup_get_reclaim_stat_from_page(struct page *page)
> >  	return NULL;
> >  }
> >  
> > +void mem_cgroup_print_mem_info(struct mem_cgroup *memcg)
> > +{
> > +}
> > +
> >  #endif /* CONFIG_CGROUP_MEM_CONT */
> >  
> >  #endif /* _LINUX_MEMCONTROL_H */
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 8e4be9c..75eae85 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -813,6 +813,21 @@ bool mem_cgroup_oom_called(struct task_struct *task)
> >  	rcu_read_unlock();
> >  	return ret;
> >  }
> > +
> > +void mem_cgroup_print_mem_info(struct mem_cgroup *memcg)
> > +{
> > +	printk(KERN_WARNING "Memory cgroups's name %s\n",
> > +		memcg->css.cgroup->dentry->d_name.name);
> > +	printk(KERN_WARNING "Memory cgroup RSS : usage %llu, limit %llu"
> > +		" failcnt %llu\n", res_counter_read_u64(&memcg->res, RES_USAGE),
> > +		res_counter_read_u64(&memcg->res, RES_LIMIT),
> > +		res_counter_read_u64(&memcg->res, RES_FAILCNT));
> > +	printk(KERN_WARNING "Memory cgroup swap: usage %llu, limit %llu "
> > +		"failcnt %llu\n", res_counter_read_u64(&memcg->res, RES_USAGE),
> > +		res_counter_read_u64(&memcg->res, RES_LIMIT),
> > +		res_counter_read_u64(&memcg->res, RES_FAILCNT));
> > +}
> > +
> >  /*
> >   * Unlike exported interface, "oom" parameter is added. if oom==true,
> >   * oom-killer can be invoked.
> > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > index d3b9bac..b8e53ae 100644
> > --- a/mm/oom_kill.c
> > +++ b/mm/oom_kill.c
> > @@ -392,6 +392,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
> >  			current->comm, gfp_mask, order, current->oomkilladj);
> >  		task_lock(current);
> >  		cpuset_print_task_mems_allowed(current);
> > +		mem_cgroup_print_mem_info(mem);
> 
> mem is only !NULL when we come from mem_cgroup_out_of_memory().  This
> crashes otherwise in mem_cgroup_print_mem_info(), no?
> 
I think you're right.

IMHO, "mem_cgroup_print_mem_info(current)" would be better here,
and call mem_cgroup_from_task at mem_cgroup_print_mem_info.


Thanks,
Daisuke Nishimura.

--
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: [-mm patch] Show memcg information during OOM

[Balbir Singh]

* KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> [2009-02-03 10:21:57]:

> On Mon, 2 Feb 2009 19:38:49 +0530
> Balbir Singh <balbir@linux.vnet.ibm.com> wrote:
> 
> > * Balbir Singh <balbir@linux.vnet.ibm.com> [2009-02-02 18:22:40]:
> > 
> > > Hi, All,
> > > 
> > > I found the following patch useful while debugging the memory
> > > controller. It adds additional information if memcg invoked the OOM.
> > > 
> > > Comments, Suggestions?
> > >
> > 
> > 
> > Description: Add RSS and swap to OOM output from memcg
> > 
> > From: Balbir Singh <balbir@linux.vnet.ibm.com>
> > 
> 
> > This patch displays memcg values like failcnt, usage and limit
> > when an OOM occurs due to memcg.
> > 
> 
> please use "KB" not bytes in OOM killer information.
> 
> And the most important information is dropped..
> Even if you show information, the most important information that
> "where I am and where we hit limit ?" is not coverred.
> Could you consider some way to show full-path ?
> 
>   OOM-Killer:
>   Task in /memory/xxx/yyy/zzz is killed by
>   Limit of /memory/xxxx
>   RSS Limit :     Usage:     Failcnt....
>   RSS+SWAP Limit: ....
>

Sounds good to me, let me add this information. 

-- 
	Balbir

--
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: [-mm patch] Show memcg information during OOM

[Balbir Singh]

* David Rientjes <rientjes@google.com> [2009-02-02 13:05:02]:

> On Tue, 3 Feb 2009, Balbir Singh wrote:
> 
> > David, I'd agree, but since we are under printk_ratelimit() and this
> > is a not-so-common path, does the log level matter much? If it does, I
> > don't mind using KERN_INFO.
> > 
> 
> It matters for parsing dmesg output; the only KERN_WARNING message from 
> the oom killer is normally the header.  There's a couple extra ones for 
> error conditions (that could certainly be changed to KERN_ERR), but only 
> in very rare circumstances.
> 
> As defined by include/linux/kernel.h:
> 
> 	#define KERN_WARNING	"<4>"	/* warning conditions			*/
> 	...
> 	#define KERN_INFO	"<6>"	/* informational			*/
> 
> The meminfo you are printing falls under the "informational" category, no?
> 
> While you're there, it might also be helpful to make another change 
> that would also help in parsing the output:
> 
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 8e4be9c..954b0d5 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -813,6 +813,25 @@ bool mem_cgroup_oom_called(struct task_struct *task)
> >  	rcu_read_unlock();
> >  	return ret;
> >  }
> > +
> > +void mem_cgroup_print_mem_info(struct mem_cgroup *memcg)
> > +{
> > +	if (!memcg)
> > +		return;
> > +
> > +	printk(KERN_WARNING "Memory cgroups's name %s\n",
> > +		memcg->css.cgroup->dentry->d_name.name);
> 
> This should be "cgroup's", but I don't think you want to print this on a 
> line by itself since the only system-wide synchronization here is a 
> read-lock on tasklist_lock and there could be two separate memcg's that 
> are oom.
> 
> So it's quite possible, though unlikely, that two seperate oom events 
> would have these messages merged together in the ring buffer, which would 
> make parsing impossible.
> 
> I think you probably want to add the name to each line you print, such as:
> 
> > +	printk(KERN_WARNING "Cgroup memory: usage %llu, limit %llu"
> > +		" failcnt %llu\n", res_counter_read_u64(&memcg->res, RES_USAGE),
> > +		res_counter_read_u64(&memcg->res, RES_LIMIT),
> > +		res_counter_read_u64(&memcg->res, RES_FAILCNT));
> 
> 	const char *name = memcg->css.cgroup->dentry->d_name.name;
> 
> 	printk(KERN_INFO "Cgroup %s memory: usage %llu, limit %llu"
> 			" failcount %llu\n", name, ...);
> 
> > +	printk(KERN_WARNING "Cgroup memory+swap: usage %llu, limit %llu "
> > +		"failcnt %llu\n",
> > +		res_counter_read_u64(&memcg->memsw, RES_USAGE),
> > +		res_counter_read_u64(&memcg->memsw, RES_LIMIT),
> > +		res_counter_read_u64(&memcg->memsw, RES_FAILCNT));
> 
> and
> 
> 	printk(KERN_INFO "Cgroup %s memory+swap: usage %llu, limit %llu "
> 		"failcnt %llu\n", name, ...);
> 
> > +}
>

Thanks for the review, I'll incorporate as much of it as possible in
the next version. 

-- 
	Balbir

--
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: [-mm patch] Show memcg information during OOM

[Balbir Singh]

* Li Zefan <lizf@cn.fujitsu.com> [2009-02-03 09:29:09]:

> > +void mem_cgroup_print_mem_info(struct mem_cgroup *memcg)
> > +{
> > +	printk(KERN_WARNING "Memory cgroups's name %s\n",
> > +		memcg->css.cgroup->dentry->d_name.name);
> > +	printk(KERN_WARNING "Memory cgroup RSS : usage %llu, limit %llu"
> > +		" failcnt %llu\n", res_counter_read_u64(&memcg->res, RES_USAGE),
> 
> ", failcnt %llu\n" ?
> 
> > +		res_counter_read_u64(&memcg->res, RES_LIMIT),
> > +		res_counter_read_u64(&memcg->res, RES_FAILCNT));
> > +	printk(KERN_WARNING "Memory cgroup swap: usage %llu, limit %llu "
> > +		"failcnt %llu\n",
> 
> ", failcnt %llu\n" ?
> 
> > +		res_counter_read_u64(&memcg->memsw, RES_USAGE),
> > +		res_counter_read_u64(&memcg->memsw, RES_LIMIT),
> > +		res_counter_read_u64(&memcg->memsw, RES_FAILCNT));
> > +}
> > +
> >  /*
> >   * Unlike exported interface, "oom" parameter is added. if oom==true,
> >   * oom-killer can be invoked.
> > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > index d3b9bac..b8e53ae 100644
> > --- a/mm/oom_kill.c
> > +++ b/mm/oom_kill.c
> > @@ -392,6 +392,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
> >  			current->comm, gfp_mask, order, current->oomkilladj);
> >  		task_lock(current);
> >  		cpuset_print_task_mems_allowed(current);
> > +		mem_cgroup_print_mem_info(mem);
> 
> I think this can be put outside the task lock. The lock is used to call task_cs() safely in
> cpuset_print_task_mems_allowed().
>

Thanks, I'll work on that in the next version.
 
> >  		task_unlock(current);
> >  		dump_stack();
> >  		show_mem();
> > 
> 

-- 
	Balbir

--
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: [-mm patch] Show memcg information during OOM

[David Rientjes]

On Tue, 3 Feb 2009, Balbir Singh wrote:

> > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > > index d3b9bac..b8e53ae 100644
> > > --- a/mm/oom_kill.c
> > > +++ b/mm/oom_kill.c
> > > @@ -392,6 +392,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
> > >  			current->comm, gfp_mask, order, current->oomkilladj);
> > >  		task_lock(current);
> > >  		cpuset_print_task_mems_allowed(current);
> > > +		mem_cgroup_print_mem_info(mem);
> > 
> > I think this can be put outside the task lock. The lock is used to call task_cs() safely in
> > cpuset_print_task_mems_allowed().
> >
> 
> Thanks, I'll work on that in the next version.
>  

I was also wondering about this and assumed that it was necessary to 
prevent the cgroup from disappearing during the oom.  If task_lock() isn't 
held, is the memcg->css.cgroup->dentry->d_name.name dereference always 
safe without rcu?

--
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: [-mm patch] Show memcg information during OOM

[Balbir Singh]

* David Rientjes <rientjes@google.com> [2009-02-02 20:47:05]:

> On Tue, 3 Feb 2009, Balbir Singh wrote:
> 
> > > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > > > index d3b9bac..b8e53ae 100644
> > > > --- a/mm/oom_kill.c
> > > > +++ b/mm/oom_kill.c
> > > > @@ -392,6 +392,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
> > > >  			current->comm, gfp_mask, order, current->oomkilladj);
> > > >  		task_lock(current);
> > > >  		cpuset_print_task_mems_allowed(current);
> > > > +		mem_cgroup_print_mem_info(mem);
> > > 
> > > I think this can be put outside the task lock. The lock is used to call task_cs() safely in
> > > cpuset_print_task_mems_allowed().
> > >
> > 
> > Thanks, I'll work on that in the next version.
> >  
> 
> I was also wondering about this and assumed that it was necessary to 
> prevent the cgroup from disappearing during the oom.  If task_lock() isn't 
> held, is the memcg->css.cgroup->dentry->d_name.name dereference always 
> safe without rcu?
>

oom_kill_process is called with tasklist_lock held (read-mode). That
should suffice, no? The memcg cannot go away since it has other groups
or tasks associated with it. 

-- 
	Balbir

--
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: [-mm patch] Show memcg information during OOM

[Li Zefan]

David Rientjes wrote:
> On Tue, 3 Feb 2009, Balbir Singh wrote:
> 
>>>> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
>>>> index d3b9bac..b8e53ae 100644
>>>> --- a/mm/oom_kill.c
>>>> +++ b/mm/oom_kill.c
>>>> @@ -392,6 +392,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
>>>>  			current->comm, gfp_mask, order, current->oomkilladj);
>>>>  		task_lock(current);
>>>>  		cpuset_print_task_mems_allowed(current);
>>>> +		mem_cgroup_print_mem_info(mem);
>>> I think this can be put outside the task lock. The lock is used to call task_cs() safely in
>>> cpuset_print_task_mems_allowed().
>>>
>> Thanks, I'll work on that in the next version.
>>  
> 
> I was also wondering about this and assumed that it was necessary to 
> prevent the cgroup from disappearing during the oom.  If task_lock() isn't 
> held, is the memcg->css.cgroup->dentry->d_name.name dereference always 
> safe without rcu?
> 

The cgroup won't disappear, since mem_cgroup_out_of_memory() is called with memcg's css refcnt
increased. :)


--
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: [-mm patch] Show memcg information during OOM

[David Rientjes]

On Tue, 3 Feb 2009, Balbir Singh wrote:

> > > > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > > > > index d3b9bac..b8e53ae 100644
> > > > > --- a/mm/oom_kill.c
> > > > > +++ b/mm/oom_kill.c
> > > > > @@ -392,6 +392,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
> > > > >  			current->comm, gfp_mask, order, current->oomkilladj);
> > > > >  		task_lock(current);
> > > > >  		cpuset_print_task_mems_allowed(current);
> > > > > +		mem_cgroup_print_mem_info(mem);
> > > > 
> > > > I think this can be put outside the task lock. The lock is used to call task_cs() safely in
> > > > cpuset_print_task_mems_allowed().
> > > >
> > > 
> > > Thanks, I'll work on that in the next version.
> > >  
> > 
> > I was also wondering about this and assumed that it was necessary to 
> > prevent the cgroup from disappearing during the oom.  If task_lock() isn't 
> > held, is the memcg->css.cgroup->dentry->d_name.name dereference always 
> > safe without rcu?
> >
> 
> oom_kill_process is called with tasklist_lock held (read-mode). That
> should suffice, no? The memcg cannot go away since it has other groups
> or tasks associated with it. 
> 

I don't see how this prevents a task from being reattached to a different 
cgroup and then a rmdir on memcg->css.cgroup would destroy the dentry 
without cgroup_mutex or dereferencing via rcu.

--
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: [-mm patch] Show memcg information during OOM

[Balbir Singh]

* David Rientjes <rientjes@google.com> [2009-02-02 21:25:52]:

> On Tue, 3 Feb 2009, Balbir Singh wrote:
> 
> > > > > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > > > > > index d3b9bac..b8e53ae 100644
> > > > > > --- a/mm/oom_kill.c
> > > > > > +++ b/mm/oom_kill.c
> > > > > > @@ -392,6 +392,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
> > > > > >  			current->comm, gfp_mask, order, current->oomkilladj);
> > > > > >  		task_lock(current);
> > > > > >  		cpuset_print_task_mems_allowed(current);
> > > > > > +		mem_cgroup_print_mem_info(mem);
> > > > > 
> > > > > I think this can be put outside the task lock. The lock is used to call task_cs() safely in
> > > > > cpuset_print_task_mems_allowed().
> > > > >
> > > > 
> > > > Thanks, I'll work on that in the next version.
> > > >  
> > > 
> > > I was also wondering about this and assumed that it was necessary to 
> > > prevent the cgroup from disappearing during the oom.  If task_lock() isn't 
> > > held, is the memcg->css.cgroup->dentry->d_name.name dereference always 
> > > safe without rcu?
> > >
> > 
> > oom_kill_process is called with tasklist_lock held (read-mode). That
> > should suffice, no? The memcg cannot go away since it has other groups
> > or tasks associated with it. 
> > 
> 
> I don't see how this prevents a task from being reattached to a different 
> cgroup and then a rmdir on memcg->css.cgroup would destroy the dentry 
> without cgroup_mutex or dereferencing via rcu.

That scenario is not possible today from the memory controller
perspective.

We hold memcg_tasklist during task movement and during OOM, task
migration is held till OOM completes.

-- 
	Balbir

--
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: [-mm patch] Show memcg information during OOM

[Balbir Singh]

* Li Zefan <lizf@cn.fujitsu.com> [2009-02-03 13:24:34]:

> David Rientjes wrote:
> > On Tue, 3 Feb 2009, Balbir Singh wrote:
> > 
> >>>> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> >>>> index d3b9bac..b8e53ae 100644
> >>>> --- a/mm/oom_kill.c
> >>>> +++ b/mm/oom_kill.c
> >>>> @@ -392,6 +392,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
> >>>>  			current->comm, gfp_mask, order, current->oomkilladj);
> >>>>  		task_lock(current);
> >>>>  		cpuset_print_task_mems_allowed(current);
> >>>> +		mem_cgroup_print_mem_info(mem);
> >>> I think this can be put outside the task lock. The lock is used to call task_cs() safely in
> >>> cpuset_print_task_mems_allowed().
> >>>
> >> Thanks, I'll work on that in the next version.
> >>  
> > 
> > I was also wondering about this and assumed that it was necessary to 
> > prevent the cgroup from disappearing during the oom.  If task_lock() isn't 
> > held, is the memcg->css.cgroup->dentry->d_name.name dereference always 
> > safe without rcu?
> > 
> 
> The cgroup won't disappear, since mem_cgroup_out_of_memory() is called with memcg's css refcnt
> increased. :)
>

And this as well, yes!
 

-- 
	Balbir

--
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: [-mm patch] Show memcg information during OOM

[Balbir Singh]

* David Rientjes <rientjes@google.com> [2009-02-02 13:05:02]:

> On Tue, 3 Feb 2009, Balbir Singh wrote:
> 
> > David, I'd agree, but since we are under printk_ratelimit() and this
> > is a not-so-common path, does the log level matter much? If it does, I
> > don't mind using KERN_INFO.
> > 
> 
> It matters for parsing dmesg output; the only KERN_WARNING message from 
> the oom killer is normally the header.  There's a couple extra ones for 
> error conditions (that could certainly be changed to KERN_ERR), but only 
> in very rare circumstances.
> 
> As defined by include/linux/kernel.h:
> 
> 	#define KERN_WARNING	"<4>"	/* warning conditions			*/
> 	...
> 	#define KERN_INFO	"<6>"	/* informational			*/
> 
> The meminfo you are printing falls under the "informational" category, no?
> 
> While you're there, it might also be helpful to make another change 
> that would also help in parsing the output:
> 
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 8e4be9c..954b0d5 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -813,6 +813,25 @@ bool mem_cgroup_oom_called(struct task_struct *task)
> >  	rcu_read_unlock();
> >  	return ret;
> >  }
> > +
> > +void mem_cgroup_print_mem_info(struct mem_cgroup *memcg)
> > +{
> > +	if (!memcg)
> > +		return;
> > +
> > +	printk(KERN_WARNING "Memory cgroups's name %s\n",
> > +		memcg->css.cgroup->dentry->d_name.name);
> 
> This should be "cgroup's", but I don't think you want to print this on a 
> line by itself since the only system-wide synchronization here is a 
> read-lock on tasklist_lock and there could be two separate memcg's that 
> are oom.
> 
> So it's quite possible, though unlikely, that two seperate oom events 
> would have these messages merged together in the ring buffer, which would 
> make parsing impossible.
> 
> I think you probably want to add the name to each line you print, such as:

See below

> 
> > +	printk(KERN_WARNING "Cgroup memory: usage %llu, limit %llu"
> > +		" failcnt %llu\n", res_counter_read_u64(&memcg->res, RES_USAGE),
> > +		res_counter_read_u64(&memcg->res, RES_LIMIT),
> > +		res_counter_read_u64(&memcg->res, RES_FAILCNT));
> 
> 	const char *name = memcg->css.cgroup->dentry->d_name.name;
> 
> 	printk(KERN_INFO "Cgroup %s memory: usage %llu, limit %llu"
> 			" failcount %llu\n", name, ...);
> 
> > +	printk(KERN_WARNING "Cgroup memory+swap: usage %llu, limit %llu "
> > +		"failcnt %llu\n",
> > +		res_counter_read_u64(&memcg->memsw, RES_USAGE),
> > +		res_counter_read_u64(&memcg->memsw, RES_LIMIT),
> > +		res_counter_read_u64(&memcg->memsw, RES_FAILCNT));
> 
> and
> 
> 	printk(KERN_INFO "Cgroup %s memory+swap: usage %llu, limit %llu "
> 		"failcnt %llu\n", name, ...);
> 
> > +}
>

FYI, we have OOM serialization via the memcg_tasklist mutex in the
memory controller.
 

-- 
	Balbir

--
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: [-mm patch] Show memcg information during OOM

[David Rientjes]

On Tue, 3 Feb 2009, Balbir Singh wrote:

> > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > > index 8e4be9c..954b0d5 100644
> > > --- a/mm/memcontrol.c
> > > +++ b/mm/memcontrol.c
> > > @@ -813,6 +813,25 @@ bool mem_cgroup_oom_called(struct task_struct *task)
> > >  	rcu_read_unlock();
> > >  	return ret;
> > >  }
> > > +
> > > +void mem_cgroup_print_mem_info(struct mem_cgroup *memcg)
> > > +{
> > > +	if (!memcg)
> > > +		return;
> > > +
> > > +	printk(KERN_WARNING "Memory cgroups's name %s\n",
> > > +		memcg->css.cgroup->dentry->d_name.name);
> > 
> > This should be "cgroup's", but I don't think you want to print this on a 
> > line by itself since the only system-wide synchronization here is a 
> > read-lock on tasklist_lock and there could be two separate memcg's that 
> > are oom.
> > 
> > So it's quite possible, though unlikely, that two seperate oom events 
> > would have these messages merged together in the ring buffer, which would 
> > make parsing impossible.
> > 
> > I think you probably want to add the name to each line you print, such as:
> 
> See below
> 
> > 
> > > +	printk(KERN_WARNING "Cgroup memory: usage %llu, limit %llu"
> > > +		" failcnt %llu\n", res_counter_read_u64(&memcg->res, RES_USAGE),
> > > +		res_counter_read_u64(&memcg->res, RES_LIMIT),
> > > +		res_counter_read_u64(&memcg->res, RES_FAILCNT));
> > 
> > 	const char *name = memcg->css.cgroup->dentry->d_name.name;
> > 
> > 	printk(KERN_INFO "Cgroup %s memory: usage %llu, limit %llu"
> > 			" failcount %llu\n", name, ...);
> > 
> > > +	printk(KERN_WARNING "Cgroup memory+swap: usage %llu, limit %llu "
> > > +		"failcnt %llu\n",
> > > +		res_counter_read_u64(&memcg->memsw, RES_USAGE),
> > > +		res_counter_read_u64(&memcg->memsw, RES_LIMIT),
> > > +		res_counter_read_u64(&memcg->memsw, RES_FAILCNT));
> > 
> > and
> > 
> > 	printk(KERN_INFO "Cgroup %s memory+swap: usage %llu, limit %llu "
> > 		"failcnt %llu\n", name, ...);
> > 
> > > +}
> >
> 
> FYI, we have OOM serialization via the memcg_tasklist mutex in the
> memory controller.
>  

I think it would be easier to parse in userspace if you provided the 
cgroup name on the same lines as the usage, limit, and failcnt values 
instead of storing that from the header line, but do whatever you want.

--
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: [-mm patch] Show memcg information during OOM

[Daisuke Nishimura]

> > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > > index d3b9bac..b8e53ae 100644
> > > --- a/mm/oom_kill.c
> > > +++ b/mm/oom_kill.c
> > > @@ -392,6 +392,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
> > >  			current->comm, gfp_mask, order, current->oomkilladj);
> > >  		task_lock(current);
> > >  		cpuset_print_task_mems_allowed(current);
> > > +		mem_cgroup_print_mem_info(mem);
> > 
> > mem is only !NULL when we come from mem_cgroup_out_of_memory().  This
> > crashes otherwise in mem_cgroup_print_mem_info(), no?
> > 
> I think you're right.
> 
> IMHO, "mem_cgroup_print_mem_info(current)" would be better here,
> and call mem_cgroup_from_task at mem_cgroup_print_mem_info.
> 
Reading other messages on this thread, mem_cgroup_print_mem_info
should be called only when oom_kill_process is called from mem_cgroup_out_of_memory,
so checking "if (!mem)" would be enough.


Thanks,
Daisuke Nishimura.

--
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: [-mm patch] Show memcg information during OOM

[David Rientjes]

On Tue, 3 Feb 2009, Daisuke Nishimura wrote:

> > > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > > > index d3b9bac..b8e53ae 100644
> > > > --- a/mm/oom_kill.c
> > > > +++ b/mm/oom_kill.c
> > > > @@ -392,6 +392,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
> > > >  			current->comm, gfp_mask, order, current->oomkilladj);
> > > >  		task_lock(current);
> > > >  		cpuset_print_task_mems_allowed(current);
> > > > +		mem_cgroup_print_mem_info(mem);
> > > 
> > > mem is only !NULL when we come from mem_cgroup_out_of_memory().  This
> > > crashes otherwise in mem_cgroup_print_mem_info(), no?
> > > 
> > I think you're right.
> > 
> > IMHO, "mem_cgroup_print_mem_info(current)" would be better here,
> > and call mem_cgroup_from_task at mem_cgroup_print_mem_info.
> > 
> Reading other messages on this thread, mem_cgroup_print_mem_info
> should be called only when oom_kill_process is called from mem_cgroup_out_of_memory,
> so checking "if (!mem)" would be enough.
> 

You're right, but it's understandable why there would be confusion since 
it's very poorly documented.

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