Messages in this thread | | | Date | Mon, 26 Jun 2017 15:12:52 -0400 | From | Steven Rostedt <> | Subject | Re: [PATCH v4 1/3] tracing: Add support for recording tgid of tasks |
| |
On Mon, 26 Jun 2017 12:02:12 -0700 Joel Fernandes <joelaf@google.com> wrote:
> >> 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
Thinking about this more. Just allocate the array as soon as the option is enabled, regardless if an trace event is set. That will make it a lot simpler. If it fails to allocate, you can simply bail out with -ENOMEM, and the setting of the option will return that. Then we can even remove the WARN_ONCE() at allocation failure, as the user will know what happened.
-- Steve
| |