Hi Namhyung,

On Tue, 2018-12-04 at 16:25 +0900, Namhyung Kim wrote:

On Mon, Dec 03, 2018 at 04:22:02PM -0600, Tom Zanussi wrote:

quoted

Hi Namhyung,

On Fri, 2018-11-23 at 11:50 +0900, Namhyung Kim wrote:

quoted

Hi Tom,

On Wed, Nov 14, 2018 at 02:18:02PM -0600, Tom Zanussi wrote:

quoted

From: Tom Zanussi <redacted>

[snip]

quoted

quoted

 enum handler_id {

quoted

quoted

 	HANDLER_ONMATCH = 1,
 	HANDLER_ONMAX,

@@ -349,14 +358,18 @@ struct action_data {
 
 		struct {
 			char			*var_str;
-			unsigned int		max_var_re

f_idx;
-			struct hist_field	*max_var;
-			struct hist_field	*var;
-		} onmax;
+			struct hist_field	*var_ref;
+			unsigned int		var_ref_id
x;

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!

quoted

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.

hist_field_var_ref() needs to be be able to retrieve var_ref_idx given
the field alone, so I'm not sure this can be removed.

quoted

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.

Yes, you're right, that seems to be redundant in the code, will remove
it.

quoted

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.

It's also used to destroy hist_fields in destroy_synth_var_refs(), but
this points out something that should be cleaned up too - because of
the way the code developed over time, we now have separate sets of
fields like this that should be unified - I'll add some patches to do
that.  And that will get rid of the separate synth_var_refs and use
hist_data->var_refs, which as you correctly point out will be more
efficient.

quoted

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?

Well, it's slightly different in that it still has the '$' prefix -
it's used for printing the action, but there's no reason not to get rid
of it and use var_ref instead.

Thanks for the useful comments,

Tom