by Steven Rostedt

[permalink] [raw]

Subject: Re: [PATCH v4 1/3] tracing: Add support for recording tgid of tasks

On Mon, 26 Jun 2017 12:10:04 -0700
Joel Fernandes <[email protected]> wrote:

> > 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
>
> Sorry I hit send too soon!
>
> I was saying, While going through this code again I noticed another
> potential issue in __ftrace_event_enable_disable in existing code:
>
> Here we do:
> ret = call->class->reg(call, TRACE_REG_REGISTER, file);
>
> But in the error handling path we only do a
> tracing_stop_cmdline_record() and not clear the
> EVENT_FILE_FL_RECORDED_CMD_BIT flag. Is that not an issue? I am
> guessing because enabling of the event itself will fail, that's Ok.
> But just to keep it consistent, I am thinking if we should just clear
> the bit here.

Yeah, we should probably add some better error handling for failed
setting of events. But the tgid-map is low hanging fruit to get
right ;-)

-- Steve

2017-06-27 03:16:29

by Joel Fernandes

[permalink] [raw]

Subject: Re: [PATCH v4 3/3] tracing/ftrace: Add support to record and display tgid

Hi Steven,

Please drop this patch (3/3) for now because the new allocation
rearrangement we made to (1/3) will not work with this patch because
flag_changed is called before the allocation failure case. Also TGIDs
for event tracing is the main use case for my series, the function
tracer isn't the main use case so I can post a proper version of this
patch later once we get the main pieces in.

1/3 and 2/3 are still good to go, following are you patchwork links to them

1/3: https://patchwork.kernel.org/patch/9810685/
2/3: https://patchwork.kernel.org/patch/9808641/

Incase its confusing, please let me know and I can just send you both
these patches again.

Thanks,
Joel

On Sun, Jun 25, 2017 at 10:38 PM, Joel Fernandes <[email protected]> wrote:
> Make function tracer able to record tgid if/when record-tgid is enabled.
>
> Cc: [email protected]
> Cc: Steven Rostedt <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Tested-by: Michael Sartain <[email protected]>
> Signed-off-by: Joel Fernandes <[email protected]>
> ---
> kernel/trace/trace_functions.c | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
> index a3bddbfd0874..d6bdc38ab273 100644
> --- a/kernel/trace/trace_functions.c
> +++ b/kernel/trace/trace_functions.c
> @@ -27,6 +27,7 @@ static void
> function_stack_trace_call(unsigned long ip, unsigned long parent_ip,
> struct ftrace_ops *op, struct pt_regs *pt_regs);
> static struct tracer_flags func_flags;
> +static bool tgid_recorded;
>
> /* Our option */
> enum {
> @@ -104,6 +105,11 @@ static int function_trace_init(struct trace_array *tr)
> put_cpu();
>
> tracing_start_cmdline_record();
> +
> + if (tr->trace_flags & TRACE_ITER_RECORD_TGID) {
> + tgid_recorded = true;
> + tracing_start_tgid_record();
> + }
> tracing_start_function_trace(tr);
> return 0;
> }
> @@ -112,6 +118,10 @@ static void function_trace_reset(struct trace_array *tr)
> {
> tracing_stop_function_trace(tr);
> tracing_stop_cmdline_record();
> + if (tgid_recorded) {
> + tracing_stop_tgid_record();
> + tgid_recorded = false;
> + }
> ftrace_reset_array_ops(tr);
> }
>
> @@ -252,6 +262,21 @@ func_set_flag(struct trace_array *tr, u32 old_flags, u32 bit, int set)
> return 0;
> }
>
> +static int
> +func_tracer_flag_changed(struct trace_array *tr, unsigned int mask,
> + int enabled)
> +{
> + if (mask == TRACE_ITER_RECORD_TGID) {
> + tgid_recorded = !!enabled;
> + if (enabled)
> + tracing_start_tgid_record();
> + else
> + tracing_stop_tgid_record();
> + }
> +
> + return 0;
> +}
> +
> static struct tracer function_trace __tracer_data =
> {
> .name = "function",
> @@ -260,6 +285,7 @@ static struct tracer function_trace __tracer_data =
> .start = function_trace_start,
> .flags = &func_flags,
> .set_flag = func_set_flag,
> + .flag_changed = func_tracer_flag_changed,
> .allow_instances = true,
> #ifdef CONFIG_FTRACE_SELFTEST
> .selftest = trace_selftest_startup_function,
> --
> 2.13.1.611.g7e3b11ae1-goog
>