Hi Namhyung,
On Mon, 2017-07-24 at 00:12 +0900, Namhyung Kim wrote:
> On Mon, Jun 26, 2017 at 05:49:24PM -0500, Tom Zanussi wrote:
> > Add an 'onmatch(matching.event).<synthetic_event_name>(param list)'
> > hist trigger action which is invoked with the set of variables or
> > event fields named in the 'param list'. The result is the generation
> > of a synthetic event that consists of the values contained in those
> > variables and/or fields at the time the invoking event was hit.
> >
> > As an example the below defines a simple synthetic event using a
> > variable defined on the sched_wakeup_new event, and shows the event
> > definition with unresolved fields, since the sched_wakeup_new event
> > with the testpid variable hasn't been defined yet:
> >
> > # echo 'wakeup_new_test pid_t pid; int prio' >> \
> > /sys/kernel/debug/tracing/synthetic_events
> >
> > # cat /sys/kernel/debug/tracing/synthetic_events
> > wakeup_new_test pid_t pid; int prio
> >
> > The following hist trigger both defines a testpid variable and
> > specifies an onmatch() trace action that uses that variable along with
> > a non-variable field to generate a wakeup_new_test synthetic event
> > whenever a sched_wakeup_new event occurs, which because of the 'if
> > comm == "cyclictest"' filter only happens when the executable is
> > cyclictest:
> >
> > # echo 'hist:keys=testpid=pid:\
> > onmatch(sched.sched_wakeup_new).wakeup_new_test($testpid, prio) \
> > if comm=="cyclictest"' >> \
> > /sys/kernel/debug/tracing/events/sched/sched_wakeup_new/trigger
> >
> > Creating and displaying a histogram based on those events is now just
> > a matter of using the fields and new synthetic event in the
> > tracing/events/synthetic directory, as usual:
> >
> > # echo 'hist:keys=pid,prio:sort=pid,prio' >> \
> > /sys/kernel/debug/tracing/events/synthetic/wakeup_new_test/trigger
> >
> > Signed-off-by: Tom Zanussi <tom.zanu...@linux.intel.com>
> > ---
>
> This patch is also big and it'd be better splitting - add type to
> hist_field, add variable handling and implement onmatch() action.
>
Yeah, makes sense, will do.
>
> > kernel/trace/trace_events_hist.c | 955
> > ++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 940 insertions(+), 15 deletions(-)
> >
> > diff --git a/kernel/trace/trace_events_hist.c
> > b/kernel/trace/trace_events_hist.c
> > index e11b3a3..b1f859c 100644
> > --- a/kernel/trace/trace_events_hist.c
> > +++ b/kernel/trace/trace_events_hist.c
> > @@ -59,6 +59,7 @@ struct hist_field {
> > unsigned int size;
> > unsigned int offset;
> > unsigned int is_signed;
> > + const char *type;
> > struct hist_field *operands[HIST_FIELD_OPERANDS_MAX];
> > struct hist_trigger_data *hist_data;
> > struct hist_var var;
> > @@ -243,6 +244,16 @@ struct hist_trigger_attrs {
> > unsigned int n_actions;
> > };
> >
> > +struct field_var {
> > + struct hist_field *var;
> > + struct hist_field *val;
> > +};
> > +
> > +struct field_var_hist {
> > + struct hist_trigger_data *hist_data;
> > + char *cmd;
> > +};
> > +
> > struct hist_trigger_data {
> > struct hist_field *fields[HIST_FIELDS_MAX];
> > unsigned int n_vals;
> > @@ -263,6 +274,14 @@ struct hist_trigger_data {
> >
> > struct action_data *actions[HIST_ACTIONS_MAX];
> > unsigned int n_actions;
> > +
> > + struct hist_field *synth_var_refs[SYNTH_FIELDS_MAX];
> > + unsigned int n_synth_var_refs;
> > + struct field_var *field_vars[SYNTH_FIELDS_MAX];
> > + unsigned int n_field_vars;
> > + unsigned int n_field_var_str;
> > + struct field_var_hist *field_var_hists[SYNTH_FIELDS_MAX];
> > + unsigned int n_field_var_hists;
> > };
> >
> > struct synth_field {
> > @@ -291,7 +310,14 @@ typedef void (*action_fn_t) (struct hist_trigger_data
> > *hist_data,
> >
> > struct action_data {
> > action_fn_t fn;
> > + unsigned int n_params;
> > + char *params[SYNTH_FIELDS_MAX];
> > +
> > unsigned int var_ref_idx;
> > + char *match_event;
> > + char *match_event_system;
> > + char *synth_event_name;
> > + struct synth_event *synth_event;
> > };
> >
> > static LIST_HEAD(synth_event_list);
> > @@ -802,6 +828,50 @@ static struct synth_event *alloc_synth_event(char
> > *event_name, int n_fields,
> > return event;
> > }
> >
> > +static void action_trace(struct hist_trigger_data *hist_data,
> > + struct tracing_map_elt *elt, void *rec,
> > + struct ring_buffer_event *rbe,
> > + struct action_data *data, u64 *var_ref_vals)
> > +{
> > + struct synth_event *event = data->synth_event;
> > +
> > + trace_synth(event, var_ref_vals, data->var_ref_idx);
> > +}
> > +
> > +static bool check_hist_action_refs(struct hist_trigger_data *hist_data,
> > + struct synth_event *event)
> > +{
> > + unsigned int i;
> > +
> > + for (i = 0; i < hist_data->n_actions; i++) {
> > + struct action_data *data = hist_data->actions[i];
> > +
> > + if (data->fn == action_trace && data->synth_event == event)
> > + return true;
> > + }
> > +
> > + return false;
> > +}
> > +
> > +static LIST_HEAD(hist_action_list);
> > +static LIST_HEAD(hist_var_list);
> > +
> > +struct hist_var_data {
> > + struct list_head list;
> > + struct hist_trigger_data *hist_data;
> > +};
> > +
> > +static bool check_synth_action_refs(struct synth_event *event)
> > +{
> > + struct hist_var_data *var_data;
> > +
> > + list_for_each_entry(var_data, &hist_action_list, list)
> > + if (check_hist_action_refs(var_data->hist_data, event))
> > + return true;
> > +
> > + return false;
> > +}
> > +
> > static int create_synth_event(int argc, char **argv)
> > {
> > struct synth_field *fields[SYNTH_FIELDS_MAX];
> > @@ -832,15 +902,17 @@ static int create_synth_event(int argc, char **argv)
> > event = find_synth_event(name);
> > if (event) {
> > if (delete_event) {
> > + if (check_synth_action_refs(event)) {
> > + ret = -EBUSY;
> > + goto out;
> > + }
> > remove_synth_event(event);
> > goto err;
> > } else
> > ret = -EEXIST;
> > goto out;
> > - } else if (delete_event) {
> > - ret = -EINVAL;
> > + } else if (delete_event)
> > goto out;
> > - }
>
> Why is it changed?
>
Yeah, shouldn't have been, thanks for pointing this out.
Tom
