Re: [PATCH v11 24/28] tracing: Add support for multiple hist triggers per event

From: Namhyung Kim
Date: Mon Nov 02 2015 - 19:34:55 EST


On Thu, Oct 22, 2015 at 01:14:28PM -0500, Tom Zanussi wrote:
> Allow users to define any number of hist triggers per trace event.
> Any number of hist triggers may be added for a given event, which may
> differ by key, value, or filter.
>
> Reading the event's 'hist' file will display the output of all the
> hist triggers defined on an event concatenated in the order they were
> defined.
>
> Signed-off-by: Tom Zanussi <tom.zanussi@xxxxxxxxxxxxxxx>
> Tested-by: Masami Hiramatsu <masami.hiramatsu.pt@xxxxxxxxxxx>
> ---
> Documentation/trace/events.txt | 151 +++++++++++++++++++++++++++++++++++++--
> kernel/trace/trace.c | 8 ++-
> kernel/trace/trace_events_hist.c | 138 ++++++++++++++++++++++++++---------
> 3 files changed, 256 insertions(+), 41 deletions(-)
>
> diff --git a/Documentation/trace/events.txt b/Documentation/trace/events.txt
> index b3aa47e..6c64cb7 100644
> --- a/Documentation/trace/events.txt
> +++ b/Documentation/trace/events.txt
> @@ -532,12 +532,14 @@ The following commands are supported:
>
> 'hist' triggers add a 'hist' file to each event's subdirectory.
> Reading the 'hist' file for the event will dump the hash table in
> - its entirety to stdout. Each printed hash table entry is a simple
> - list of the keys and values comprising the entry; keys are printed
> - first and are delineated by curly braces, and are followed by the
> - set of value fields for the entry. By default, numeric fields are
> - displayed as base-10 integers. This can be modified by appending
> - any of the following modifiers to the field name:
> + its entirety to stdout. If there are multiple hist triggers
> + attached to an event, there will be a table for each trigger in the
> + output. Each printed hash table entry is a simple list of the keys
> + and values comprising the entry; keys are printed first and are
> + delineated by curly braces, and are followed by the set of value
> + fields for the entry. By default, numeric fields are displayed as
> + base-10 integers. This can be modified by appending any of the
> + following modifiers to the field name:
>
> .hex display a number as a hex value
> .sym display an address as a symbol
> @@ -1629,3 +1631,140 @@ The following commands are supported:
> .
> .
> .
> +
> + The following example demonstrates how multiple hist triggers can be
> + attached to a given event. This capability can be useful for
> + creating a set of different summaries derived from the same set of
> + events, or for comparing the effects of different filters, among
> + other things.
> +
> + # echo 'hist:keys=skbaddr.hex:vals=len if len < 0' > \
> + /sys/kernel/debug/tracing/events/net/netif_receive_skb/trigger
> + # echo 'hist:keys=skbaddr.hex:vals=len if len > 4096' > \
> + /sys/kernel/debug/tracing/events/net/netif_receive_skb/trigger
> + # echo 'hist:keys=skbaddr.hex:vals=len if len == 256' > \
> + /sys/kernel/debug/tracing/events/net/netif_receive_skb/trigger
> + # echo 'hist:keys=skbaddr.hex:vals=len' > \
> + /sys/kernel/debug/tracing/events/net/netif_receive_skb/trigger
> + # echo 'hist:keys=len:vals=common_preempt_count' > \
> + /sys/kernel/debug/tracing/events/net/netif_receive_skb/trigger

AFAIK other tracefs files honor the truncation flag so open for
writing would destroy other hist triggers. What do you think?


> +
> + The above set of commands create four triggers differing only in
> + their filters, along with a completely different though fairly
> + nonsensical trigger.

[SNIP]
> @@ -1289,22 +1368,18 @@ static int event_hist_trigger_func(struct event_command *cmd_ops,
>
> trigger_data->private_data = hist_data;
>
> + if (param) { /* if param is non-empty, it's supposed to be a filter */
> + ret = cmd_ops->set_filter(param, trigger_data, file);

Maybe you want to check ->set_filter being NULL first. :)

Thanks,
Namhyung


> + if (ret < 0)
> + goto out_free;
> + }
> +
> if (glob[0] == '!') {
> cmd_ops->unreg(glob+1, trigger_ops, trigger_data, file);
> ret = 0;
> goto out_free;
> }
>
> - if (!param) /* if param is non-empty, it's supposed to be a filter */
> - goto out_reg;
> -
> - if (!cmd_ops->set_filter)
> - goto out_reg;
> -
> - ret = cmd_ops->set_filter(param, trigger_data, file);
> - if (ret < 0)
> - goto out_free;
> - out_reg:
> ret = cmd_ops->reg(glob, trigger_ops, trigger_data, file);
> /*
> * The above returns on success the # of triggers registered,
> @@ -1337,7 +1412,7 @@ static struct event_command trigger_hist_cmd = {
> .needs_rec = true,
> .func = event_hist_trigger_func,
> .reg = hist_register_trigger,
> - .unreg = unregister_trigger,
> + .unreg = hist_unregister_trigger,
> .get_trigger_ops = event_hist_get_trigger_ops,
> .set_filter = set_trigger_filter,
> };
> @@ -1364,7 +1439,6 @@ hist_enable_trigger(struct event_trigger_data *data, void *rec)
> test->paused = false;
> else
> test->paused = true;
> - break;
> }
> }
> }
> --
> 1.9.3
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/