[patch] oom: print triggering task's cpuset and mems allowed

[patch] oom: print triggering task's cpuset and mems allowed

[David Rientjes]

When cpusets are enabled, it's necessary to print the triggering task's
set of allowable nodes so the subsequently printed meminfo can be
interpreted correctly.

We also print the task's cpuset name for informational purposes.

Cc: Paul Menage <menage@google.com>
Signed-off-by: David Rientjes <rientjes@google.com>
---
 include/linux/cpuset.h |    7 +++++++
 kernel/cpuset.c        |   15 +++++++++++++++
 mm/oom_kill.c          |   22 ++++++++++++++++++++++
 3 files changed, 44 insertions(+), 0 deletions(-)

diff --git a/include/linux/cpuset.h b/include/linux/cpuset.h
--- a/include/linux/cpuset.h
+++ b/include/linux/cpuset.h
@@ -79,6 +79,7 @@ extern void cpuset_track_online_nodes(void);
 extern int current_cpuset_is_being_rebound(void);
 
 extern void rebuild_sched_domains(void);
+extern int cpuset_get_name(struct task_struct *p, size_t len, char *buffer);
 
 #else /* !CONFIG_CPUSETS */
 
@@ -163,6 +164,12 @@ static inline void rebuild_sched_domains(void)
 	partition_sched_domains(1, NULL, NULL);
 }
 
+static inline int cpuset_get_name(struct task_struct *p, size_t len,
+				  char *buffer)
+{
+	return 0;
+}
+
 #endif /* !CONFIG_CPUSETS */
 
 #endif /* _LINUX_CPUSET_H */
diff --git a/kernel/cpuset.c b/kernel/cpuset.c
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -2339,6 +2339,21 @@ int cpuset_mems_allowed_intersects(const struct task_struct *tsk1,
 	return nodes_intersects(tsk1->mems_allowed, tsk2->mems_allowed);
 }
 
+/**
+ * cpuset_get_name - grabs the name of @task's cpuset
+ * @task: pointer to task_struct of some task.
+ *
+ * Description: Place @task's cpuset name in @buffer and return the
+ * number of characters copied.  Must hold task_lock(task).
+ */
+int cpuset_get_name(struct task_struct *task, size_t len, char *buffer)
+{
+        struct dentry *dentry;
+        dentry = task_cs(task)->css.cgroup->dentry;
+        return snprintf(buffer, len,
+                        dentry ? (const char *)dentry->d_name.name : "/");
+}
+
 /*
  * Collection of memory_pressure is suppressed unless
  * this flag is enabled by writing "1" to the special
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -305,6 +305,27 @@ static void dump_tasks(const struct mem_cgroup *mem)
 	} while_each_thread(g, p);
 }
 
+#ifdef CONFIG_CPUSETS
+#define BUFFER_LEN	(256)
+
+static void print_task_cpuset(struct task_struct *p)
+{
+	char buffer[BUFFER_LEN];
+
+	if (cpuset_get_name(current, BUFFER_LEN, buffer) > 0) {
+		char nodelist[BUFFER_LEN];
+
+		nodelist_scnprintf(nodelist, BUFFER_LEN, current->mems_allowed);
+		printk(KERN_ERR "%s cpuset=%s mems_allowed=%s\n",
+		       current->comm, buffer, nodelist);
+	}
+}
+#else
+static inline void print_task_cpuset(struct task_struct *p)
+{
+}
+#endif
+
 /*
  * Send SIGKILL to the selected  process irrespective of  CAP_SYS_RAW_IO
  * flag though it's unlikely that  we select a process with CAP_SYS_RAW_IO
@@ -391,6 +412,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
 		printk(KERN_WARNING "%s invoked oom-killer: "
 			"gfp_mask=0x%x, order=%d, oomkilladj=%d\n",
 			current->comm, gfp_mask, order, current->oomkilladj);
+		print_task_cpuset(current);
 		dump_stack();
 		show_mem();
 		if (sysctl_oom_dump_tasks)

[patch] oom: print triggering task's cpuset and mems allowed

[David Rientjes]

When cpusets are enabled, it's necessary to print the triggering task's
set of allowable nodes so the subsequently printed meminfo can be
interpreted correctly.

We also print the task's cpuset name for informational purposes.

Cc: Paul Menage <menage@google.com>
Signed-off-by: David Rientjes <rientjes@google.com>
---
 Updated version of the same patch for -mm inclusion.

 include/linux/cpuset.h |    7 +++++++
 kernel/cpuset.c        |   15 +++++++++++++++
 mm/oom_kill.c          |   22 ++++++++++++++++++++++
 3 files changed, 44 insertions(+), 0 deletions(-)

diff --git a/include/linux/cpuset.h b/include/linux/cpuset.h
--- a/include/linux/cpuset.h
+++ b/include/linux/cpuset.h
@@ -79,6 +79,7 @@ extern void cpuset_track_online_nodes(void);
 extern int current_cpuset_is_being_rebound(void);
 
 extern void rebuild_sched_domains(void);
+extern int cpuset_get_name(struct task_struct *p, size_t len, char *buffer);
 
 #else /* !CONFIG_CPUSETS */
 
@@ -163,6 +164,12 @@ static inline void rebuild_sched_domains(void)
 	partition_sched_domains(1, NULL, NULL);
 }
 
+static inline int cpuset_get_name(struct task_struct *p, size_t len,
+				  char *buffer)
+{
+	return 0;
+}
+
 #endif /* !CONFIG_CPUSETS */
 
 #endif /* _LINUX_CPUSET_H */
diff --git a/kernel/cpuset.c b/kernel/cpuset.c
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -2339,6 +2339,21 @@ int cpuset_mems_allowed_intersects(const struct task_struct *tsk1,
 	return nodes_intersects(tsk1->mems_allowed, tsk2->mems_allowed);
 }
 
+/**
+ * cpuset_get_name - grabs the name of @task's cpuset
+ * @task: pointer to task_struct of some task.
+ *
+ * Description: Place @task's cpuset name in @buffer and return the
+ * number of characters copied.  Must hold task_lock(task).
+ */
+int cpuset_get_name(struct task_struct *task, size_t len, char *buffer)
+{
+        struct dentry *dentry;
+        dentry = task_cs(task)->css.cgroup->dentry;
+        return snprintf(buffer, len,
+                        dentry ? (const char *)dentry->d_name.name : "/");
+}
+
 /*
  * Collection of memory_pressure is suppressed unless
  * this flag is enabled by writing "1" to the special
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -305,6 +305,27 @@ static void dump_tasks(const struct mem_cgroup *mem)
 	} while_each_thread(g, p);
 }
 
+#ifdef CONFIG_CPUSETS
+#define BUFFER_LEN	(256)
+
+static void print_task_cpuset(struct task_struct *p)
+{
+	char buffer[BUFFER_LEN];
+
+	if (cpuset_get_name(p, BUFFER_LEN, buffer) > 0) {
+		char nodelist[BUFFER_LEN];
+
+		nodelist_scnprintf(nodelist, BUFFER_LEN, p->mems_allowed);
+		printk(KERN_ERR "%s cpuset=%s mems_allowed=%s\n",
+		       p->comm, buffer, nodelist);
+	}
+}
+#else
+static inline void print_task_cpuset(struct task_struct *p)
+{
+}
+#endif
+
 /*
  * Send SIGKILL to the selected  process irrespective of  CAP_SYS_RAW_IO
  * flag though it's unlikely that  we select a process with CAP_SYS_RAW_IO
@@ -391,6 +412,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
 		printk(KERN_WARNING "%s invoked oom-killer: "
 			"gfp_mask=0x%x, order=%d, oomkilladj=%d\n",
 			current->comm, gfp_mask, order, current->oomkilladj);
+		print_task_cpuset(current);
 		dump_stack();
 		show_mem();
 		if (sysctl_oom_dump_tasks)

Re: [patch] oom: print triggering task's cpuset and mems allowed

[Andrew Morton]

On Fri, 24 Oct 2008 17:15:32 -0700 (PDT)
David Rientjes <rientjes@google.com> wrote:

> +#ifdef CONFIG_CPUSETS
> +#define BUFFER_LEN	(256)
> +
> +static void print_task_cpuset(struct task_struct *p)
> +{
> +	char buffer[BUFFER_LEN];
> +
> +	if (cpuset_get_name(p, BUFFER_LEN, buffer) > 0) {
> +		char nodelist[BUFFER_LEN];
> +
> +		nodelist_scnprintf(nodelist, BUFFER_LEN, p->mems_allowed);
> +		printk(KERN_ERR "%s cpuset=%s mems_allowed=%s\n",
> +		       p->comm, buffer, nodelist);
> +	}
> +}
> +#else
> +static inline void print_task_cpuset(struct task_struct *p)
> +{
> +}
> +#endif
> +
>  /*
>   * Send SIGKILL to the selected  process irrespective of  CAP_SYS_RAW_IO
>   * flag though it's unlikely that  we select a process with CAP_SYS_RAW_IO
> @@ -391,6 +412,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
>  		printk(KERN_WARNING "%s invoked oom-killer: "
>  			"gfp_mask=0x%x, order=%d, oomkilladj=%d\n",
>  			current->comm, gfp_mask, order, current->oomkilladj);
> +		print_task_cpuset(current);
>  		dump_stack();
>  		show_mem();
>  		if (sysctl_oom_dump_tasks)

We can call the oom-killer at very very deep nesting levels, and adding
another 512 bytes of stack consuption to that call path is really
risky.  Perhaps use statically allocated buffers protected by a local
spinlock?

Also, 256 bytes might be overkill for storing the cpuset's name?

Also, it's Just Wrong that this code has to hardwire private knowledge
of the max possible length of a cpuset name and of the
nodelist_scnprintf() return string.  These things should be controlled
by a single #define in a shared header file.

Re: [patch] oom: print triggering task's cpuset and mems allowed

[David Rientjes]

On Mon, 27 Oct 2008, Andrew Morton wrote:

> We can call the oom-killer at very very deep nesting levels, and adding
> another 512 bytes of stack consuption to that call path is really
> risky.  Perhaps use statically allocated buffers protected by a local
> spinlock?
> 

That sounds appropriate.  I've also moved all the cpuset-specific code 
over to kernel/cpuset.c where it belongs.

> Also, 256 bytes might be overkill for storing the cpuset's name?
> 
> Also, it's Just Wrong that this code has to hardwire private knowledge
> of the max possible length of a cpuset name and of the
> nodelist_scnprintf() return string.  These things should be controlled
> by a single #define in a shared header file.
> 

The max length of a cpuset name is dependant on the dentry, but it's of no 
concern here: we're more interested in only printing a single line with 
the pertinent information and truncate it as necessary.

The same is true of the nodelist, since it's possible for this string to 
be 2000 characters in length on architectures such as ia64 where 
CONFIG_NODES_SHIFT is >= 10.  

We must truncate at some point, so 256 bytes was an arbitrary length.

How about this updated version?



oom: print triggering task's cpuset and mems allowed

When cpusets are enabled, it's necessary to print the triggering task's
set of allowable nodes so the subsequently printed meminfo can be
interpreted correctly.

We also print the task's cpuset name for informational purposes.

Cc: Paul Menage <menage@google.com>
Signed-off-by: David Rientjes <rientjes@google.com>
---
 include/linux/cpuset.h |    6 ++++++
 kernel/cpuset.c        |   34 ++++++++++++++++++++++++++++++++++
 mm/oom_kill.c          |    1 +
 3 files changed, 41 insertions(+), 0 deletions(-)

diff --git a/include/linux/cpuset.h b/include/linux/cpuset.h
--- a/include/linux/cpuset.h
+++ b/include/linux/cpuset.h
@@ -80,6 +80,8 @@ extern int current_cpuset_is_being_rebound(void);
 
 extern void rebuild_sched_domains(void);
 
+extern void cpuset_print_task_mems_allowed(struct task_struct *p);
+
 #else /* !CONFIG_CPUSETS */
 
 static inline int cpuset_init_early(void) { return 0; }
@@ -163,6 +165,10 @@ static inline void rebuild_sched_domains(void)
 	partition_sched_domains(1, NULL, NULL);
 }
 
+static inline void cpuset_print_task_mems_allowed(struct task_struct *p)
+{
+}
+
 #endif /* !CONFIG_CPUSETS */
 
 #endif /* _LINUX_CPUSET_H */
diff --git a/kernel/cpuset.c b/kernel/cpuset.c
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -239,6 +239,17 @@ static struct cpuset top_cpuset = {
 static DEFINE_MUTEX(callback_mutex);
 
 /*
+ * cpuset_buffer_lock protects both the cpuset_name and cpuset_nodelist
+ * buffers.  They are statically allocated to prevent using excess stack
+ * when calling cpuset_print_task_mems_allowed().
+ */
+#define CPUSET_NAME_LEN		(128)
+#define	CPUSET_NODELIST_LEN	(256)
+static char cpuset_name[CPUSET_NAME_LEN];
+static char cpuset_nodelist[CPUSET_NODELIST_LEN];
+static DEFINE_SPINLOCK(cpuset_buffer_lock);
+
+/*
  * This is ugly, but preserves the userspace API for existing cpuset
  * users. If someone tries to mount the "cpuset" filesystem, we
  * silently switch it to mount "cgroup" instead
@@ -2339,6 +2350,29 @@ int cpuset_mems_allowed_intersects(const struct task_struct *tsk1,
 	return nodes_intersects(tsk1->mems_allowed, tsk2->mems_allowed);
 }
 
+/**
+ * cpuset_print_task_mems_allowed - prints task's cpuset and mems_allowed
+ * @task: pointer to task_struct of some task.
+ *
+ * Description: Prints @task's name, cpuset name, and cached copy of its
+ * mems_allowed to the kernel log.  Must hold task_lock(task) to allow
+ * dereferencing task_cs(task).
+ */
+void cpuset_print_task_mems_allowed(struct task_struct *tsk)
+{
+	struct dentry *dentry;
+
+	dentry = task_cs(tsk)->css.cgroup->dentry;
+	spin_lock(&cpuset_buffer_lock);
+	snprintf(cpuset_name, CPUSET_NAME_LEN,
+		 dentry ? (const char *)dentry->d_name.name : "/");
+	nodelist_scnprintf(cpuset_nodelist, CPUSET_NODELIST_LEN,
+			   tsk->mems_allowed);
+	printk(KERN_INFO "%s cpuset=%s mems_allowed=%s\n",
+	       tsk->comm, cpuset_name, cpuset_nodelist);
+	spin_unlock(&cpuset_buffer_lock);
+}
+
 /*
  * Collection of memory_pressure is suppressed unless
  * this flag is enabled by writing "1" to the special
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -391,6 +391,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
 		printk(KERN_WARNING "%s invoked oom-killer: "
 			"gfp_mask=0x%x, order=%d, oomkilladj=%d\n",
 			current->comm, gfp_mask, order, current->oomkilladj);
+		cpuset_print_task_mems_allowed(current);
 		dump_stack();
 		show_mem();
 		if (sysctl_oom_dump_tasks)

Re: [patch] oom: print triggering task's cpuset and mems allowed

[Andrew Morton]

On Tue, 28 Oct 2008 09:08:15 -0700 (PDT)
David Rientjes <rientjes@google.com> wrote:

> +#define CPUSET_NAME_LEN		(128)
> +#define	CPUSET_NODELIST_LEN	(256)
> +static char cpuset_name[CPUSET_NAME_LEN];
> +static char cpuset_nodelist[CPUSET_NODELIST_LEN];
>
> ...
>
> +	snprintf(cpuset_name, CPUSET_NAME_LEN,
> +		 dentry ? (const char *)dentry->d_name.name : "/");

nit: this requires that the reviewer (and the maintainer) ensure that
CPUSET_NAME_LEN=sizeof(cpuset_name).  This must be done manually and
introduces risk.

Better would be:


static char cpuset_name[128];

...

	snprintf(cpuset_name, sizeof(cpuset_name), ...);

Re: [patch] oom: print triggering task's cpuset and mems allowed

[Li Zefan]

> oom: print triggering task's cpuset and mems allowed
> 
> When cpusets are enabled, it's necessary to print the triggering task's
> set of allowable nodes so the subsequently printed meminfo can be
> interpreted correctly.
> 
> We also print the task's cpuset name for informational purposes.
> 
> Cc: Paul Menage <menage@google.com>
> Signed-off-by: David Rientjes <rientjes@google.com>
> ---
>  include/linux/cpuset.h |    6 ++++++
>  kernel/cpuset.c        |   34 ++++++++++++++++++++++++++++++++++
>  mm/oom_kill.c          |    1 +
>  3 files changed, 41 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/cpuset.h b/include/linux/cpuset.h
> --- a/include/linux/cpuset.h
> +++ b/include/linux/cpuset.h
> @@ -80,6 +80,8 @@ extern int current_cpuset_is_being_rebound(void);
>  
>  extern void rebuild_sched_domains(void);
>  
> +extern void cpuset_print_task_mems_allowed(struct task_struct *p);
> +
>  #else /* !CONFIG_CPUSETS */
>  
>  static inline int cpuset_init_early(void) { return 0; }
> @@ -163,6 +165,10 @@ static inline void rebuild_sched_domains(void)
>  	partition_sched_domains(1, NULL, NULL);
>  }
>  
> +static inline void cpuset_print_task_mems_allowed(struct task_struct *p)
> +{
> +}
> +
>  #endif /* !CONFIG_CPUSETS */
>  
>  #endif /* _LINUX_CPUSET_H */
> diff --git a/kernel/cpuset.c b/kernel/cpuset.c
> --- a/kernel/cpuset.c
> +++ b/kernel/cpuset.c
> @@ -239,6 +239,17 @@ static struct cpuset top_cpuset = {
>  static DEFINE_MUTEX(callback_mutex);
>  
>  /*
> + * cpuset_buffer_lock protects both the cpuset_name and cpuset_nodelist
> + * buffers.  They are statically allocated to prevent using excess stack
> + * when calling cpuset_print_task_mems_allowed().
> + */
> +#define CPUSET_NAME_LEN		(128)
> +#define	CPUSET_NODELIST_LEN	(256)
> +static char cpuset_name[CPUSET_NAME_LEN];
> +static char cpuset_nodelist[CPUSET_NODELIST_LEN];
> +static DEFINE_SPINLOCK(cpuset_buffer_lock);
> +
> +/*
>   * This is ugly, but preserves the userspace API for existing cpuset
>   * users. If someone tries to mount the "cpuset" filesystem, we
>   * silently switch it to mount "cgroup" instead
> @@ -2339,6 +2350,29 @@ int cpuset_mems_allowed_intersects(const struct task_struct *tsk1,
>  	return nodes_intersects(tsk1->mems_allowed, tsk2->mems_allowed);
>  }
>  
> +/**
> + * cpuset_print_task_mems_allowed - prints task's cpuset and mems_allowed
> + * @task: pointer to task_struct of some task.
> + *
> + * Description: Prints @task's name, cpuset name, and cached copy of its
> + * mems_allowed to the kernel log.  Must hold task_lock(task) to allow
> + * dereferencing task_cs(task).
> + */

But none of the callers of this function holds task_lock nor rcu_read_lock,
they hold tasklist_lock. Have you confirmed this is ok? It seems racy that
the task may be moved to another cgroup and the original cgroup gets removed.

> +void cpuset_print_task_mems_allowed(struct task_struct *tsk)
> +{
> +	struct dentry *dentry;
> +
> +	dentry = task_cs(tsk)->css.cgroup->dentry;
> +	spin_lock(&cpuset_buffer_lock);
> +	snprintf(cpuset_name, CPUSET_NAME_LEN,
> +		 dentry ? (const char *)dentry->d_name.name : "/");
> +	nodelist_scnprintf(cpuset_nodelist, CPUSET_NODELIST_LEN,
> +			   tsk->mems_allowed);
> +	printk(KERN_INFO "%s cpuset=%s mems_allowed=%s\n",
> +	       tsk->comm, cpuset_name, cpuset_nodelist);
> +	spin_unlock(&cpuset_buffer_lock);
> +}
> +
>  /*
>   * Collection of memory_pressure is suppressed unless
>   * this flag is enabled by writing "1" to the special
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -391,6 +391,7 @@ static int oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
>  		printk(KERN_WARNING "%s invoked oom-killer: "
>  			"gfp_mask=0x%x, order=%d, oomkilladj=%d\n",
>  			current->comm, gfp_mask, order, current->oomkilladj);
> +		cpuset_print_task_mems_allowed(current);
>  		dump_stack();
>  		show_mem();
>  		if (sysctl_oom_dump_tasks)
> --

Re: [patch] oom: print triggering task's cpuset and mems allowed

[David Rientjes]

On Wed, 19 Nov 2008, Li Zefan wrote:

> But none of the callers of this function holds task_lock nor rcu_read_lock,
> they hold tasklist_lock. Have you confirmed this is ok? It seems racy that
> the task may be moved to another cgroup and the original cgroup gets removed.
> 

It needs task_lock(current).