Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751668AbdFZTCU (ORCPT ); Mon, 26 Jun 2017 15:02:20 -0400 Received: from mail-ot0-f178.google.com ([74.125.82.178]:36506 "EHLO mail-ot0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751376AbdFZTCO (ORCPT ); Mon, 26 Jun 2017 15:02:14 -0400 MIME-Version: 1.0 In-Reply-To: <20170626125226.05c9a80e@gandalf.local.home> References: <20170626053844.5746-1-joelaf@google.com> <20170626053844.5746-2-joelaf@google.com> <20170626125226.05c9a80e@gandalf.local.home> From: Joel Fernandes Date: Mon, 26 Jun 2017 12:02:12 -0700 Message-ID: Subject: Re: [PATCH v4 1/3] tracing: Add support for recording tgid of tasks To: Steven Rostedt Cc: LKML , Michael Sartain , kernel-team@android.com, Ingo Molnar Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8887 Lines: 251 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