Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751681AbdFZTJc (ORCPT ); Mon, 26 Jun 2017 15:09:32 -0400 Received: from mail.kernel.org ([198.145.29.99]:47874 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751381AbdFZTJ0 (ORCPT ); Mon, 26 Jun 2017 15:09:26 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 22FA022B5B Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=goodmis.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=rostedt@goodmis.org Date: Mon, 26 Jun 2017 15:09:23 -0400 From: Steven Rostedt To: Joel Fernandes Cc: LKML , Michael Sartain , kernel-team@android.com, Ingo Molnar Subject: Re: [PATCH v4 1/3] tracing: Add support for recording tgid of tasks Message-ID: <20170626150923.39341c03@gandalf.local.home> In-Reply-To: References: <20170626053844.5746-1-joelaf@google.com> <20170626053844.5746-2-joelaf@google.com> <20170626125226.05c9a80e@gandalf.local.home> X-Mailer: Claws Mail 3.14.0 (GTK+ 2.24.31; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9513 Lines: 256 On Mon, 26 Jun 2017 12:02:12 -0700 Joel Fernandes wrote: > On Mon, Jun 26, 2017 at 9:52 AM, Steven Rostedt wrote: > > On Sun, 25 Jun 2017 22:38:42 -0700 > > Joel Fernandes wrote: > > > >> +int *tgid_map; > >> + > >> +void tracing_alloc_tgid_map(void) > >> +{ > >> + if (tgid_map) > >> + return; > >> + > >> + tgid_map = kzalloc((PID_MAX_DEFAULT + 1) * sizeof(*tgid_map), > >> + GFP_KERNEL); > >> + WARN_ONCE(!tgid_map, "Allocation of tgid_map failed\n"); > > > > This needs to return a value. If it fails to allocate, then we must not > > set the bit to do the recording. More below about why. > > > >> +} > >> + > >> #define SAVED_CMDLINES_DEFAULT 128 > >> #define NO_CMDLINE_MAP UINT_MAX > >> static arch_spinlock_t trace_cmdline_lock = __ARCH_SPIN_LOCK_UNLOCKED; > >> @@ -1722,7 +1734,7 @@ struct saved_cmdlines_buffer { > >> static struct saved_cmdlines_buffer *savedcmd; > >> > >> /* temporary disable recording */ > >> -static atomic_t trace_record_cmdline_disabled __read_mostly; > >> +static atomic_t trace_record_taskinfo_disabled __read_mostly; > >> > >> static inline char *get_saved_cmdlines(int idx) > >> { > >> @@ -1992,16 +2004,87 @@ void trace_find_cmdline(int pid, char comm[]) > >> preempt_enable(); > >> } > >> > >> -void tracing_record_cmdline(struct task_struct *tsk) > >> +int trace_find_tgid(int pid) > >> +{ > >> + if (unlikely(!tgid_map || !pid || pid > PID_MAX_DEFAULT)) > >> + return 0; > >> + > >> + return tgid_map[pid]; > >> +} > >> + > >> +static int trace_save_tgid(struct task_struct *tsk) > >> +{ > >> + if (unlikely(!tgid_map || !tsk->pid || tsk->pid > PID_MAX_DEFAULT)) > >> + return 0; > > > > If we failed to allocate, this will always return 0. > > > >> + > >> + tgid_map[tsk->pid] = tsk->tgid; > >> + return 1; > >> +} > >> + > >> +static bool tracing_record_taskinfo_skip(int flags) > >> +{ > >> + if (unlikely(!(flags & (TRACE_RECORD_CMDLINE | TRACE_RECORD_TGID)))) > >> + return true; > >> + if (atomic_read(&trace_record_taskinfo_disabled) || !tracing_is_on()) > >> + return true; > >> + if (!__this_cpu_read(trace_taskinfo_save)) > >> + return true; > >> + return false; > >> +} > >> + > >> +/** > >> + * tracing_record_taskinfo - record the task info of a task > >> + * > >> + * @task - task to record > >> + * @flags - TRACE_RECORD_CMDLINE for recording comm > >> + * - TRACE_RECORD_TGID for recording tgid > >> + */ > >> +void tracing_record_taskinfo(struct task_struct *task, int flags) > >> +{ > >> + if (tracing_record_taskinfo_skip(flags)) > >> + return; > >> + if ((flags & TRACE_RECORD_CMDLINE) && !trace_save_cmdline(task)) > >> + return; > >> + if ((flags & TRACE_RECORD_TGID) && !trace_save_tgid(task)) > > > > With a failed allocation, this will return here. > > > >> + return; > >> + > >> + __this_cpu_write(trace_taskinfo_save, false); > > > > The above will never be cleared, and this callback will constantly be > > called, slowing down the tracer. > > > > > >> +} > >> + > >> +/** > >> + * tracing_record_taskinfo_sched_switch - record task info for sched_switch > >> + * > >> + * @prev - previous task during sched_switch > >> + * @next - next task during sched_switch > >> + * @flags - TRACE_RECORD_CMDLINE for recording comm > >> + * TRACE_RECORD_TGID for recording tgid > >> + */ > >> +void tracing_record_taskinfo_sched_switch(struct task_struct *prev, > >> + struct task_struct *next, int flags) > >> { > >> - if (atomic_read(&trace_record_cmdline_disabled) || !tracing_is_on()) > >> + if (tracing_record_taskinfo_skip(flags)) > >> + return; > >> + > >> + if ((flags & TRACE_RECORD_CMDLINE) && > >> + (!trace_save_cmdline(prev) || !trace_save_cmdline(next))) > >> return; > >> > >> - if (!__this_cpu_read(trace_cmdline_save)) > >> + if ((flags & TRACE_RECORD_TGID) && > >> + (!trace_save_tgid(prev) || !trace_save_tgid(next))) > > > > On failed allocation, this will return here. > > > >> return; > >> > >> - if (trace_save_cmdline(tsk)) > >> - __this_cpu_write(trace_cmdline_save, false); > >> + __this_cpu_write(trace_taskinfo_save, false); > > > > This will not be cleared. > > > >> +} > >> + > >> +/* Helpers to record a specific task information */ > >> +void tracing_record_cmdline(struct task_struct *task) > >> +{ > >> + tracing_record_taskinfo(task, TRACE_RECORD_CMDLINE); > >> +} > >> + > >> +void tracing_record_tgid(struct task_struct *task) > >> +{ > >> + tracing_record_taskinfo(task, TRACE_RECORD_TGID); > >> } > >> > >> /* > >> @@ -3146,7 +3229,7 @@ static void *s_start(struct seq_file *m, loff_t *pos) > >> #endif > >> > >> if (!iter->snapshot) > >> - atomic_inc(&trace_record_cmdline_disabled); > >> + atomic_inc(&trace_record_taskinfo_disabled); > >> > >> if (*pos != iter->pos) { > >> iter->ent = NULL; > >> @@ -3191,7 +3274,7 @@ static void s_stop(struct seq_file *m, void *p) > >> #endif > >> > >> if (!iter->snapshot) > >> - atomic_dec(&trace_record_cmdline_disabled); > >> + atomic_dec(&trace_record_taskinfo_disabled); > >> > >> trace_access_unlock(iter->cpu_file); > >> trace_event_read_unlock(); > >> @@ -4238,6 +4321,9 @@ int set_tracer_flag(struct trace_array *tr, unsigned int mask, int enabled) > >> if (mask == TRACE_ITER_RECORD_CMD) > >> trace_event_enable_cmd_record(enabled); > >> > >> + if (mask == TRACE_ITER_RECORD_TGID) > >> + trace_event_enable_tgid_record(enabled); > >> + > >> if (mask == TRACE_ITER_EVENT_FORK) > >> trace_event_follow_fork(tr, enabled); > >> > >> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h > >> index 39fd77330aab..90318b016fd9 100644 > >> --- a/kernel/trace/trace.h > >> +++ b/kernel/trace/trace.h > >> @@ -635,8 +635,13 @@ void trace_graph_return(struct ftrace_graph_ret *trace); > >> int trace_graph_entry(struct ftrace_graph_ent *trace); > >> void set_graph_array(struct trace_array *tr); > >> > >> +extern int *tgid_map; > >> +void tracing_alloc_tgid_map(void); > >> void tracing_start_cmdline_record(void); > >> void tracing_stop_cmdline_record(void); > >> +void tracing_start_tgid_record(void); > >> +void tracing_stop_tgid_record(void); > >> + > >> int register_tracer(struct tracer *type); > >> int is_tracing_stopped(void); > >> > >> @@ -697,6 +702,7 @@ static inline void __trace_stack(struct trace_array *tr, unsigned long flags, > >> extern u64 ftrace_now(int cpu); > >> > >> extern void trace_find_cmdline(int pid, char comm[]); > >> +extern int trace_find_tgid(int pid); > >> extern void trace_event_follow_fork(struct trace_array *tr, bool enable); > >> > >> #ifdef CONFIG_DYNAMIC_FTRACE > >> @@ -1107,6 +1113,7 @@ extern int trace_get_user(struct trace_parser *parser, const char __user *ubuf, > >> C(CONTEXT_INFO, "context-info"), /* Print pid/cpu/time */ \ > >> C(LATENCY_FMT, "latency-format"), \ > >> C(RECORD_CMD, "record-cmd"), \ > >> + C(RECORD_TGID, "record-tgid"), \ > >> C(OVERWRITE, "overwrite"), \ > >> C(STOP_ON_FREE, "disable_on_free"), \ > >> C(IRQ_INFO, "irq-info"), \ > >> @@ -1423,6 +1430,8 @@ struct ftrace_event_field * > >> trace_find_event_field(struct trace_event_call *call, char *name); > >> > >> extern void trace_event_enable_cmd_record(bool enable); > >> +extern void trace_event_enable_tgid_record(bool enable); > >> + > >> extern int event_trace_add_tracer(struct dentry *parent, struct trace_array *tr); > >> extern int event_trace_del_tracer(struct trace_array *tr); > >> > >> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c > >> index e7973e10398c..240c6df95ea6 100644 > >> --- a/kernel/trace/trace_events.c > >> +++ b/kernel/trace/trace_events.c > >> @@ -343,6 +343,28 @@ void trace_event_enable_cmd_record(bool enable) > >> mutex_unlock(&event_mutex); > >> } > >> > >> +void trace_event_enable_tgid_record(bool enable) > > > > This should return a value. > > > >> +{ > >> + struct trace_event_file *file; > >> + struct trace_array *tr; > >> + > >> + mutex_lock(&event_mutex); > >> + do_for_each_event_file(tr, file) { > >> + if (!(file->flags & EVENT_FILE_FL_ENABLED)) > >> + continue; > >> + > >> + if (enable) { > >> + tracing_start_tgid_record(); > > > > If we fail to start, the bit should not be set, and we should return > > failure. Note, it can only fail on the first try, as once it is > > allocated, you don't need to worry about it failing. Thus, if it fails, > > break out of the loop now and return failure. > > > > That seems Ok with me to do, I think a valid point. > > I think that I should do it in the second call to > tracing_start_tgid_record too then (__ftrace_event_enable_disable) to > error out if the allocation fails. > > While going this code I again, I noticed another potential issue in > __ftrace_event_enable_disable set_tracer