Re: [PATCH 23/32] tracing: Add 'onmatch' hist trigger action support

Namhyung Kim
2017-07-23T15:13:28Z

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.zanussi@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.

> 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?

Thanks,
Namhyung

>
> if (argc < 2) {
> ret = -EINVAL;