Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754066AbbGWP6n (ORCPT ); Thu, 23 Jul 2015 11:58:43 -0400 Received: from mga09.intel.com ([134.134.136.24]:24672 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754027AbbGWP62 (ORCPT ); Thu, 23 Jul 2015 11:58:28 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.15,531,1432623600"; d="scan'208";a="611582520" Message-ID: <1437667104.20186.8.camel@tzanussi-mobl.amr.corp.intel.com> Subject: Re: 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: Thu, 23 Jul 2015 10:58:24 -0500 In-Reply-To: <55B0F4DF.9050801@hitachi.com> References: <55AF523C.10601@hitachi.com> <1437596557.22524.88.camel@tzanussi-mobl.amr.corp.intel.com> <55B0F4DF.9050801@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: 7359 Lines: 198 On Thu, 2015-07-23 at 23:06 +0900, Masami Hiramatsu wrote: > On 2015/07/23 5:22, Tom Zanussi wrote: > > 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. > > Agreed. > > BTW, as I've wrote in another reply, pause/cont/clear commands should be > for the "hist" file, not for trigger. So, below command will be better. > > # echo pause:name=foo > event/hist > # echo clear:keys=xxx > event/hist > Right, that all makes sense, I think we're on the same page with everything now.. Thanks, Tom > Thank you, > > > > >> [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/ > > > > -- 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/