Messages in this thread Patch in this message | | | Date | Sat, 24 Jul 2021 19:31:45 +0900 | From | Masami Hiramatsu <> | Subject | Re: [PATCH v2 2/2] tracing: Allow execnames to be passed as args for synthetic events |
| |
Hi Steve,
On Thu, 22 Jul 2021 21:24:38 -0400 Steven Rostedt <rostedt@goodmis.org> wrote:
> On Fri, 23 Jul 2021 10:11:33 +0900 > Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > I understand. As far as I can see the code, it looks a bit complicated. > > To simplify it, I need to understand the spec for "hist_field" > > for keys and for vars. And maybe need to split both case. > > I'll give you a hint that took me a bit to figure out. > > 1) The execname is saved at the start of the histogram and not by one > of the ->fn() functions. > > It's saved by hist_trigger_elt_data_init() if the elt_data->comm is > allocated. That function is part of the "tracing_map_ops" which gets > assigned by tracing_map_create() (in tracing_map.c) as the "elt_init" > function, which is called when getting a new elt element by > get_free_elt(). > > 2) That elt_data->comm is only allocated if it finds a "hist_field" > that has HIST_FIELD_FL_EXECNAME flag set. It currently only looks for > that flag in the "keys" fields, which means that .execname is useless > for everything else. This patch changed it to search all hist_fields so > that it can find that flag if a variable has it set (which I added).
Thanks for the hints, but actually, that part looks good to me.
So, what I pointed was the part of update_var_execname(). Below diff is what I intended. This moves HIST_FIELD_FL_EXECNAME setup in the create_hist_field() as same as other flags, and removed the add-hoc update_var_execname() fixup function.
I confirmed it passed the ftracetest trigger testcases and your example code.
Thank you,
--- kernel/trace/trace_events_hist.c | 69 ++++++++++++++------------------ 1 file changed, 31 insertions(+), 38 deletions(-)
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c index 14b840de1326..2fab91a22628 100644 --- a/kernel/trace/trace_events_hist.c +++ b/kernel/trace/trace_events_hist.c @@ -492,6 +492,27 @@ struct snapshot_context { void *key; }; +static const char *no_comm = "(no comm)"; + +static u64 hist_field_execname(struct hist_field *hist_field, + struct tracing_map_elt *elt, + struct trace_buffer *buffer, + struct ring_buffer_event *rbe, + void *event) +{ + struct hist_elt_data *elt_data; + + if (WARN_ON_ONCE(!elt)) + return (u64)(unsigned long)no_comm; + + elt_data = elt->private_data; + + if (WARN_ON_ONCE(!elt_data->comm)) + return (u64)(unsigned long)no_comm; + + return (u64)(unsigned long)(elt_data->comm); +} + static void track_data_free(struct track_data *track_data) { struct hist_elt_data *elt_data; @@ -1682,6 +1703,16 @@ static struct hist_field *create_hist_field(struct hist_trigger_data *hist_data, goto out; } + if ((flags & HIST_FIELD_FL_EXECNAME) && var_name) { + flags |= HIST_FIELD_FL_STRING | HIST_FIELD_FL_VAR; + hist_field->size = MAX_FILTER_STR_VAL; + hist_field->is_signed = 0; + + hist_field->type = "char[]"; + hist_field->fn = hist_field_execname; + goto out; + } + if (WARN_ON_ONCE(!field)) goto out; @@ -3703,41 +3734,6 @@ static int create_val_field(struct hist_trigger_data *hist_data, return __create_val_field(hist_data, val_idx, file, NULL, field_str, 0); } -static const char *no_comm = "(no comm)"; - -static u64 hist_field_execname(struct hist_field *hist_field, - struct tracing_map_elt *elt, - struct trace_buffer *buffer, - struct ring_buffer_event *rbe, - void *event) -{ - struct hist_elt_data *elt_data; - - if (WARN_ON_ONCE(!elt)) - return (u64)(unsigned long)no_comm; - - elt_data = elt->private_data; - - if (WARN_ON_ONCE(!elt_data->comm)) - return (u64)(unsigned long)no_comm; - - return (u64)(unsigned long)(elt_data->comm); -} - -/* Convert a var that points to common_pid.execname to a string */ -static void update_var_execname(struct hist_field *hist_field) -{ - hist_field->flags = HIST_FIELD_FL_STRING | HIST_FIELD_FL_VAR | - HIST_FIELD_FL_EXECNAME; - hist_field->size = MAX_FILTER_STR_VAL; - hist_field->is_signed = 0; - - kfree_const(hist_field->type); - hist_field->type = "char[]"; - - hist_field->fn = hist_field_execname; -} - static int create_var_field(struct hist_trigger_data *hist_data, unsigned int val_idx, struct trace_event_file *file, @@ -3762,9 +3758,6 @@ static int create_var_field(struct hist_trigger_data *hist_data, ret = __create_val_field(hist_data, val_idx, file, var_name, expr_str, flags); - if (!ret && hist_data->fields[val_idx]->flags & HIST_FIELD_FL_EXECNAME) - update_var_execname(hist_data->fields[val_idx]); - if (!ret && hist_data->fields[val_idx]->flags & HIST_FIELD_FL_STRING) hist_data->fields[val_idx]->var_str_idx = hist_data->n_var_str++; -- 2.25.1
-- Masami Hiramatsu <mhiramat@kernel.org>
| |