[PATCH ftrace/core 0/2] ftrace: Introduce the new file "saved_cmdlines_size"

[PATCH ftrace/core 0/2] ftrace: Introduce the new file "saved_cmdlines_size"

[Yoshihiro YUNOMAE]

Hi Steven,

This patch set introduces the new file "saved_cmdlines_size" for increasing
the number of saved cmdlines. Current saved_cmdlines can store just 128 command 
names and PIDs, but process names are often lost like <...> when we read trace
data. If the process exists, we can get the name by using ps command. However,
if the process already has not existed, we cannot get the name.

To solve this issue, we introduce the new file "saved_cmdlines_size" to expand
the max number of saved command line names. This file is very simple.
If we write a number to nr_saved_cmdlines, the number of command name will be
stored. And, if we read the file, we can get current maximum number of command
name. The default number is 128 which is current default number, so this patch
does not change the usage of memory for saved_cmdlines when we boot kernel.

I found a bug for current ftrace, so I fixed the bug before introducing
saved_cmdlines_size file (2nd patch). Note that the 2nd patch depends on
the 1st patch.

Thanks!

Changes in V2:
[2/2]
- Fix a racing problem of savedcmd between saved_cmdlines I/F and
  nr_saved_cmdlines I/F. If one reads saved_cmdlines and writes a value to
  nr_saved_cmdlines at the same time, then the write returns -EBUSY.

  <How to test>
  [terminal 1] Read saved_cmdlines
  # while true; do cat saved_cmdlines > /dev/null; done;

  [terminal 2] Write 1024 to nr_saved_cmdlines
  # while true; do echo 1024 > nr_saved_cmdlines; done;
  -bash: echo: write error: Device or resource busy
  -bash: echo: write error: Device or resource busy
  -bash: echo: write error: Device or resource busy

Changes in V3:
[1/2]
- Introduce this patch
[2/2]
- Change 'nr_saved_cmdlines' to 'saved_cmdlines_size'
- Delete two helper functions(trace_init_savedcmd() and
  trace_creare_and_init_saved_cmd())
- Rebase this patch for current ftrace/core tree
- Remove reader member in saved_cmdlines_buffer structure because the racing
  problem does not occur even if we don't the member in this patch
- Fix several typos

---

Yoshihiro YUNOMAE (2):
      [BUGFIX] ftrace: Avoid panic when allocation of max_buffer is failed
      ftrace: Introduce saved_cmdlines_size file


 kernel/trace/trace.c |  205 ++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 179 insertions(+), 26 deletions(-)

-- 
Yoshihiro YUNOMAE
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: yoshihiro.yunomae.ez@hitachi.com

[PATCH ftrace/core 1/2] [BUGFIX] ftrace: Avoid panic when allocation of max_buffer is failed

[Yoshihiro YUNOMAE]

When allocation of max_buffer is failed, the kernel frees tr->trace_buffer.data
per CPU and return -ENOMEM in allocate_trace_buffers(). However,
tracer_alloc_buffers() calling allocate_trace_buffers() also frees the data
per CPU for -ENOMEM by allocate_trace_buffers(). Therefore, the allocation
failure induces double free.

For the out_free_mask path in tracer_alloc_buffers(),
global_trace.trace_buffer.data and global_trace.max_buffer.data are
not allocated yet, so free_percpu of those are not needed.

Signed-off-by: Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: linux-kernel@vger.kernel.org
---
 kernel/trace/trace.c |    4 ----
 1 file changed, 4 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 626dbfd..135af32 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -6671,10 +6671,6 @@ __init static int tracer_alloc_buffers(void)
 out_free_temp_buffer:
 	ring_buffer_free(temp_buffer);
 out_free_cpumask:
-	free_percpu(global_trace.trace_buffer.data);
-#ifdef CONFIG_TRACER_MAX_TRACE
-	free_percpu(global_trace.max_buffer.data);
-#endif
 	free_cpumask_var(global_trace.tracing_cpumask);
 out_free_buffer_mask:
 	free_cpumask_var(tracing_buffer_mask);

[PATCH ftrace/core 2/2] ftrace: Introduce saved_cmdlines_size file

[Yoshihiro YUNOMAE]

Introduce saved_cmdlines_size file for changing the number of pid-comm list.
saved_cmdlines can store 128 command names using SAVED_CMDLINES now, but
'no-existing processes' names are often lost in saved_cmdlines when we
read trace data. So, by introducing saved_cmdlines_size file, the rule storing
128 command names is changed to the command numbers defined users.

When we write a value to saved_cmdlines_size, the number of the value will
be stored in pid-comm list:

	# echo 1024 > /sys/kernel/debug/tracing/saved_cmdlines_size

Here, 1024 command names are stored. The default number is 128 and the maximum
number is PID_MAX_DEFAULT (=32768 if CONFIG_BASE_SMALL is not set). So, if we
want to avoid to lose command names, we need to set 32768 to
saved_cmdlines_size.

We can read the maximum number of the list:

	# cat /sys/kernel/debug/tracing/saved_cmdlines_size
	128

Changes in V2:
- Fix a racing problem of savedcmd between saved_cmdlines I/F and
  nr_saved_cmdlines I/F. If one reads saved_cmdlines and writes a value to
  nr_saved_cmdlines at the same time, then the write returns -EBUSY.

  <How to test>
  [terminal 1] Read saved_cmdlines
  # while true; do cat saved_cmdlines > /dev/null; done;

  [terminal 2] Write 1024 to nr_saved_cmdlines
  # while true; do echo 1024 > nr_saved_cmdlines; done;
  -bash: echo: write error: Device or resource busy
  -bash: echo: write error: Device or resource busy
  -bash: echo: write error: Device or resource busy

Changes in V3:
- Change 'nr_saved_cmdlines' to 'saved_cmdlines_size'
- Delete two helper functions(trace_init_savedcmd() and
  trace_create_and_init_saved_cmd())
- Rebase this patch for current ftrace/core tree
- Remove reader member in saved_cmdlines_buffer structure because the racing
  problem does not occur even if we don't use the member in this patch
- Fix several typos

Signed-off-by: Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: linux-kernel@vger.kernel.org
---
 kernel/trace/trace.c |  203 ++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 180 insertions(+), 23 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 135af32..473eb68 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -1285,22 +1285,82 @@ void tracing_reset_all_online_cpus(void)
 	}
 }
 
-#define SAVED_CMDLINES 128
+#define SAVED_CMDLINES_DEFAULT 128
 #define NO_CMDLINE_MAP UINT_MAX
-static unsigned map_pid_to_cmdline[PID_MAX_DEFAULT+1];
-static unsigned map_cmdline_to_pid[SAVED_CMDLINES];
-static char saved_cmdlines[SAVED_CMDLINES][TASK_COMM_LEN];
-static int cmdline_idx;
 static arch_spinlock_t trace_cmdline_lock = __ARCH_SPIN_LOCK_UNLOCKED;
+struct saved_cmdlines_buffer {
+	unsigned map_pid_to_cmdline[PID_MAX_DEFAULT+1];
+	unsigned *map_cmdline_to_pid;
+	unsigned cmdline_num;
+	int cmdline_idx;
+	char *saved_cmdlines;
+};
+static struct saved_cmdlines_buffer *savedcmd;
 
 /* temporary disable recording */
 static atomic_t trace_record_cmdline_disabled __read_mostly;
 
-static void trace_init_cmdlines(void)
+static inline char *get_saved_cmdlines(int idx)
+{
+	return &savedcmd->saved_cmdlines[idx * TASK_COMM_LEN];
+}
+
+static inline void set_cmdline(int idx, const char *cmdline)
+{
+	memcpy(get_saved_cmdlines(idx), cmdline, TASK_COMM_LEN);
+}
+
+static int allocate_cmdlines_buffer(unsigned int val,
+				    struct saved_cmdlines_buffer *s)
 {
-	memset(&map_pid_to_cmdline, NO_CMDLINE_MAP, sizeof(map_pid_to_cmdline));
-	memset(&map_cmdline_to_pid, NO_CMDLINE_MAP, sizeof(map_cmdline_to_pid));
-	cmdline_idx = 0;
+	s->map_cmdline_to_pid = kmalloc(val * sizeof(unsigned), GFP_KERNEL);
+	if (!s->map_cmdline_to_pid)
+		goto out;
+
+	s->saved_cmdlines = kmalloc(val * TASK_COMM_LEN, GFP_KERNEL);
+	if (!s->saved_cmdlines)
+		goto out_free_map_cmdline_to_pid;
+
+	return 0;
+
+out_free_map_cmdline_to_pid:
+	kfree(s->map_cmdline_to_pid);
+out:
+	return -ENOMEM;
+}
+
+static void trace_init_cmdlines_buffer(unsigned int val,
+				       struct saved_cmdlines_buffer *s)
+{
+	s->cmdline_idx = 0;
+	s->cmdline_num = val;
+	memset(&s->map_pid_to_cmdline, NO_CMDLINE_MAP,
+	       sizeof(s->map_pid_to_cmdline));
+	memset(s->map_cmdline_to_pid, NO_CMDLINE_MAP,
+	       val * sizeof(*s->map_cmdline_to_pid));
+}
+
+static int trace_create_savedcmd(void)
+{
+	int ret;
+
+	savedcmd = kmalloc(sizeof(struct saved_cmdlines_buffer), GFP_KERNEL);
+	if (!savedcmd)
+		goto out;
+
+	ret = allocate_cmdlines_buffer(SAVED_CMDLINES_DEFAULT, savedcmd);
+	if (ret < 0)
+		goto out_free;
+
+	trace_init_cmdlines_buffer(SAVED_CMDLINES_DEFAULT, savedcmd);
+
+	return 0;
+
+out_free:
+	kfree(savedcmd);
+	savedcmd = NULL;
+out:
+	return -ENOMEM;
 }
 
 int is_tracing_stopped(void)
@@ -1457,9 +1517,9 @@ static int trace_save_cmdline(struct task_struct *tsk)
 	if (!arch_spin_trylock(&trace_cmdline_lock))
 		return 0;
 
-	idx = map_pid_to_cmdline[tsk->pid];
+	idx = savedcmd->map_pid_to_cmdline[tsk->pid];
 	if (idx == NO_CMDLINE_MAP) {
-		idx = (cmdline_idx + 1) % SAVED_CMDLINES;
+		idx = (savedcmd->cmdline_idx + 1) % savedcmd->cmdline_num;
 
 		/*
 		 * Check whether the cmdline buffer at idx has a pid
@@ -1467,17 +1527,17 @@ static int trace_save_cmdline(struct task_struct *tsk)
 		 * need to clear the map_pid_to_cmdline. Otherwise we
 		 * would read the new comm for the old pid.
 		 */
-		pid = map_cmdline_to_pid[idx];
+		pid = savedcmd->map_cmdline_to_pid[idx];
 		if (pid != NO_CMDLINE_MAP)
-			map_pid_to_cmdline[pid] = NO_CMDLINE_MAP;
+			savedcmd->map_pid_to_cmdline[pid] = NO_CMDLINE_MAP;
 
-		map_cmdline_to_pid[idx] = tsk->pid;
-		map_pid_to_cmdline[tsk->pid] = idx;
+		savedcmd->map_cmdline_to_pid[idx] = tsk->pid;
+		savedcmd->map_pid_to_cmdline[tsk->pid] = idx;
 
-		cmdline_idx = idx;
+		savedcmd->cmdline_idx = idx;
 	}
 
-	memcpy(&saved_cmdlines[idx], tsk->comm, TASK_COMM_LEN);
+	set_cmdline(idx, tsk->comm);
 
 	arch_spin_unlock(&trace_cmdline_lock);
 
@@ -1503,9 +1563,9 @@ static void __trace_find_cmdline(int pid, char comm[])
 		return;
 	}
 
-	map = map_pid_to_cmdline[pid];
+	map = savedcmd->map_pid_to_cmdline[pid];
 	if (map != NO_CMDLINE_MAP)
-		strcpy(comm, saved_cmdlines[map]);
+		strcpy(comm, get_saved_cmdlines(map));
 	else
 		strcpy(comm, "<...>");
 }
@@ -3593,6 +3653,7 @@ static const char readme_msg[] =
 	"  trace_options\t\t- Set format or modify how tracing happens\n"
 	"\t\t\t  Disable an option by adding a suffix 'no' to the\n"
 	"\t\t\t  option name\n"
+	"  saved_cmdlines_size\t- echo command number in here to store comm-pid list\n"
 #ifdef CONFIG_DYNAMIC_FTRACE
 	"\n  available_filter_functions - list of functions that can be filtered on\n"
 	"  set_ftrace_filter\t- echo function name in here to only trace these\n"
@@ -3715,7 +3776,8 @@ static void *saved_cmdlines_next(struct seq_file *m, void *v, loff_t *pos)
 
 	(*pos)++;
 
-	for (; ptr < &map_cmdline_to_pid[SAVED_CMDLINES]; ptr++) {
+	for (; ptr < &savedcmd->map_cmdline_to_pid[savedcmd->cmdline_num];
+	     ptr++) {
 		if (*ptr == -1 || *ptr == NO_CMDLINE_MAP)
 			continue;
 
@@ -3733,7 +3795,7 @@ static void *saved_cmdlines_start(struct seq_file *m, loff_t *pos)
 	preempt_disable();
 	arch_spin_lock(&trace_cmdline_lock);
 
-	v = &map_cmdline_to_pid[0];
+	v = &savedcmd->map_cmdline_to_pid[0];
 	while (l <= *pos) {
 		v = saved_cmdlines_next(m, v, &l);
 		if (!v)
@@ -3774,11 +3836,95 @@ static int tracing_saved_cmdlines_open(struct inode *inode, struct file *filp)
 	return seq_open(filp, &tracing_saved_cmdlines_seq_ops);
 }
 
+static int tracing_saved_cmdlines_close(struct inode *inode, struct file *filp)
+{
+	return seq_release(inode, filp);
+}
+
 static const struct file_operations tracing_saved_cmdlines_fops = {
 	.open		= tracing_saved_cmdlines_open,
 	.read		= seq_read,
 	.llseek		= seq_lseek,
-	.release	= seq_release,
+	.release	= tracing_saved_cmdlines_close,
+};
+
+static ssize_t
+tracing_saved_cmdlines_size_read(struct file *filp, char __user *ubuf,
+				 size_t cnt, loff_t *ppos)
+{
+	char buf[64];
+	int r;
+
+	arch_spin_lock(&trace_cmdline_lock);
+	r = sprintf(buf, "%u\n", savedcmd->cmdline_num);
+	arch_spin_unlock(&trace_cmdline_lock);
+
+	return simple_read_from_buffer(ubuf, cnt, ppos, buf, r);
+}
+
+static void free_saved_cmdlines_buffer(struct saved_cmdlines_buffer *s)
+{
+	kfree(s->saved_cmdlines);
+	kfree(s->map_cmdline_to_pid);
+	kfree(s);
+}
+
+static int tracing_resize_saved_cmdlines(unsigned int val)
+{
+	struct saved_cmdlines_buffer *s, *savedcmd_temp;
+	int err = -ENOMEM;
+
+	s = kmalloc(sizeof(struct saved_cmdlines_buffer), GFP_KERNEL);
+	if (!s)
+		goto out;
+
+	if (allocate_cmdlines_buffer(val, s) < 0)
+		goto out_free;
+
+	trace_init_cmdlines_buffer(val, s);
+
+	arch_spin_lock(&trace_cmdline_lock);
+	savedcmd_temp = savedcmd;
+	savedcmd = s;
+	arch_spin_unlock(&trace_cmdline_lock);
+	free_saved_cmdlines_buffer(savedcmd_temp);
+
+	return 0;
+
+out_free:
+	kfree(s);
+out:
+	return err;
+}
+
+static ssize_t
+tracing_saved_cmdlines_size_write(struct file *filp, const char __user *ubuf,
+				  size_t cnt, loff_t *ppos)
+{
+	unsigned long val;
+	int ret;
+
+	ret = kstrtoul_from_user(ubuf, cnt, 10, &val);
+	if (ret)
+		return ret;
+
+	/* must have at least 1 entry or less than PID_MAX_DEFAULT */
+	if (!val || val > PID_MAX_DEFAULT)
+		return -EINVAL;
+
+	ret = tracing_resize_saved_cmdlines((unsigned int)val);
+	if (ret < 0)
+		return ret;
+
+	*ppos += cnt;
+
+	return cnt;
+}
+
+static const struct file_operations tracing_saved_cmdlines_size_fops = {
+	.open		= tracing_open_generic,
+	.read		= tracing_saved_cmdlines_size_read,
+	.write		= tracing_saved_cmdlines_size_write,
 };
 
 static ssize_t
@@ -6375,6 +6521,9 @@ static __init int tracer_init_debugfs(void)
 	trace_create_file("saved_cmdlines", 0444, d_tracer,
 			NULL, &tracing_saved_cmdlines_fops);
 
+	trace_create_file("saved_cmdlines_size", 0644, d_tracer,
+			  NULL, &tracing_saved_cmdlines_size_fops);
+
 #ifdef CONFIG_DYNAMIC_FTRACE
 	trace_create_file("dyn_ftrace_total_info", 0444, d_tracer,
 			&ftrace_update_tot_cnt, &tracing_dyn_info_fops);
@@ -6621,7 +6770,8 @@ __init static int tracer_alloc_buffers(void)
 	if (global_trace.buffer_disabled)
 		tracing_off();
 
-	trace_init_cmdlines();
+	if (trace_create_savedcmd() < 0)
+		goto out_free_trace_buffers;
 
 	if (trace_boot_clock) {
 		ret = tracing_set_clock(&global_trace, trace_boot_clock);
@@ -6668,6 +6818,13 @@ __init static int tracer_alloc_buffers(void)
 
 	return 0;
 
+out_free_trace_buffers:
+	ring_buffer_free(global_trace.trace_buffer.buffer);
+	free_percpu(global_trace.trace_buffer.data);
+#ifdef CONFIG_TRACER_MAX_TRACE
+	ring_buffer_free(global_trace.max_buffer.buffer);
+	free_percpu(global_trace.max_buffer.data);
+#endif
 out_free_temp_buffer:
 	ring_buffer_free(temp_buffer);
 out_free_cpumask:

Re: [PATCH ftrace/core 1/2] [BUGFIX] ftrace: Avoid panic when allocation of max_buffer is failed

[Steven Rostedt]

On Tue, 03 Jun 2014 13:28:03 +0900
Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com> wrote:

> When allocation of max_buffer is failed, the kernel frees tr->trace_buffer.data
> per CPU and return -ENOMEM in allocate_trace_buffers(). However,
> tracer_alloc_buffers() calling allocate_trace_buffers() also frees the data
> per CPU for -ENOMEM by allocate_trace_buffers(). Therefore, the allocation
> failure induces double free.
> 
> For the out_free_mask path in tracer_alloc_buffers(),
> global_trace.trace_buffer.data and global_trace.max_buffer.data are
> not allocated yet, so free_percpu of those are not needed.
> 
> Signed-off-by: Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: linux-kernel@vger.kernel.org
> ---
>  kernel/trace/trace.c |    4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 626dbfd..135af32 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -6671,10 +6671,6 @@ __init static int tracer_alloc_buffers(void)
>  out_free_temp_buffer:
>  	ring_buffer_free(temp_buffer);
>  out_free_cpumask:
> -	free_percpu(global_trace.trace_buffer.data);
> -#ifdef CONFIG_TRACER_MAX_TRACE
> -	free_percpu(global_trace.max_buffer.data);
> -#endif
>  	free_cpumask_var(global_trace.tracing_cpumask);
>  out_free_buffer_mask:
>  	free_cpumask_var(tracing_buffer_mask);

OK, so this is a double free on an error path at boot up. As it is
highly unlikely, I'll just add it for my 3.16 queue. It doesn't need to
go to stable.

-- Steve

Re: [PATCH ftrace/core 1/2] [BUGFIX] ftrace: Avoid panic when allocation of max_buffer is failed

[Steven Rostedt]

As your change log and subject make the bug sound much worse than it
is, I re-wrote both. Here's the new version:

Subject: [PATCH ftrace/core 1/2] [BUGFIX] tracing: Eliminate double free on failure of allocation on boot up

If allocation of the max_buffer fails on boot up, the error path will
free both per_cpu data structures from the buffers. With the new redesign
of the code, those structures are freed if allocations failed. That is,
the helper function that allocates the buffers will free the per cpu data
on failure. No need to do it again. In fact, the second free will cause
a bug as the code can not handle a double free.

Signed-off-by: Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com>
---


-- Steve

Re: [PATCH ftrace/core 2/2] ftrace: Introduce saved_cmdlines_size file

[Steven Rostedt]

On Tue, 03 Jun 2014 13:28:05 +0900
Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com> wrote:

> ---
>  kernel/trace/trace.c |  203 ++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 180 insertions(+), 23 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 135af32..473eb68 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -1285,22 +1285,82 @@ void tracing_reset_all_online_cpus(void)
>  	}
>  }
>  
> -#define SAVED_CMDLINES 128
> +#define SAVED_CMDLINES_DEFAULT 128
>  #define NO_CMDLINE_MAP UINT_MAX
> -static unsigned map_pid_to_cmdline[PID_MAX_DEFAULT+1];
> -static unsigned map_cmdline_to_pid[SAVED_CMDLINES];
> -static char saved_cmdlines[SAVED_CMDLINES][TASK_COMM_LEN];
> -static int cmdline_idx;
>  static arch_spinlock_t trace_cmdline_lock = __ARCH_SPIN_LOCK_UNLOCKED;
> +struct saved_cmdlines_buffer {
> +	unsigned map_pid_to_cmdline[PID_MAX_DEFAULT+1];
> +	unsigned *map_cmdline_to_pid;
> +	unsigned cmdline_num;
> +	int cmdline_idx;
> +	char *saved_cmdlines;
> +};
> +static struct saved_cmdlines_buffer *savedcmd;
>  
>  /* temporary disable recording */
>  static atomic_t trace_record_cmdline_disabled __read_mostly;
>  
> -static void trace_init_cmdlines(void)
> +static inline char *get_saved_cmdlines(int idx)
> +{
> +	return &savedcmd->saved_cmdlines[idx * TASK_COMM_LEN];
> +}
> +
> +static inline void set_cmdline(int idx, const char *cmdline)
> +{
> +	memcpy(get_saved_cmdlines(idx), cmdline, TASK_COMM_LEN);
> +}
> +
> +static int allocate_cmdlines_buffer(unsigned int val,
> +				    struct saved_cmdlines_buffer *s)
>  {
> -	memset(&map_pid_to_cmdline, NO_CMDLINE_MAP, sizeof(map_pid_to_cmdline));
> -	memset(&map_cmdline_to_pid, NO_CMDLINE_MAP, sizeof(map_cmdline_to_pid));
> -	cmdline_idx = 0;
> +	s->map_cmdline_to_pid = kmalloc(val * sizeof(unsigned), GFP_KERNEL);
> +	if (!s->map_cmdline_to_pid)
> +		goto out;
> +
> +	s->saved_cmdlines = kmalloc(val * TASK_COMM_LEN, GFP_KERNEL);
> +	if (!s->saved_cmdlines)
> +		goto out_free_map_cmdline_to_pid;
> +
> +	return 0;
> +
> +out_free_map_cmdline_to_pid:
> +	kfree(s->map_cmdline_to_pid);
> +out:
> +	return -ENOMEM;
> +}
> +
> +static void trace_init_cmdlines_buffer(unsigned int val,
> +				       struct saved_cmdlines_buffer *s)
> +{
> +	s->cmdline_idx = 0;
> +	s->cmdline_num = val;
> +	memset(&s->map_pid_to_cmdline, NO_CMDLINE_MAP,
> +	       sizeof(s->map_pid_to_cmdline));
> +	memset(s->map_cmdline_to_pid, NO_CMDLINE_MAP,
> +	       val * sizeof(*s->map_cmdline_to_pid));
> +}

Please combine the above two functions. Get rid of
trace_init_cmdlines_buffer(). That is, move the contents of
trace_init_cmdlines_buffer() into allocate_cmdline_buffer() just before
the return 0. There's no reason that we need to have two functions, one
to allocate and one to initialize it. The allocation can also
initialize it.

> +
> +static int trace_create_savedcmd(void)
> +{
> +	int ret;
> +
> +	savedcmd = kmalloc(sizeof(struct saved_cmdlines_buffer), GFP_KERNEL);
> +	if (!savedcmd)
> +		goto out;

	if (!savedcmd)
		return -ENOMEM;

> +
> +	ret = allocate_cmdlines_buffer(SAVED_CMDLINES_DEFAULT, savedcmd);
> +	if (ret < 0)
> +		goto out_free;

	if (ret < 0) {
		kfree(savedcmd);
		savedcmd = NULL;
		return -ENOMEM;
	}

This function is small enough that we don't need to use gotos to make
it clean.

> +
> +	trace_init_cmdlines_buffer(SAVED_CMDLINES_DEFAULT, savedcmd);
> +
> +	return 0;
> +
> +out_free:
> +	kfree(savedcmd);
> +	savedcmd = NULL;
> +out:
> +	return -ENOMEM;
>  }
>  
>  int is_tracing_stopped(void)
> @@ -1457,9 +1517,9 @@ static int trace_save_cmdline(struct task_struct *tsk)
>  	if (!arch_spin_trylock(&trace_cmdline_lock))
>  		return 0;
>  
> -	idx = map_pid_to_cmdline[tsk->pid];
> +	idx = savedcmd->map_pid_to_cmdline[tsk->pid];
>  	if (idx == NO_CMDLINE_MAP) {
> -		idx = (cmdline_idx + 1) % SAVED_CMDLINES;
> +		idx = (savedcmd->cmdline_idx + 1) % savedcmd->cmdline_num;
>  
>  		/*
>  		 * Check whether the cmdline buffer at idx has a pid
> @@ -1467,17 +1527,17 @@ static int trace_save_cmdline(struct task_struct *tsk)
>  		 * need to clear the map_pid_to_cmdline. Otherwise we
>  		 * would read the new comm for the old pid.
>  		 */
> -		pid = map_cmdline_to_pid[idx];
> +		pid = savedcmd->map_cmdline_to_pid[idx];
>  		if (pid != NO_CMDLINE_MAP)
> -			map_pid_to_cmdline[pid] = NO_CMDLINE_MAP;
> +			savedcmd->map_pid_to_cmdline[pid] = NO_CMDLINE_MAP;
>  
> -		map_cmdline_to_pid[idx] = tsk->pid;
> -		map_pid_to_cmdline[tsk->pid] = idx;
> +		savedcmd->map_cmdline_to_pid[idx] = tsk->pid;
> +		savedcmd->map_pid_to_cmdline[tsk->pid] = idx;
>  
> -		cmdline_idx = idx;
> +		savedcmd->cmdline_idx = idx;
>  	}
>  
> -	memcpy(&saved_cmdlines[idx], tsk->comm, TASK_COMM_LEN);
> +	set_cmdline(idx, tsk->comm);
>  
>  	arch_spin_unlock(&trace_cmdline_lock);
>  
> @@ -1503,9 +1563,9 @@ static void __trace_find_cmdline(int pid, char comm[])
>  		return;
>  	}
>  
> -	map = map_pid_to_cmdline[pid];
> +	map = savedcmd->map_pid_to_cmdline[pid];
>  	if (map != NO_CMDLINE_MAP)
> -		strcpy(comm, saved_cmdlines[map]);
> +		strcpy(comm, get_saved_cmdlines(map));
>  	else
>  		strcpy(comm, "<...>");
>  }
> @@ -3593,6 +3653,7 @@ static const char readme_msg[] =
>  	"  trace_options\t\t- Set format or modify how tracing happens\n"
>  	"\t\t\t  Disable an option by adding a suffix 'no' to the\n"
>  	"\t\t\t  option name\n"
> +	"  saved_cmdlines_size\t- echo command number in here to store comm-pid list\n"
>  #ifdef CONFIG_DYNAMIC_FTRACE
>  	"\n  available_filter_functions - list of functions that can be filtered on\n"
>  	"  set_ftrace_filter\t- echo function name in here to only trace these\n"
> @@ -3715,7 +3776,8 @@ static void *saved_cmdlines_next(struct seq_file *m, void *v, loff_t *pos)
>  
>  	(*pos)++;
>  
> -	for (; ptr < &map_cmdline_to_pid[SAVED_CMDLINES]; ptr++) {
> +	for (; ptr < &savedcmd->map_cmdline_to_pid[savedcmd->cmdline_num];
> +	     ptr++) {
>  		if (*ptr == -1 || *ptr == NO_CMDLINE_MAP)
>  			continue;
>  
> @@ -3733,7 +3795,7 @@ static void *saved_cmdlines_start(struct seq_file *m, loff_t *pos)
>  	preempt_disable();
>  	arch_spin_lock(&trace_cmdline_lock);
>  
> -	v = &map_cmdline_to_pid[0];
> +	v = &savedcmd->map_cmdline_to_pid[0];
>  	while (l <= *pos) {
>  		v = saved_cmdlines_next(m, v, &l);
>  		if (!v)
> @@ -3774,11 +3836,95 @@ static int tracing_saved_cmdlines_open(struct inode *inode, struct file *filp)
>  	return seq_open(filp, &tracing_saved_cmdlines_seq_ops);
>  }
>  
> +static int tracing_saved_cmdlines_close(struct inode *inode, struct file *filp)
> +{
> +	return seq_release(inode, filp);
> +}
> +
>  static const struct file_operations tracing_saved_cmdlines_fops = {
>  	.open		= tracing_saved_cmdlines_open,
>  	.read		= seq_read,
>  	.llseek		= seq_lseek,
> -	.release	= seq_release,
> +	.release	= tracing_saved_cmdlines_close,

Confused. tracing_saved_cmdlines_close() just does a seq_release()
passing in the same arguments. Why this change? Is there some clean up
missing here?

Oh, with the updates in ftrace/core, you don't need it anymore. Remove
that helper function and just call seq_release() via .release.

> +};
> +
> +static ssize_t
> +tracing_saved_cmdlines_size_read(struct file *filp, char __user *ubuf,
> +				 size_t cnt, loff_t *ppos)
> +{
> +	char buf[64];
> +	int r;
> +
> +	arch_spin_lock(&trace_cmdline_lock);
> +	r = sprintf(buf, "%u\n", savedcmd->cmdline_num);
> +	arch_spin_unlock(&trace_cmdline_lock);
> +
> +	return simple_read_from_buffer(ubuf, cnt, ppos, buf, r);
> +}
> +
> +static void free_saved_cmdlines_buffer(struct saved_cmdlines_buffer *s)
> +{
> +	kfree(s->saved_cmdlines);
> +	kfree(s->map_cmdline_to_pid);
> +	kfree(s);
> +}
> +
> +static int tracing_resize_saved_cmdlines(unsigned int val)
> +{
> +	struct saved_cmdlines_buffer *s, *savedcmd_temp;
> +	int err = -ENOMEM;

You don't need err.

> +
> +	s = kmalloc(sizeof(struct saved_cmdlines_buffer), GFP_KERNEL);
> +	if (!s)
		return -ENOMEM;

> +		goto out;
> +
> +	if (allocate_cmdlines_buffer(val, s) < 0)
					{
		kfree(s);
		return -ENOMEM;
	}

Again, this function is small enough not to need the extra gotos.

> +		goto out_free;
> +
> +	trace_init_cmdlines_buffer(val, s);
> +
> +	arch_spin_lock(&trace_cmdline_lock);
> +	savedcmd_temp = savedcmd;
> +	savedcmd = s;
> +	arch_spin_unlock(&trace_cmdline_lock);
> +	free_saved_cmdlines_buffer(savedcmd_temp);
> +
> +	return 0;
> +
> +out_free:
> +	kfree(s);
> +out:
> +	return err;
> +}
> +
> +static ssize_t
> +tracing_saved_cmdlines_size_write(struct file *filp, const char __user *ubuf,
> +				  size_t cnt, loff_t *ppos)
> +{
> +	unsigned long val;
> +	int ret;
> +
> +	ret = kstrtoul_from_user(ubuf, cnt, 10, &val);
> +	if (ret)
> +		return ret;
> +
> +	/* must have at least 1 entry or less than PID_MAX_DEFAULT */
> +	if (!val || val > PID_MAX_DEFAULT)
> +		return -EINVAL;
> +
> +	ret = tracing_resize_saved_cmdlines((unsigned int)val);
> +	if (ret < 0)
> +		return ret;
> +
> +	*ppos += cnt;
> +
> +	return cnt;
> +}
> +
> +static const struct file_operations tracing_saved_cmdlines_size_fops = {
> +	.open		= tracing_open_generic,
> +	.read		= tracing_saved_cmdlines_size_read,
> +	.write		= tracing_saved_cmdlines_size_write,
>  };
>  
>  static ssize_t
> @@ -6375,6 +6521,9 @@ static __init int tracer_init_debugfs(void)
>  	trace_create_file("saved_cmdlines", 0444, d_tracer,
>  			NULL, &tracing_saved_cmdlines_fops);
>  
> +	trace_create_file("saved_cmdlines_size", 0644, d_tracer,
> +			  NULL, &tracing_saved_cmdlines_size_fops);
> +
>  #ifdef CONFIG_DYNAMIC_FTRACE
>  	trace_create_file("dyn_ftrace_total_info", 0444, d_tracer,
>  			&ftrace_update_tot_cnt, &tracing_dyn_info_fops);
> @@ -6621,7 +6770,8 @@ __init static int tracer_alloc_buffers(void)
>  	if (global_trace.buffer_disabled)
>  		tracing_off();
>  
> -	trace_init_cmdlines();
> +	if (trace_create_savedcmd() < 0)
> +		goto out_free_trace_buffers;

Actually, since the freeing of saved_cmdlines is easier, move the
creation of it before the buffers, and if it fails, we don't need to
worry about freeing the buffers. Then if the buffers fail, we just need
to free the cmdlines.

-- Steve

>  
>  	if (trace_boot_clock) {
>  		ret = tracing_set_clock(&global_trace, trace_boot_clock);
> @@ -6668,6 +6818,13 @@ __init static int tracer_alloc_buffers(void)
>  
>  	return 0;
>  
> +out_free_trace_buffers:
> +	ring_buffer_free(global_trace.trace_buffer.buffer);
> +	free_percpu(global_trace.trace_buffer.data);
> +#ifdef CONFIG_TRACER_MAX_TRACE
> +	ring_buffer_free(global_trace.max_buffer.buffer);
> +	free_percpu(global_trace.max_buffer.data);
> +#endif
>  out_free_temp_buffer:
>  	ring_buffer_free(temp_buffer);
>  out_free_cpumask:

Re: Re: [PATCH ftrace/core 2/2] ftrace: Introduce saved_cmdlines_size file

[Yoshihiro YUNOMAE]

Hi Steven,

Thank you for your review.

(2014/06/04 9:30), Steven Rostedt wrote:
> On Tue, 03 Jun 2014 13:28:05 +0900
> Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com> wrote:
>
>> ---
>>   kernel/trace/trace.c |  203 ++++++++++++++++++++++++++++++++++++++++++++------
>>   1 file changed, 180 insertions(+), 23 deletions(-)
>>
>> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
>> index 135af32..473eb68 100644
>> --- a/kernel/trace/trace.c
>> +++ b/kernel/trace/trace.c
>> @@ -1285,22 +1285,82 @@ void tracing_reset_all_online_cpus(void)
>>   	}
>>   }
>>
>> -#define SAVED_CMDLINES 128
>> +#define SAVED_CMDLINES_DEFAULT 128
>>   #define NO_CMDLINE_MAP UINT_MAX
>> -static unsigned map_pid_to_cmdline[PID_MAX_DEFAULT+1];
>> -static unsigned map_cmdline_to_pid[SAVED_CMDLINES];
>> -static char saved_cmdlines[SAVED_CMDLINES][TASK_COMM_LEN];
>> -static int cmdline_idx;
>>   static arch_spinlock_t trace_cmdline_lock = __ARCH_SPIN_LOCK_UNLOCKED;
>> +struct saved_cmdlines_buffer {
>> +	unsigned map_pid_to_cmdline[PID_MAX_DEFAULT+1];
>> +	unsigned *map_cmdline_to_pid;
>> +	unsigned cmdline_num;
>> +	int cmdline_idx;
>> +	char *saved_cmdlines;
>> +};
>> +static struct saved_cmdlines_buffer *savedcmd;
>>
>>   /* temporary disable recording */
>>   static atomic_t trace_record_cmdline_disabled __read_mostly;
>>
>> -static void trace_init_cmdlines(void)
>> +static inline char *get_saved_cmdlines(int idx)
>> +{
>> +	return &savedcmd->saved_cmdlines[idx * TASK_COMM_LEN];
>> +}
>> +
>> +static inline void set_cmdline(int idx, const char *cmdline)
>> +{
>> +	memcpy(get_saved_cmdlines(idx), cmdline, TASK_COMM_LEN);
>> +}
>> +
>> +static int allocate_cmdlines_buffer(unsigned int val,
>> +				    struct saved_cmdlines_buffer *s)
>>   {
>> -	memset(&map_pid_to_cmdline, NO_CMDLINE_MAP, sizeof(map_pid_to_cmdline));
>> -	memset(&map_cmdline_to_pid, NO_CMDLINE_MAP, sizeof(map_cmdline_to_pid));
>> -	cmdline_idx = 0;
>> +	s->map_cmdline_to_pid = kmalloc(val * sizeof(unsigned), GFP_KERNEL);
>> +	if (!s->map_cmdline_to_pid)
>> +		goto out;
>> +
>> +	s->saved_cmdlines = kmalloc(val * TASK_COMM_LEN, GFP_KERNEL);
>> +	if (!s->saved_cmdlines)
>> +		goto out_free_map_cmdline_to_pid;
>> +
>> +	return 0;
>> +
>> +out_free_map_cmdline_to_pid:
>> +	kfree(s->map_cmdline_to_pid);
>> +out:
>> +	return -ENOMEM;
>> +}
>> +
>> +static void trace_init_cmdlines_buffer(unsigned int val,
>> +				       struct saved_cmdlines_buffer *s)
>> +{
>> +	s->cmdline_idx = 0;
>> +	s->cmdline_num = val;
>> +	memset(&s->map_pid_to_cmdline, NO_CMDLINE_MAP,
>> +	       sizeof(s->map_pid_to_cmdline));
>> +	memset(s->map_cmdline_to_pid, NO_CMDLINE_MAP,
>> +	       val * sizeof(*s->map_cmdline_to_pid));
>> +}
>
> Please combine the above two functions. Get rid of
> trace_init_cmdlines_buffer(). That is, move the contents of
> trace_init_cmdlines_buffer() into allocate_cmdline_buffer() just before
> the return 0. There's no reason that we need to have two functions, one
> to allocate and one to initialize it. The allocation can also
> initialize it.

OK, I'll move the initialization into allocate_cmdline_buffer().

>> +
>> +static int trace_create_savedcmd(void)
>> +{
>> +	int ret;
>> +
>> +	savedcmd = kmalloc(sizeof(struct saved_cmdlines_buffer), GFP_KERNEL);
>> +	if (!savedcmd)
>> +		goto out;
>
> 	if (!savedcmd)
> 		return -ENOMEM;
>
>> +
>> +	ret = allocate_cmdlines_buffer(SAVED_CMDLINES_DEFAULT, savedcmd);
>> +	if (ret < 0)
>> +		goto out_free;
>
> 	if (ret < 0) {
> 		kfree(savedcmd);
> 		savedcmd = NULL;
> 		return -ENOMEM;
> 	}
>
> This function is small enough that we don't need to use gotos to make
> it clean.

OK. I'll remove gotos.

>> +
>> +	trace_init_cmdlines_buffer(SAVED_CMDLINES_DEFAULT, savedcmd);
>> +
>> +	return 0;
>> +
>> +out_free:
>> +	kfree(savedcmd);
>> +	savedcmd = NULL;
>> +out:
>> +	return -ENOMEM;
>>   }
>>
>>   int is_tracing_stopped(void)
>> @@ -1457,9 +1517,9 @@ static int trace_save_cmdline(struct task_struct *tsk)
>>   	if (!arch_spin_trylock(&trace_cmdline_lock))
>>   		return 0;
>>
>> -	idx = map_pid_to_cmdline[tsk->pid];
>> +	idx = savedcmd->map_pid_to_cmdline[tsk->pid];
>>   	if (idx == NO_CMDLINE_MAP) {
>> -		idx = (cmdline_idx + 1) % SAVED_CMDLINES;
>> +		idx = (savedcmd->cmdline_idx + 1) % savedcmd->cmdline_num;
>>
>>   		/*
>>   		 * Check whether the cmdline buffer at idx has a pid
>> @@ -1467,17 +1527,17 @@ static int trace_save_cmdline(struct task_struct *tsk)
>>   		 * need to clear the map_pid_to_cmdline. Otherwise we
>>   		 * would read the new comm for the old pid.
>>   		 */
>> -		pid = map_cmdline_to_pid[idx];
>> +		pid = savedcmd->map_cmdline_to_pid[idx];
>>   		if (pid != NO_CMDLINE_MAP)
>> -			map_pid_to_cmdline[pid] = NO_CMDLINE_MAP;
>> +			savedcmd->map_pid_to_cmdline[pid] = NO_CMDLINE_MAP;
>>
>> -		map_cmdline_to_pid[idx] = tsk->pid;
>> -		map_pid_to_cmdline[tsk->pid] = idx;
>> +		savedcmd->map_cmdline_to_pid[idx] = tsk->pid;
>> +		savedcmd->map_pid_to_cmdline[tsk->pid] = idx;
>>
>> -		cmdline_idx = idx;
>> +		savedcmd->cmdline_idx = idx;
>>   	}
>>
>> -	memcpy(&saved_cmdlines[idx], tsk->comm, TASK_COMM_LEN);
>> +	set_cmdline(idx, tsk->comm);
>>
>>   	arch_spin_unlock(&trace_cmdline_lock);
>>
>> @@ -1503,9 +1563,9 @@ static void __trace_find_cmdline(int pid, char comm[])
>>   		return;
>>   	}
>>
>> -	map = map_pid_to_cmdline[pid];
>> +	map = savedcmd->map_pid_to_cmdline[pid];
>>   	if (map != NO_CMDLINE_MAP)
>> -		strcpy(comm, saved_cmdlines[map]);
>> +		strcpy(comm, get_saved_cmdlines(map));
>>   	else
>>   		strcpy(comm, "<...>");
>>   }
>> @@ -3593,6 +3653,7 @@ static const char readme_msg[] =
>>   	"  trace_options\t\t- Set format or modify how tracing happens\n"
>>   	"\t\t\t  Disable an option by adding a suffix 'no' to the\n"
>>   	"\t\t\t  option name\n"
>> +	"  saved_cmdlines_size\t- echo command number in here to store comm-pid list\n"
>>   #ifdef CONFIG_DYNAMIC_FTRACE
>>   	"\n  available_filter_functions - list of functions that can be filtered on\n"
>>   	"  set_ftrace_filter\t- echo function name in here to only trace these\n"
>> @@ -3715,7 +3776,8 @@ static void *saved_cmdlines_next(struct seq_file *m, void *v, loff_t *pos)
>>
>>   	(*pos)++;
>>
>> -	for (; ptr < &map_cmdline_to_pid[SAVED_CMDLINES]; ptr++) {
>> +	for (; ptr < &savedcmd->map_cmdline_to_pid[savedcmd->cmdline_num];
>> +	     ptr++) {
>>   		if (*ptr == -1 || *ptr == NO_CMDLINE_MAP)
>>   			continue;
>>
>> @@ -3733,7 +3795,7 @@ static void *saved_cmdlines_start(struct seq_file *m, loff_t *pos)
>>   	preempt_disable();
>>   	arch_spin_lock(&trace_cmdline_lock);
>>
>> -	v = &map_cmdline_to_pid[0];
>> +	v = &savedcmd->map_cmdline_to_pid[0];
>>   	while (l <= *pos) {
>>   		v = saved_cmdlines_next(m, v, &l);
>>   		if (!v)
>> @@ -3774,11 +3836,95 @@ static int tracing_saved_cmdlines_open(struct inode *inode, struct file *filp)
>>   	return seq_open(filp, &tracing_saved_cmdlines_seq_ops);
>>   }
>>
>> +static int tracing_saved_cmdlines_close(struct inode *inode, struct file *filp)
>> +{
>> +	return seq_release(inode, filp);
>> +}
>> +
>>   static const struct file_operations tracing_saved_cmdlines_fops = {
>>   	.open		= tracing_saved_cmdlines_open,
>>   	.read		= seq_read,
>>   	.llseek		= seq_lseek,
>> -	.release	= seq_release,
>> +	.release	= tracing_saved_cmdlines_close,
>
> Confused. tracing_saved_cmdlines_close() just does a seq_release()
> passing in the same arguments. Why this change? Is there some clean up
> missing here?
>
> Oh, with the updates in ftrace/core, you don't need it anymore. Remove
> that helper function and just call seq_release() via .release.

Oh, I forgot to fix here.
As you say, we should call just seq_release here.

>> +};
>> +
>> +static ssize_t
>> +tracing_saved_cmdlines_size_read(struct file *filp, char __user *ubuf,
>> +				 size_t cnt, loff_t *ppos)
>> +{
>> +	char buf[64];
>> +	int r;
>> +
>> +	arch_spin_lock(&trace_cmdline_lock);
>> +	r = sprintf(buf, "%u\n", savedcmd->cmdline_num);
>> +	arch_spin_unlock(&trace_cmdline_lock);
>> +
>> +	return simple_read_from_buffer(ubuf, cnt, ppos, buf, r);
>> +}
>> +
>> +static void free_saved_cmdlines_buffer(struct saved_cmdlines_buffer *s)
>> +{
>> +	kfree(s->saved_cmdlines);
>> +	kfree(s->map_cmdline_to_pid);
>> +	kfree(s);
>> +}
>> +
>> +static int tracing_resize_saved_cmdlines(unsigned int val)
>> +{
>> +	struct saved_cmdlines_buffer *s, *savedcmd_temp;
>> +	int err = -ENOMEM;
>
> You don't need err.
>
>> +
>> +	s = kmalloc(sizeof(struct saved_cmdlines_buffer), GFP_KERNEL);
>> +	if (!s)
> 		return -ENOMEM;
>
>> +		goto out;
>> +
>> +	if (allocate_cmdlines_buffer(val, s) < 0)
> 					{
> 		kfree(s);
> 		return -ENOMEM;
> 	}
>
> Again, this function is small enough not to need the extra gotos.

OK, I'll remove gotos.

>> +		goto out_free;
>> +
>> +	trace_init_cmdlines_buffer(val, s);
>> +
>> +	arch_spin_lock(&trace_cmdline_lock);
>> +	savedcmd_temp = savedcmd;
>> +	savedcmd = s;
>> +	arch_spin_unlock(&trace_cmdline_lock);
>> +	free_saved_cmdlines_buffer(savedcmd_temp);
>> +
>> +	return 0;
>> +
>> +out_free:
>> +	kfree(s);
>> +out:
>> +	return err;
>> +}
>> +
>> +static ssize_t
>> +tracing_saved_cmdlines_size_write(struct file *filp, const char __user *ubuf,
>> +				  size_t cnt, loff_t *ppos)
>> +{
>> +	unsigned long val;
>> +	int ret;
>> +
>> +	ret = kstrtoul_from_user(ubuf, cnt, 10, &val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* must have at least 1 entry or less than PID_MAX_DEFAULT */
>> +	if (!val || val > PID_MAX_DEFAULT)
>> +		return -EINVAL;
>> +
>> +	ret = tracing_resize_saved_cmdlines((unsigned int)val);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	*ppos += cnt;
>> +
>> +	return cnt;
>> +}
>> +
>> +static const struct file_operations tracing_saved_cmdlines_size_fops = {
>> +	.open		= tracing_open_generic,
>> +	.read		= tracing_saved_cmdlines_size_read,
>> +	.write		= tracing_saved_cmdlines_size_write,
>>   };
>>
>>   static ssize_t
>> @@ -6375,6 +6521,9 @@ static __init int tracer_init_debugfs(void)
>>   	trace_create_file("saved_cmdlines", 0444, d_tracer,
>>   			NULL, &tracing_saved_cmdlines_fops);
>>
>> +	trace_create_file("saved_cmdlines_size", 0644, d_tracer,
>> +			  NULL, &tracing_saved_cmdlines_size_fops);
>> +
>>   #ifdef CONFIG_DYNAMIC_FTRACE
>>   	trace_create_file("dyn_ftrace_total_info", 0444, d_tracer,
>>   			&ftrace_update_tot_cnt, &tracing_dyn_info_fops);
>> @@ -6621,7 +6770,8 @@ __init static int tracer_alloc_buffers(void)
>>   	if (global_trace.buffer_disabled)
>>   		tracing_off();
>>
>> -	trace_init_cmdlines();
>> +	if (trace_create_savedcmd() < 0)
>> +		goto out_free_trace_buffers;
>
> Actually, since the freeing of saved_cmdlines is easier, move the
> creation of it before the buffers, and if it fails, we don't need to
> worry about freeing the buffers. Then if the buffers fail, we just need
> to free the cmdlines.

OK, I'll move trace_create_savedcmd() before allocate_trace_buffers().

Thank you,
Yoshihiro YUNOMAE

-- 
Yoshihiro YUNOMAE
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: yoshihiro.yunomae.ez@hitachi.com

[V4 PATCH ftrace/core] ftrace: Introduce saved_cmdlines_size file

[Yoshihiro YUNOMAE]

Introduce saved_cmdlines_size file for changing the number of pid-comm list.
saved_cmdlines can store 128 command names using SAVED_CMDLINES now, but
'no-existing processes' names are often lost in saved_cmdlines when we
read trace data. So, by introducing saved_cmdlines_size file, the rule storing
128 command names is changed to the command numbers defined users.

When we write a value to saved_cmdlines_size, the number of the value will
be stored in pid-comm list:

	# echo 1024 > /sys/kernel/debug/tracing/saved_cmdlines_size

Here, 1024 command names are stored. The default number is 128 and the maximum
number is PID_MAX_DEFAULT (=32768 if CONFIG_BASE_SMALL is not set). So, if we
want to avoid to lose command names, we need to set 32768 to
saved_cmdlines_size.

We can read the maximum number of the list:

	# cat /sys/kernel/debug/tracing/saved_cmdlines_size
	128

Changes in V2:
- Fix a racing problem of savedcmd between saved_cmdlines I/F and
  nr_saved_cmdlines I/F. If one reads saved_cmdlines and writes a value to
  nr_saved_cmdlines at the same time, then the write returns -EBUSY.

  <How to test>
  [terminal 1] Read saved_cmdlines
  # while true; do cat saved_cmdlines > /dev/null; done;

  [terminal 2] Write 1024 to nr_saved_cmdlines
  # while true; do echo 1024 > nr_saved_cmdlines; done;
  -bash: echo: write error: Device or resource busy
  -bash: echo: write error: Device or resource busy
  -bash: echo: write error: Device or resource busy

Changes in V3:
- Change 'nr_saved_cmdlines' to 'saved_cmdlines_size'
- Delete two helper functions(trace_init_savedcmd() and
  trace_create_and_init_saved_cmd())
- Rebase this patch for current ftrace/core tree
- Remove reader member in saved_cmdlines_buffer structure because the racing
  problem does not occur even if we don't use the member in this patch
- Fix several typos

Changes in V4:
- Remove trace_init_cmdlines_buffer()
- Remove extra gotos in allocate_cmdlines_buffer(), trace_create_savedcmd(),
  and tracing_resize_saved_cmdlines.
- Use seq_release instead of tracing_saved_cmdlines_close
- Move trace_create_savedcmd() before allocate_trace_buffers because freeing
  of saved_cmdlines is easier than that of global_trace buffers.

Signed-off-by: Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: linux-kernel@vger.kernel.org
---
 kernel/trace/trace.c |  177 +++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 153 insertions(+), 24 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 135af32..ed9185c 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -1285,22 +1285,70 @@ void tracing_reset_all_online_cpus(void)
 	}
 }
 
-#define SAVED_CMDLINES 128
+#define SAVED_CMDLINES_DEFAULT 128
 #define NO_CMDLINE_MAP UINT_MAX
-static unsigned map_pid_to_cmdline[PID_MAX_DEFAULT+1];
-static unsigned map_cmdline_to_pid[SAVED_CMDLINES];
-static char saved_cmdlines[SAVED_CMDLINES][TASK_COMM_LEN];
-static int cmdline_idx;
 static arch_spinlock_t trace_cmdline_lock = __ARCH_SPIN_LOCK_UNLOCKED;
+struct saved_cmdlines_buffer {
+	unsigned map_pid_to_cmdline[PID_MAX_DEFAULT+1];
+	unsigned *map_cmdline_to_pid;
+	unsigned cmdline_num;
+	int cmdline_idx;
+	char *saved_cmdlines;
+};
+static struct saved_cmdlines_buffer *savedcmd;
 
 /* temporary disable recording */
 static atomic_t trace_record_cmdline_disabled __read_mostly;
 
-static void trace_init_cmdlines(void)
+static inline char *get_saved_cmdlines(int idx)
+{
+	return &savedcmd->saved_cmdlines[idx * TASK_COMM_LEN];
+}
+
+static inline void set_cmdline(int idx, const char *cmdline)
 {
-	memset(&map_pid_to_cmdline, NO_CMDLINE_MAP, sizeof(map_pid_to_cmdline));
-	memset(&map_cmdline_to_pid, NO_CMDLINE_MAP, sizeof(map_cmdline_to_pid));
-	cmdline_idx = 0;
+	memcpy(get_saved_cmdlines(idx), cmdline, TASK_COMM_LEN);
+}
+
+static int allocate_cmdlines_buffer(unsigned int val,
+				    struct saved_cmdlines_buffer *s)
+{
+	s->map_cmdline_to_pid = kmalloc(val * sizeof(unsigned), GFP_KERNEL);
+	if (!s->map_cmdline_to_pid)
+		return -ENOMEM;
+
+	s->saved_cmdlines = kmalloc(val * TASK_COMM_LEN, GFP_KERNEL);
+	if (!s->saved_cmdlines) {
+		kfree(s->map_cmdline_to_pid);
+		return -ENOMEM;
+	}
+
+	s->cmdline_idx = 0;
+	s->cmdline_num = val;
+	memset(&s->map_pid_to_cmdline, NO_CMDLINE_MAP,
+	       sizeof(s->map_pid_to_cmdline));
+	memset(s->map_cmdline_to_pid, NO_CMDLINE_MAP,
+	       val * sizeof(*s->map_cmdline_to_pid));
+
+	return 0;
+}
+
+static int trace_create_savedcmd(void)
+{
+	int ret;
+
+	savedcmd = kmalloc(sizeof(struct saved_cmdlines_buffer), GFP_KERNEL);
+	if (!savedcmd)
+		return -ENOMEM;
+
+	ret = allocate_cmdlines_buffer(SAVED_CMDLINES_DEFAULT, savedcmd);
+	if (ret < 0) {
+		kfree(savedcmd);
+		savedcmd = NULL;
+		return -ENOMEM;
+	}
+
+	return 0;
 }
 
 int is_tracing_stopped(void)
@@ -1457,9 +1505,9 @@ static int trace_save_cmdline(struct task_struct *tsk)
 	if (!arch_spin_trylock(&trace_cmdline_lock))
 		return 0;
 
-	idx = map_pid_to_cmdline[tsk->pid];
+	idx = savedcmd->map_pid_to_cmdline[tsk->pid];
 	if (idx == NO_CMDLINE_MAP) {
-		idx = (cmdline_idx + 1) % SAVED_CMDLINES;
+		idx = (savedcmd->cmdline_idx + 1) % savedcmd->cmdline_num;
 
 		/*
 		 * Check whether the cmdline buffer at idx has a pid
@@ -1467,17 +1515,17 @@ static int trace_save_cmdline(struct task_struct *tsk)
 		 * need to clear the map_pid_to_cmdline. Otherwise we
 		 * would read the new comm for the old pid.
 		 */
-		pid = map_cmdline_to_pid[idx];
+		pid = savedcmd->map_cmdline_to_pid[idx];
 		if (pid != NO_CMDLINE_MAP)
-			map_pid_to_cmdline[pid] = NO_CMDLINE_MAP;
+			savedcmd->map_pid_to_cmdline[pid] = NO_CMDLINE_MAP;
 
-		map_cmdline_to_pid[idx] = tsk->pid;
-		map_pid_to_cmdline[tsk->pid] = idx;
+		savedcmd->map_cmdline_to_pid[idx] = tsk->pid;
+		savedcmd->map_pid_to_cmdline[tsk->pid] = idx;
 
-		cmdline_idx = idx;
+		savedcmd->cmdline_idx = idx;
 	}
 
-	memcpy(&saved_cmdlines[idx], tsk->comm, TASK_COMM_LEN);
+	set_cmdline(idx, tsk->comm);
 
 	arch_spin_unlock(&trace_cmdline_lock);
 
@@ -1503,9 +1551,9 @@ static void __trace_find_cmdline(int pid, char comm[])
 		return;
 	}
 
-	map = map_pid_to_cmdline[pid];
+	map = savedcmd->map_pid_to_cmdline[pid];
 	if (map != NO_CMDLINE_MAP)
-		strcpy(comm, saved_cmdlines[map]);
+		strcpy(comm, get_saved_cmdlines(map));
 	else
 		strcpy(comm, "<...>");
 }
@@ -3593,6 +3641,7 @@ static const char readme_msg[] =
 	"  trace_options\t\t- Set format or modify how tracing happens\n"
 	"\t\t\t  Disable an option by adding a suffix 'no' to the\n"
 	"\t\t\t  option name\n"
+	"  saved_cmdlines_size\t- echo command number in here to store comm-pid list\n"
 #ifdef CONFIG_DYNAMIC_FTRACE
 	"\n  available_filter_functions - list of functions that can be filtered on\n"
 	"  set_ftrace_filter\t- echo function name in here to only trace these\n"
@@ -3715,7 +3764,8 @@ static void *saved_cmdlines_next(struct seq_file *m, void *v, loff_t *pos)
 
 	(*pos)++;
 
-	for (; ptr < &map_cmdline_to_pid[SAVED_CMDLINES]; ptr++) {
+	for (; ptr < &savedcmd->map_cmdline_to_pid[savedcmd->cmdline_num];
+	     ptr++) {
 		if (*ptr == -1 || *ptr == NO_CMDLINE_MAP)
 			continue;
 
@@ -3733,7 +3783,7 @@ static void *saved_cmdlines_start(struct seq_file *m, loff_t *pos)
 	preempt_disable();
 	arch_spin_lock(&trace_cmdline_lock);
 
-	v = &map_cmdline_to_pid[0];
+	v = &savedcmd->map_cmdline_to_pid[0];
 	while (l <= *pos) {
 		v = saved_cmdlines_next(m, v, &l);
 		if (!v)
@@ -3782,6 +3832,79 @@ static const struct file_operations tracing_saved_cmdlines_fops = {
 };
 
 static ssize_t
+tracing_saved_cmdlines_size_read(struct file *filp, char __user *ubuf,
+				 size_t cnt, loff_t *ppos)
+{
+	char buf[64];
+	int r;
+
+	arch_spin_lock(&trace_cmdline_lock);
+	r = sprintf(buf, "%u\n", savedcmd->cmdline_num);
+	arch_spin_unlock(&trace_cmdline_lock);
+
+	return simple_read_from_buffer(ubuf, cnt, ppos, buf, r);
+}
+
+static void free_saved_cmdlines_buffer(struct saved_cmdlines_buffer *s)
+{
+	kfree(s->saved_cmdlines);
+	kfree(s->map_cmdline_to_pid);
+	kfree(s);
+}
+
+static int tracing_resize_saved_cmdlines(unsigned int val)
+{
+	struct saved_cmdlines_buffer *s, *savedcmd_temp;
+
+	s = kmalloc(sizeof(struct saved_cmdlines_buffer), GFP_KERNEL);
+	if (!s)
+		return -ENOMEM;
+
+	if (allocate_cmdlines_buffer(val, s) < 0) {
+		kfree(s);
+		return -ENOMEM;
+	}
+
+	arch_spin_lock(&trace_cmdline_lock);
+	savedcmd_temp = savedcmd;
+	savedcmd = s;
+	arch_spin_unlock(&trace_cmdline_lock);
+	free_saved_cmdlines_buffer(savedcmd_temp);
+
+	return 0;
+}
+
+static ssize_t
+tracing_saved_cmdlines_size_write(struct file *filp, const char __user *ubuf,
+				  size_t cnt, loff_t *ppos)
+{
+	unsigned long val;
+	int ret;
+
+	ret = kstrtoul_from_user(ubuf, cnt, 10, &val);
+	if (ret)
+		return ret;
+
+	/* must have at least 1 entry or less than PID_MAX_DEFAULT */
+	if (!val || val > PID_MAX_DEFAULT)
+		return -EINVAL;
+
+	ret = tracing_resize_saved_cmdlines((unsigned int)val);
+	if (ret < 0)
+		return ret;
+
+	*ppos += cnt;
+
+	return cnt;
+}
+
+static const struct file_operations tracing_saved_cmdlines_size_fops = {
+	.open		= tracing_open_generic,
+	.read		= tracing_saved_cmdlines_size_read,
+	.write		= tracing_saved_cmdlines_size_write,
+};
+
+static ssize_t
 tracing_set_trace_read(struct file *filp, char __user *ubuf,
 		       size_t cnt, loff_t *ppos)
 {
@@ -6375,6 +6498,9 @@ static __init int tracer_init_debugfs(void)
 	trace_create_file("saved_cmdlines", 0444, d_tracer,
 			NULL, &tracing_saved_cmdlines_fops);
 
+	trace_create_file("saved_cmdlines_size", 0644, d_tracer,
+			  NULL, &tracing_saved_cmdlines_size_fops);
+
 #ifdef CONFIG_DYNAMIC_FTRACE
 	trace_create_file("dyn_ftrace_total_info", 0444, d_tracer,
 			&ftrace_update_tot_cnt, &tracing_dyn_info_fops);
@@ -6611,18 +6737,19 @@ __init static int tracer_alloc_buffers(void)
 	if (!temp_buffer)
 		goto out_free_cpumask;
 
+	if (trace_create_savedcmd() < 0)
+		goto out_free_temp_buffer;
+
 	/* TODO: make the number of buffers hot pluggable with CPUS */
 	if (allocate_trace_buffers(&global_trace, ring_buf_size) < 0) {
 		printk(KERN_ERR "tracer: failed to allocate ring buffer!\n");
 		WARN_ON(1);
-		goto out_free_temp_buffer;
+		goto out_free_savedcmd;
 	}
 
 	if (global_trace.buffer_disabled)
 		tracing_off();
 
-	trace_init_cmdlines();
-
 	if (trace_boot_clock) {
 		ret = tracing_set_clock(&global_trace, trace_boot_clock);
 		if (ret < 0)
@@ -6668,6 +6795,8 @@ __init static int tracer_alloc_buffers(void)
 
 	return 0;
 
+out_free_savedcmd:
+	free_saved_cmdlines_buffer(savedcmd);
 out_free_temp_buffer:
 	ring_buffer_free(temp_buffer);
 out_free_cpumask:

Re: [V4 PATCH ftrace/core] ftrace: Introduce saved_cmdlines_size file

[Steven Rostedt]

Almost there ;-)

On Wed, 04 Jun 2014 16:18:48 +0900
Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com> wrote:


> +
> +static int allocate_cmdlines_buffer(unsigned int val,
> +				    struct saved_cmdlines_buffer *s)
> +{
> +	s->map_cmdline_to_pid = kmalloc(val * sizeof(unsigned), GFP_KERNEL);

s/sizeof(unsigned)/sizeof(*s->map_cmdline_to_pid)/

For the same reason as the memset.

Other than that, I think the rest looks good.

-- Steve

> +	if (!s->map_cmdline_to_pid)
> +		return -ENOMEM;
> +
> +	s->saved_cmdlines = kmalloc(val * TASK_COMM_LEN, GFP_KERNEL);
> +	if (!s->saved_cmdlines) {
> +		kfree(s->map_cmdline_to_pid);
> +		return -ENOMEM;
> +	}
> +
> +	s->cmdline_idx = 0;
> +	s->cmdline_num = val;
> +	memset(&s->map_pid_to_cmdline, NO_CMDLINE_MAP,
> +	       sizeof(s->map_pid_to_cmdline));
> +	memset(s->map_cmdline_to_pid, NO_CMDLINE_MAP,
> +	       val * sizeof(*s->map_cmdline_to_pid));
> +
> +	return 0;
> +}
> +