Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754859AbaFDDPK (ORCPT ); Tue, 3 Jun 2014 23:15:10 -0400 Received: from mail7.hitachi.co.jp ([133.145.228.42]:48270 "EHLO mail7.hitachi.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754581AbaFDDPI (ORCPT ); Tue, 3 Jun 2014 23:15:08 -0400 X-AuditID: 85900ec0-d0927b9000001514-5f-538e8f3918a1 Message-ID: <538E8F38.2070706@hitachi.com> Date: Wed, 04 Jun 2014 12:15:04 +0900 From: Yoshihiro YUNOMAE User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120604 Thunderbird/13.0 MIME-Version: 1.0 To: Steven Rostedt Cc: linux-kernel@vger.kernel.org, Hidehiro Kawai , Frederic Weisbecker , Masami Hiramatsu , Ingo Molnar , yrl.pp-manager.tt@hitachi.com Subject: Re: Re: [PATCH ftrace/core 2/2] ftrace: Introduce saved_cmdlines_size file References: <20140603042800.27308.48050.stgit@yunodevel> <20140603042805.27308.92080.stgit@yunodevel> <20140603203052.63bb819a@gandalf.local.home> In-Reply-To: <20140603203052.63bb819a@gandalf.local.home> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-Brightmail-Tracker: AAAAAA== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/