Messages in this thread | | | Date | Sun, 25 Jul 2021 11:18:30 +0900 | From | Masami Hiramatsu <> | Subject | Re: [PATCH v2 2/2] tracing: Allow execnames to be passed as args for synthetic events |
| |
Hi,
On Sat, 24 Jul 2021 19:31:45 +0900 Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 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, >
I found a bug in this change.
[..] > @@ -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;
Here, we don't need to check 'var_name' and remove HIST_FIELD_FL_VAR, since it must be set in the flag.
if (flags & HIST_FIELD_FL_EXECNAME) { flags |= HIST_FIELD_FL_STRING;
> + hist_field->size = MAX_FILTER_STR_VAL; > + hist_field->is_signed = 0; > + > + hist_field->type = "char[]"; > + hist_field->fn = hist_field_execname; > + goto out; > + } > +
Thank you,
-- Masami Hiramatsu <mhiramat@kernel.org>
| |