From: Namhyung Kim <namhyung@kernel.org>
To: Tom Zanussi <zanussi@kernel.org>
Cc: rostedt@goodmis.org, tglx@linutronix.de, mhiramat@kernel.org,
vedang.patel@intel.com, bigeasy@linutronix.de,
joel@joelfernandes.org, mathieu.desnoyers@efficios.com,
julia@ni.com, linux-kernel@vger.kernel.org,
linux-rt-users@vger.kernel.org, kernel-team@lge.com
Subject: Re: [PATCH v7 05/16] tracing: Generalize hist trigger onmax and save action
Date: Tue, 4 Dec 2018 16:25:49 +0900 [thread overview]
Message-ID: <20181204072549.GA16733@sejong> (raw)
In-Reply-To: <1543875722.2018.22.camel@kernel.org>
On Mon, Dec 03, 2018 at 04:22:02PM -0600, Tom Zanussi wrote:
> Hi Namhyung,
>
> On Fri, 2018-11-23 at 11:50 +0900, Namhyung Kim wrote:
> > Hi Tom,
> >
> > On Wed, Nov 14, 2018 at 02:18:02PM -0600, Tom Zanussi wrote:
> > > From: Tom Zanussi <tom.zanussi@linux.intel.com>
> > >
>
> [snip]
>
> > >
> enum handler_id {
> > > HANDLER_ONMATCH = 1,
> > > HANDLER_ONMAX,
> > > @@ -349,14 +358,18 @@ struct action_data {
> > >
> > > struct {
> > > char *var_str;
> > > - unsigned int max_var_ref_idx;
> > > - struct hist_field *max_var;
> > > - struct hist_field *var;
> > > - } onmax;
> > > + struct hist_field *var_ref;
> > > + unsigned int var_ref_idx;
> >
> > I have a question. It's confusing for me there are many indexes for
> > a
> > variable (ref). The hist_field already has var.idx, var_idx and
> > var_ref_idx in it. But you also added an external var_ref_idx along
> > with the var_ref. Also I see another var_ref_idx in the action data.
> > Is all that really needed? Could you please add some comment then?
> >
>
> Below is a patch with some comments I'll merge into the next version
> that I hope will help make things more clear. Basically, the
> hist_field.var_idx isn't used so I've removed it and therefore that
Thanks!
> source of confusion, while var.idx is the variable's unique 'handle' in
> the tracing_map, used when getting and setting the variable. And then
> there are the several versions of var_ref_idx used for different
> purposes depending on the context, but all of them are indices into the
> array of variable values collected when a trigger is hit. For example,
So IIUC field->var_ref_idx is an index to the val_ref_vals array,
right? Then if we keep the all hist_fields we don't need to have a
separate var_ref_idx IMHO.
> the var_ref_idx defined inside track_data is the index that points to
> the tracked var value, which the action can use directly, and the
I guess the track_data.var_ref_idx is always same as the
track_data.track_var.var_ref_idx, no? If so we can get rid of it.
> var_ref_idx alongside the synth fields in action_data is the index of
> the first param used when generating a synthetic event, and so on.
For synth event, we have hist_data->synth_var_refs[] but it's not
passed to trace_synth() so no way to know original var_ref_idx and I'm
ok with having action_data.var_ref_idx.
But I don't see where hist_data->synth_var_refs is used other than
find_var_ref(). And for that purpose, I guess it's more efficient to
use hist_data->var_refs[] so that we can remove synth_var_refs.
>
> Tom
>
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 818944391d97..5310ef73f023 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -39,6 +39,16 @@ enum field_op_id {
> FIELD_OP_UNARY_MINUS,
> };
>
> +/*
> + * A hist_var (histogram variable) contains variable information for
> + * hist_fields having the HIST_FIELD_FL_VAR or HIST_FIELD_FL_VAR_REF
> + * flag set. A hist_var has a variable name e.g. ts0, and is
> + * associated with a given histogram trigger, as specified by
> + * hist_data. The hist_var idx is the unique index assigned to the
> + * variable by the hist trigger's tracing_map. The idx is what is
> + * used to set a variable's value and, by a variable reference, to
> + * retrieve it.
> + */
> struct hist_var {
> char *name;
> struct hist_trigger_data *hist_data;
> @@ -60,7 +70,15 @@ struct hist_field {
> char *system;
> char *event_name;
> char *name;
> - unsigned int var_idx;
> +
> + /*
> + * When a histogram trigger is hit, if it has any references
> + * to variables, the values of those variables are collected
> + * into a var_ref_vals array by resolve_var_refs(). The
> + * current value of each variable is read from the tracing_map
> + * using the hist field's hist_var.idx and entered into the
> + * var_ref_idx entry i.e. var_ref_vals[var_ref_idx].
> + */
> unsigned int var_ref_idx;
> bool read_once;
> };
> @@ -350,6 +368,14 @@ struct action_data {
> unsigned int n_params;
> char *params[SYNTH_FIELDS_MAX];
>
> + /*
> + * When a histogram trigger is hit, the values of any
> + * references to variables, including variables being passed
> + * as parameters to synthetic events, are collected into a
> + * var_ref_vals array. This var_ref_idx is the index of the
> + * first param in the array to be passed to the synthetic
> + * event invocation.
> + */
> unsigned int var_ref_idx;
> struct synth_event *synth_event;
> bool use_trace_keyword;
> @@ -362,10 +388,29 @@ struct action_data {
> } match_data;
>
> struct {
> + /*
> + * var_str and var_ref refer to the variable
> + * being tracked e.g onmax($var).
> + */
> char *var_str;
Can it be different from var_ref->var.name?
Thanks,
Namhyung
> struct hist_field *var_ref;
> +
> + /*
> + * When a histogram trigger is hit, the values
> + * of any references to variables, including
> + * variables being tracked e.g. onmax($var),
> + * are collected into a var_ref_vals array.
> + * This var_ref_idx is the index of the
> + * tracked var value in var_ref_vals, which
> + * the action can make use of directly.
> + */
> unsigned int var_ref_idx;
>
> + /*
> + * This track_var contains the 'invisible'
> + * tracking variable created to keep the
> + * current e.g. max value.
> + */
> struct hist_field *track_var;
>
> check_track_val_fn_t check_val;
next prev parent reply other threads:[~2018-12-04 7:25 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-14 20:17 [PATCH v7 00/16] tracing: Hist trigger snapshot and onchange additions Tom Zanussi
2018-11-14 20:17 ` [PATCH v7 01/16] tracing: Refactor hist trigger action code Tom Zanussi
2018-11-14 20:17 ` [PATCH v7 02/16] tracing: Make hist trigger Documentation better reflect actions/handlers Tom Zanussi
2018-11-14 20:18 ` [PATCH v7 03/16] tracing: Add hist trigger handler.action documentation to README Tom Zanussi
2018-11-14 20:18 ` [PATCH v7 04/16] tracing: Split up onmatch action data Tom Zanussi
2018-11-14 20:18 ` [PATCH v7 05/16] tracing: Generalize hist trigger onmax and save action Tom Zanussi
2018-11-23 2:50 ` Namhyung Kim
2018-12-03 22:22 ` Tom Zanussi
2018-12-04 7:25 ` Namhyung Kim [this message]
2018-12-04 19:53 ` Tom Zanussi
2018-11-23 7:01 ` Namhyung Kim
2018-11-27 22:48 ` Tom Zanussi
2018-11-14 20:18 ` [PATCH v7 06/16] tracing: Add conditional snapshot Tom Zanussi
2018-11-14 20:18 ` [PATCH v7 07/16] tracing: Add hist trigger snapshot() action Tom Zanussi
2018-11-14 20:18 ` [PATCH v7 08/16] tracing: Add hist trigger snapshot() action Documentation Tom Zanussi
2018-11-14 20:18 ` [PATCH v7 09/16] tracing: Add hist trigger snapshot() action test case Tom Zanussi
2018-11-26 13:03 ` Masami Hiramatsu
2018-11-27 22:53 ` Tom Zanussi
2018-11-28 2:15 ` Masami Hiramatsu
2018-11-29 1:12 ` Tom Zanussi
2018-12-04 19:59 ` Tom Zanussi
2018-11-14 20:18 ` [PATCH v7 10/16] tracing: Add hist trigger onchange() handler Tom Zanussi
2018-11-14 20:18 ` [PATCH v7 11/16] tracing: Add hist trigger onchange() handler Documentation Tom Zanussi
2018-11-14 20:18 ` [PATCH v7 12/16] tracing: Add hist trigger onchange() handler test case Tom Zanussi
2018-11-14 20:18 ` [PATCH v7 13/16] tracing: Add alternative synthetic event trace action syntax Tom Zanussi
2018-11-14 20:18 ` [PATCH v7 14/16] tracing: Add alternative synthetic event trace action test case Tom Zanussi
2018-11-14 20:18 ` [PATCH v7 15/16] tracing: Add hist trigger action 'expected fail' " Tom Zanussi
2018-11-14 20:18 ` [PATCH v7 16/16] tracing: Add SPDX license GPL-2.0 license identifier to inter-event testcases Tom Zanussi
2018-11-26 14:09 ` [PATCH v7 00/16] tracing: Hist trigger snapshot and onchange additions Masami Hiramatsu
2018-11-26 21:21 ` Tom Zanussi
2018-11-29 13:52 ` Masami Hiramatsu
2018-11-29 14:54 ` Masami Hiramatsu
2018-11-29 15:07 ` [PATCH] sefltests/ftrace: Use /bin/echo for output with options Masami Hiramatsu
2018-11-29 16:32 ` Steven Rostedt
2018-11-29 16:43 ` Tom Zanussi
2018-11-29 16:49 ` Steven Rostedt
2019-03-04 20:09 ` Tom Zanussi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20181204072549.GA16733@sejong \
--to=namhyung@kernel.org \
--cc=bigeasy@linutronix.de \
--cc=joel@joelfernandes.org \
--cc=julia@ni.com \
--cc=kernel-team@lge.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rt-users@vger.kernel.org \
--cc=mathieu.desnoyers@efficios.com \
--cc=mhiramat@kernel.org \
--cc=rostedt@goodmis.org \
--cc=tglx@linutronix.de \
--cc=vedang.patel@intel.com \
--cc=zanussi@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).