Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752819AbbGVUWk (ORCPT ); Wed, 22 Jul 2015 16:22:40 -0400 Received: from mga02.intel.com ([134.134.136.20]:57723 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752591AbbGVUWj (ORCPT ); Wed, 22 Jul 2015 16:22:39 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.15,525,1432623600"; d="scan'208";a="610996285" Message-ID: <1437596557.22524.88.camel@tzanussi-mobl.amr.corp.intel.com> Subject: Re: [PATCH v9 12/22] tracing: Add hist trigger support for pausing and continuing a trace From: Tom Zanussi To: Masami Hiramatsu Cc: rostedt@goodmis.org, daniel.wagner@bmw-carit.de, namhyung@kernel.org, josh@joshtriplett.org, andi@firstfloor.org, linux-kernel@vger.kernel.org Date: Wed, 22 Jul 2015 15:22:37 -0500 In-Reply-To: <55AF523C.10601@hitachi.com> References: <55AF523C.10601@hitachi.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.10.4 (3.10.4-4.fc20) Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6250 Lines: 167 Hi Masami, On Wed, 2015-07-22 at 17:20 +0900, Masami Hiramatsu wrote: > Hi Tom, > > On 2015/07/17 2:22, Tom Zanussi wrote: > > Allow users to append 'pause' or 'continue' to an existing trigger in > > order to have it paused or to have a paused trace continue. > > > > This expands the hist trigger syntax from this: > > # echo hist:keys=xxx:vals=yyy:sort=zzz.descending \ > > [ if filter] > event/trigger > > > > to this: > > > > # echo hist:keys=xxx:vals=yyy:sort=zzz.descending:pause or cont \ > > [ if filter] > event/trigger > > Since the only one hist trigger can be set on one event, it seems > that we don't need keys for pause/cont/clear (e.g. hist:pause is enough). > Anyway, I've found an odd behavior. Right, because currently there is only one hist trigger per event, the key is ignored, which also accounts for the 'odd' behavior. But rather than saying it's expected and document it, I think the conclusion from the other comments are that we'll be allowing multiple hist triggers per event, and in that case, they need to be uniquely identifiable by both key and filter. > > [root@localhost tracing]# echo 'hist:keys=parent_pid' > events/sched/sched_process_fork/trigger > [root@localhost tracing]# echo 'hist:keys=common_pid:pause' > events/sched/sched_process_fork/trigger > [root@localhost tracing]# cat events/sched/sched_process_fork/trigger > hist:keys=parent_pid:vals=hitcount:sort=hitcount:size=2048 [paused] > > So, the second "pause" command can work with different keys. > Moreover, I can remove it with different keys. > Right, this goes away once we have code that deals with multiple histograms per event, which I'll go ahead and implement rather than document the confusion... Tom > [root@localhost tracing]# echo '!hist:keys=child_pid' > events/sched/sched_process_fork/trigger > [root@localhost tracing]# cat events/sched/sched_process_fork/trigger > # Available triggers: > # traceon traceoff snapshot stacktrace enable_event disable_event enable_hist disable_hist hist > > Thank you, > > > > > Signed-off-by: Tom Zanussi > > --- > > kernel/trace/trace.c | 5 +++++ > > kernel/trace/trace_events_hist.c | 26 +++++++++++++++++++++++--- > > 2 files changed, 28 insertions(+), 3 deletions(-) > > > > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c > > index 5dd1fc4..547bbc8 100644 > > --- a/kernel/trace/trace.c > > +++ b/kernel/trace/trace.c > > @@ -3791,6 +3791,7 @@ static const char readme_msg[] = > > "\t [:values= > "\t [:sort=field1,field2,...]\n" > > "\t [:size=#entries]\n" > > + "\t [:pause][:continue]\n" > > "\t [if ]\n\n" > > "\t When a matching event is hit, an entry is added to a hash\n" > > "\t table using the key(s) and value(s) named. Keys and values\n" > > @@ -3821,6 +3822,10 @@ static const char readme_msg[] = > > "\t on. The default if unspecified is 'hitcount' and the.\n" > > "\t default sort order is 'ascending'. To sort in the opposite\n" > > "\t direction, append .descending' to the sort key.\n\n" > > + "\t The 'pause' param can be used to pause an existing hist\n" > > + "\t trigger or to start a hist trigger but not log any events\n" > > + "\t until told to do so. 'continue' can be used to start or\n" > > + "\t restart a paused hist trigger.\n\n" > > #endif > > ; > > > > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c > > index 6bf224f..3ae58e7 100644 > > --- a/kernel/trace/trace_events_hist.c > > +++ b/kernel/trace/trace_events_hist.c > > @@ -78,6 +78,8 @@ struct hist_trigger_attrs { > > char *keys_str; > > char *vals_str; > > char *sort_key_str; > > + bool pause; > > + bool cont; > > unsigned int map_bits; > > }; > > > > @@ -184,6 +186,11 @@ static struct hist_trigger_attrs *parse_hist_trigger_attrs(char *trigger_str) > > attrs->vals_str = kstrdup(str, GFP_KERNEL); > > else if (!strncmp(str, "sort", strlen("sort"))) > > attrs->sort_key_str = kstrdup(str, GFP_KERNEL); > > + else if (!strncmp(str, "pause", strlen("pause"))) > > + attrs->pause = true; > > + else if (!strncmp(str, "continue", strlen("continue")) || > > + !strncmp(str, "cont", strlen("cont"))) > > + attrs->cont = true; > > else if (!strncmp(str, "size", strlen("size"))) { > > int map_bits = parse_map_size(str); > > > > @@ -843,7 +850,10 @@ static int event_hist_trigger_print(struct seq_file *m, > > if (data->filter_str) > > seq_printf(m, " if %s", data->filter_str); > > > > - seq_puts(m, " [active]"); > > + if (data->paused) > > + seq_puts(m, " [paused]"); > > + else > > + seq_puts(m, " [active]"); > > > > seq_putc(m, '\n'); > > > > @@ -882,16 +892,25 @@ static int hist_register_trigger(char *glob, struct event_trigger_ops *ops, > > struct event_trigger_data *data, > > struct trace_event_file *file) > > { > > + struct hist_trigger_data *hist_data = data->private_data; > > struct event_trigger_data *test; > > int ret = 0; > > > > list_for_each_entry_rcu(test, &file->triggers, list) { > > if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) { > > - ret = -EEXIST; > > + if (hist_data->attrs->pause) > > + test->paused = true; > > + else if (hist_data->attrs->cont) > > + test->paused = false; > > + else > > + ret = -EEXIST; > > goto out; > > } > > } > > > > + if (hist_data->attrs->pause) > > + data->paused = true; > > + > > if (data->ops->init) { > > ret = data->ops->init(data->ops, data); > > if (ret < 0) > > @@ -984,7 +1003,8 @@ static int event_hist_trigger_func(struct event_command *cmd_ops, > > * triggers registered a failure too. > > */ > > if (!ret) { > > - ret = -ENOENT; > > + if (!(attrs->pause || attrs->cont)) > > + ret = -ENOENT; > > goto out_free; > > } else if (ret < 0) > > goto out_free; > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/